Repository: sqlfluff/sqlfluff

Issue:
When linter loop limit is hit, should report an error/exception
When the loop limit is hit, `sqlfluff fix` logs a warning:
```
Loop limit on fixes reached
```

This is a pretty "quiet" notification that can mask issues. Suggest we change this:
* Increase the default loop limit from 10 to 25 iterations.
* Log an ERROR and/or raise an exception. If an exception, suggest (maybe?) raising one that is caught and included in the list of lint issues, not one that stops the `fix` run. Open to other ideas, but I think this is the right tradeoff between raising visibility of what is _likely_ an infinite loop in the linter run while still giving the user useful `fix` output.

Related to #1344, among others.


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) Report
> Merging [#2628](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) (e169a0e) into [main](https://codecov.io/gh/sqlfluff/sqlfluff/commit/13f732de1a08ea7a6437392ea162dba016acb8fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) (13f732d) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628/graphs/tree.svg?width=650&height=150&src=pr&token=maM8kLgCmn&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff)](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff)

```diff
@@             Coverage Diff             @@
##              main    #2628      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          163      163              
  Lines        11915    11923       +8     
===========================================
+ Hits         11915    11921       +6     
- Misses           0        2       +2     
```


| [Impacted Files](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) | Coverage Δ | |
|---|---|---|
| [src/sqlfluff/core/linter/linter.py](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff#diff-c3JjL3NxbGZsdWZmL2NvcmUvbGludGVyL2xpbnRlci5weQ==) | `99.48% <100.00%> (-0.52%)` | :arrow_down: |
| [src/sqlfluff/core/rules/base.py](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff#diff-c3JjL3NxbGZsdWZmL2NvcmUvcnVsZXMvYmFzZS5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff). Last update [13f732d...e169a0e](https://codecov.io/gh/sqlfluff/sqlfluff/pull/2628?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff).

barrywhart: @juhoautio: Given there is work remaining to do on #2624 (i.e. rules that are currently causing the additional lint check to fail), I think your suggestion about the linter loop should be a separate issue after all. It's a great idea, but we shouldn't add it while there are still known issues.

Patch:
diff --git a/src/sqlfluff/core/linter/linter.py b/src/sqlfluff/core/linter/linter.py
index eaaaf8a7f..f86d936a3 100644
--- a/src/sqlfluff/core/linter/linter.py
+++ b/src/sqlfluff/core/linter/linter.py
@@ -483,6 +483,7 @@ class Linter:
         else:
             ignore_buff = []
 
+        save_tree = tree
         for loop in range(loop_limit):
             changed = False
 
@@ -542,8 +543,31 @@ class Linter:
                     "loops."
                 )
                 break
-        if fix and loop + 1 == loop_limit:
-            linter_logger.warning(f"Loop limit on fixes reached [{loop_limit}].")
+        else:
+            if fix:
+                # The linter loop hit the limit before reaching a stable point
+                # (i.e. free of lint errors). If this happens, it's usually
+                # because one or more rules produced fixes which did not address
+                # the original issue **or** created new issues.
+                linter_logger.warning(f"Loop limit on fixes reached [{loop_limit}].")
+
+                # Discard any fixes for the linting errors, since they caused a
+                # loop. IMPORTANT: By doing this, we are telling SQLFluff that
+                # these linting errors are "unfixable". This is important,
+                # because when "sqlfluff fix" encounters unfixable lint errors,
+                # it exits with a "failure" exit code, which is exactly what we
+                # want in this situation. (Reason: Although this is more of an
+                # internal SQLFluff issue, users deserve to know about it,
+                # because it means their file(s) weren't fixed.
+                for violation in initial_linting_errors:
+                    if isinstance(violation, SQLLintError):
+                        violation.fixes = []
+
+                # Return the original parse tree, before any fixes were applied.
+                # Reason: When the linter hits the loop limit, the file is often
+                # messy, e.g. some of the fixes were applied repeatedly, possibly
+                # other weird things. We don't want the user to see this junk!
+                return save_tree, initial_linting_errors, ignore_buff
 
         if config.get("ignore_templated_areas", default=True):
             initial_linting_errors = cls.remove_templated_errors(initial_linting_errors)
diff --git a/src/sqlfluff/core/rules/base.py b/src/sqlfluff/core/rules/base.py
index 2706f9d79..fed7ff602 100644
--- a/src/sqlfluff/core/rules/base.py
+++ b/src/sqlfluff/core/rules/base.py
@@ -555,23 +555,8 @@ class BaseRule:
             )
             return vs, raw_stack, fixes, memory
 
-        new_lerrs = []
-        new_fixes = []
-
-        def _process_lint_result(res):
-            self.discard_unsafe_fixes(res, templated_file)
-            lerr = res.to_linting_error(rule=self)
-            ignored = False
-            if lerr:
-                if ignore_mask:
-                    filtered = LintedFile.ignore_masked_violations([lerr], ignore_mask)
-                    if not filtered:
-                        lerr = None
-                        ignored = True
-            if lerr:
-                new_lerrs.append(lerr)
-            if not ignored:
-                new_fixes.extend(res.fixes)
+        new_lerrs: List[SQLLintError] = []
+        new_fixes: List[LintFix] = []
 
         if res is None:
             # Assume this means no problems (also means no memory)
@@ -579,7 +564,9 @@ class BaseRule:
         elif isinstance(res, LintResult):
             # Extract any memory
             memory = res.memory
-            _process_lint_result(res)
+            self._process_lint_result(
+                res, templated_file, ignore_mask, new_lerrs, new_fixes
+            )
         elif isinstance(res, list) and all(
             isinstance(elem, LintResult) for elem in res
         ):
@@ -587,7 +574,9 @@ class BaseRule:
             # it was the last to be added
             memory = res[-1].memory
             for elem in res:
-                _process_lint_result(elem)
+                self._process_lint_result(
+                    elem, templated_file, ignore_mask, new_lerrs, new_fixes
+                )
         else:  # pragma: no cover
             raise TypeError(
                 "Got unexpected result [{!r}] back from linting rule: {!r}".format(
@@ -629,6 +618,23 @@ class BaseRule:
 
     # HELPER METHODS --------
 
+    def _process_lint_result(
+        self, res, templated_file, ignore_mask, new_lerrs, new_fixes
+    ):
+        self.discard_unsafe_fixes(res, templated_file)
+        lerr = res.to_linting_error(rule=self)
+        ignored = False
+        if lerr:
+            if ignore_mask:
+                filtered = LintedFile.ignore_masked_violations([lerr], ignore_mask)
+                if not filtered:
+                    lerr = None
+                    ignored = True
+        if lerr:
+            new_lerrs.append(lerr)
+        if not ignored:
+            new_fixes.extend(res.fixes)
+
     @cached_property
     def indent(self) -> str:
         """String for a single indent, based on configuration."""


Test patch:
diff --git a/test/cli/commands_test.py b/test/cli/commands_test.py
index 522109509..1104e87b9 100644
--- a/test/cli/commands_test.py
+++ b/test/cli/commands_test.py
@@ -21,6 +21,8 @@ from click.testing import CliRunner
 # We import the library directly here to get the version
 import sqlfluff
 from sqlfluff.cli.commands import lint, version, rules, fix, parse, dialects, get_config
+from sqlfluff.core.rules.base import BaseRule, LintFix, LintResult
+from sqlfluff.core.parser.segments.raw import CommentSegment
 
 
 def invoke_assert_code(
@@ -670,6 +672,66 @@ def test__cli__fix_error_handling_behavior(sql, fix_args, fixed, exit_code, tmpd
             assert not fixed_path.is_file()
 
 
+_old_crawl = BaseRule.crawl
+_fix_counter = 0
+
+
+def _mock_crawl(rule, segment, ignore_mask, templated_file=None, *args, **kwargs):
+    # For test__cli__fix_loop_limit_behavior, we mock BaseRule.crawl(),
+    # replacing it with this function. This function generates an infinite
+    # sequence of fixes without ever repeating the same fix. This causes the
+    # linter to hit the loop limit, allowing us to test that behavior.
+    if segment.is_type("comment") and "Comment" in segment.raw:
+        global _fix_counter
+        _fix_counter += 1
+        fix = LintFix.replace(segment, [CommentSegment(f"-- Comment {_fix_counter}")])
+        result = LintResult(segment, fixes=[fix])
+        errors = []
+        fixes = []
+        rule._process_lint_result(result, templated_file, ignore_mask, errors, fixes)
+        return (
+            errors,
+            None,
+            fixes,
+            None,
+        )
+    else:
+        return _old_crawl(
+            rule, segment, ignore_mask, templated_file=templated_file, *args, **kwargs
+        )
+
+
+@pytest.mark.parametrize(
+    "sql, exit_code",
+    [
+        ("-- Comment A\nSELECT 1 FROM foo", 1),
+        ("-- noqa: disable=all\n-- Comment A\nSELECT 1 FROM foo", 0),
+    ],
+)
+@patch("sqlfluff.core.rules.base.BaseRule.crawl", _mock_crawl)
+def test__cli__fix_loop_limit_behavior(sql, exit_code, tmpdir):
+    """Tests how "fix" behaves when the loop limit is exceeded."""
+    fix_args = ["--force", "--fixed-suffix", "FIXED", "--rules", "L001"]
+    tmp_path = pathlib.Path(str(tmpdir))
+    filepath = tmp_path / "testing.sql"
+    filepath.write_text(textwrap.dedent(sql))
+    with tmpdir.as_cwd():
+        with pytest.raises(SystemExit) as e:
+            fix(
+                fix_args
+                + [
+                    "-f",
+                ]
+            )
+        assert exit_code == e.value.code
+    # In both parametrized test cases, no output file should have been
+    # created.
+    # - Case #1: Hitting the loop limit is an error
+    # - Case #2: "noqa" suppressed all lint errors, thus no fixes applied
+    fixed_path = tmp_path / "testingFIXED.sql"
+    assert not fixed_path.is_file()
+
+
 # Test case disabled because there isn't a good example of where to test this.
 # This *should* test the case where a rule DOES have a proposed fix, but for
 # some reason when we try to apply it, there's a failure.
diff --git a/test/core/rules/config_test.py b/test/core/rules/config_test.py
index 35580643c..e3674fbc5 100644
--- a/test/core/rules/config_test.py
+++ b/test/core/rules/config_test.py
@@ -56,9 +56,8 @@ def test__rules__runaway_fail_catch():
     # In theory this step should result in an infinite
     # loop, but the loop limit should catch it.
     linted = linter.lint_string(my_query, fix=True)
-    # We should have a lot of newlines in there.
-    # The number should equal the runaway limit
-    assert linted.tree.raw.count("\n") == runaway_limit
+    # When the linter hits the runaway limit, it returns the original SQL tree.
+    assert linted.tree.raw == my_query
 
 
 def test_rules_cannot_be_instantiated_without_declared_configs():
diff --git a/test/fixtures/rules/std_rule_cases/L019.yml b/test/fixtures/rules/std_rule_cases/L019.yml
index 32bb81101..6f842aa68 100644
--- a/test/fixtures/rules/std_rule_cases/L019.yml
+++ b/test/fixtures/rules/std_rule_cases/L019.yml
@@ -230,10 +230,10 @@ trailing_comma_move_past_several_comment_lines:
     global_actions_states
   configs:
     core:
-      # Set runaway_limit=1 to verify the fix only requires one pass. In an
+      # Set runaway_limit=2 to verify the fix only requires one pass. In an
       # earlier version, the comma before "SAFE_DIVIDE()" was being moved one
       # line per pass. Too lazy!
-      runaway_limit: 1
+      runaway_limit: 2
 
 
 leading_comma_move_past_several_comment_lines:
@@ -267,10 +267,10 @@ leading_comma_move_past_several_comment_lines:
     global_actions_states
   configs:
     core:
-      # Set runaway_limit=1 to verify the fix only requires one pass. In an
+      # Set runaway_limit=2 to verify the fix only requires one pass. In an
       # earlier version for the trailing comma case, commas were being moved
       # "through" comment blocks one line per pass. Too lazy!
-      runaway_limit: 1
+      runaway_limit: 2
     rules:
       L019:
         comma_style: leading