Repository: sqlfluff/sqlfluff
Pull request: Fix some issues where the SQL file is corrupted by lint "fixes" in or near Jinja loops
Fixes #1162, #1425 

### Are there any other side effects of this change that we should be aware of?

This PR adds a couple of new internal features to SQLFluff:
* `TemplatedFile` adds a new property, `raw_slice_block_info`, which identifies logical sections of the templated file: e.g. blocks and loops.
* All `LintResult`s now get their fixes automatically analyzed in terms of template structure. Unsafe fixes are automatically discarded, e.g. moving code into or out of a loop.

### Pull Request checklist
- [X] Please confirm you have completed any of the necessary steps below.

- Included test cases to demonstrate any code changes, which may be one or more of the following:
  - `.yml` rule test cases in `test/fixtures/rules/std_rule_cases`.
  - `.sql`/`.yml` parser test cases in `test/fixtures/parser` (note YML files can be auto generated with `python test/generate_parse_fixture_yml.py` or by running `tox` locally).
  - Full autofix test cases in `test/fixtures/linter/autofix`.
  - Other.
- Added appropriate documentation for the change.
- Created GitHub issues for any relevant followup/future enhancements if appropriate.


Problem statement:
SQL JINJA IS EXPLODING with sqlfluff fix
-- If this is a parsing or linting issue, please include a minimal SQL example which reproduces the issue, along with the `sqlfluff parse` output, `sqlfluff lint` output and `sqlfluff fix` output when relevant. Finally, please add any labels that are related to the issue before submitting so that it can be addressed easily! --

SIMPLE SQL QUERY
```
SELECT
    apple
    , {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col
      {% endfor -%}
    , pear
from fruit f
```

## Expected Behaviour

```
SELECT
    apple,
    {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col
      {% endfor -%}
    pear
FROM fruit
```

## Observed Behaviour
```
SELECT
    apple,
    {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col
      {% endfor -%}
    CASE WHEN 1 = 1 THEN cool
            ELSE moo END as 1_col

            CASE WHEN 2 = 1 THEN cool
            ELSE moo END as 2_col

            CASE WHEN 3 = 1 THEN cool
            ELSE moo END as 3_col
FROM fruit
```

## Steps to Reproduce
create a file called test.sql with
```
SELECT
    apple,
    {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col
      {% endfor -%}
    pear
FROM fruit

```


## Version
Include the output of `sqlfluff --version` along with your Python version

sqlfluff, version 0.6.0
Python 3.7.9
## Configuration
```
Include your SQLFluff configuration here
```
```
[sqlfluff]
dialect = snowflake
load_macros_from_path = macros

[sqlfluff:rules:L010]
capitalisation_policy = upper

#[sqlfluff:rules:L016]
#max_line_length = 999

[sqlfluff:rules:L019]
comma_style = trailing
```

Hints from issue discussion:
@jonny-mills: If you run `sqlfluff lint` on this file, are you seeing a parse error? I'm seeing this (note the parse error on line 3):
```
sqlfluff lint test.sql
== [test.sql] FAIL
L:   3 | P:  37 |  PRS | Line 5, Position 25: Found unparsable section: ' as
                       | 1_col\n \n CASE WHEN 2...'
L:   4 | P:  13 | L003 | Line over-indented compared to line #2
L:   6 | P:   1 | L001 | Unnecessary trailing whitespace.
L:   6 | P:   1 | L001 | Unnecessary trailing whitespace.
L:   8 | P:   1 | L003 | Indentation not consistent with line #2
```

I'm not saying that SQLFluff is behaving correctly, just trying to understand, reproduce, and prioritize issues.
The parse error appears to be caused by a missing comma at the end of line 5. Suggest testing with this instead:
```
SELECT
    apple,
    {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col,
      {% endfor %}
    pear
FROM fruit
```

`sqlfluff fix` still corrupts the output file, although it looks a bit different.
```
SELECT
    apple,
    {% for num in ('1', '2', '3') %}
            CASE WHEN {{ num }} = 1 THEN cool
            ELSE moo END as {{ num }}_col,
{% endfor %}
    CASE WHEN 3 = 1 THEN cool
               ELSE moo END as 3_col
FROM fruit
```

It would be cool if SQLFluff could actually _fix_ the template. At the very least, it should do nothing, rather than corrupt the code inside a Jinja `for` loop. (I bet there are similar issues with Jinja `if` statements.)
As I understand it SQLFluff expands templates to get real SQL and then applies fixes and then has to map fixes back to unexpanded, original template. I think that’s very tricky to do with more complex actions like for loops. I also understand Jinja doesn’t give an API mapping the two versions back cleanly so it’s something we’ve tried to implement on top.

So I think we should go one of two routes:
1. If certain keywords are used (e.g. `{% for`) then it should refuse to `fix`, but possibly still `lint` instead. That might be simplest option.
2. A better (but more tricky option) would be to see if the two versions of the file are mappable back to each other (if this is possible and accurate - dunno if it is) and the only `fix` of it it, and fall back to `lint` if it isn’t. This would basically be option 1 with a few more use cases covered (e.g. for loops with only 1 loop).

Commit message: Black

Patch to review:
diff --git a/src/sqlfluff/core/linter/linted_dir.py b/src/sqlfluff/core/linter/linted_dir.py
index c1492fb0e93..d427b23a512 100644
--- a/src/sqlfluff/core/linter/linted_dir.py
+++ b/src/sqlfluff/core/linter/linted_dir.py
@@ -57,7 +57,7 @@ def check_tuples(self, by_path: bool = False):  # pragma: no cover
         """Default overload method."""
         ...
 
-    def check_tuples(self, by_path=False):
+    def check_tuples(self, by_path=False, raise_on_non_linting_violations=True):
         """Compress all the tuples into one list.
 
         NB: This is a little crude, as you can't tell which
@@ -65,11 +65,18 @@ def check_tuples(self, by_path=False):
         For more control set the `by_path` argument to true.
         """
         if by_path:
-            return {file.path: file.check_tuples() for file in self.files}
+            return {
+                file.path: file.check_tuples(
+                    raise_on_non_linting_violations=raise_on_non_linting_violations
+                )
+                for file in self.files
+            }
         else:
             tuple_buffer: List[CheckTuple] = []
             for file in self.files:
-                tuple_buffer += file.check_tuples()
+                tuple_buffer += file.check_tuples(
+                    raise_on_non_linting_violations=raise_on_non_linting_violations
+                )
             return tuple_buffer
 
     def num_violations(self, **kwargs) -> int:
diff --git a/src/sqlfluff/core/linter/linted_file.py b/src/sqlfluff/core/linter/linted_file.py
index 5c4342e0fb8..d2eb7bfbf7a 100644
--- a/src/sqlfluff/core/linter/linted_file.py
+++ b/src/sqlfluff/core/linter/linted_file.py
@@ -47,7 +47,7 @@ class LintedFile(NamedTuple):
     templated_file: TemplatedFile
     encoding: str
 
-    def check_tuples(self) -> List[CheckTuple]:
+    def check_tuples(self, raise_on_non_linting_violations=True) -> List[CheckTuple]:
         """Make a list of check_tuples.
 
         This assumes that all the violations found are
@@ -59,7 +59,7 @@ def check_tuples(self) -> List[CheckTuple]:
         for v in self.get_violations():
             if hasattr(v, "check_tuple"):
                 vs.append(v.check_tuple())
-            else:
+            elif raise_on_non_linting_violations:
                 raise v
         return vs
 
@@ -205,15 +205,19 @@ def _log_hints(
     ):
         """Log hints for debugging during patch generation."""
         # This next bit is ALL FOR LOGGING AND DEBUGGING
-        if patch.templated_slice.start >= 10:
+        max_log_length = 10
+        if patch.templated_slice.start >= max_log_length:
             pre_hint = templated_file.templated_str[
-                patch.templated_slice.start - 10 : patch.templated_slice.start
+                patch.templated_slice.start
+                - max_log_length : patch.templated_slice.start
             ]
         else:
             pre_hint = templated_file.templated_str[: patch.templated_slice.start]
-        if patch.templated_slice.stop + 10 < len(templated_file.templated_str):
+        if patch.templated_slice.stop + max_log_length < len(
+            templated_file.templated_str
+        ):
             post_hint = templated_file.templated_str[
-                patch.templated_slice.stop : patch.templated_slice.stop + 10
+                patch.templated_slice.stop : patch.templated_slice.stop + max_log_length
             ]
         else:
             post_hint = templated_file.templated_str[patch.templated_slice.stop :]
@@ -263,6 +267,9 @@ def fix_string(self) -> Tuple[Any, bool]:
         # Make sure no patches overlap and divide up the source file into slices.
         # Any Template tags in the source file are off limits.
         source_only_slices = self.templated_file.source_only_slices()
+        raw_slices_in_template_loop = set(
+            self.templated_file.raw_slices_in_template_loop()
+        )
 
         linter_logger.debug("Source-only slices: %s", source_only_slices)
 
@@ -314,6 +321,21 @@ def fix_string(self) -> Tuple[Any, bool]:
             )
             local_type_list = [slc.slice_type for slc in local_raw_slices]
 
+            if set(local_raw_slices).intersection(raw_slices_in_template_loop):
+                # The patch is being applied to a loop body. Sanity check it.
+                patch_split_list = patch.fixed_raw.split()
+                patch_split_set = set(patch.fixed_raw.split())
+                if len(patch_split_list) - len(patch_split_set) >= 2:
+                    # This is a heuristic. If the patch contains 2 or more
+                    # duplicate strings, assume it's a poorly computed patch
+                    # where the fixes to expanded code couldn't be applied
+                    # cleanly to the source code.
+                    linter_logger.info(
+                        "      - Skipping apparent bad patch inside template loop: %s",
+                        patch,
+                    )
+                    continue
+
             enriched_patch = EnrichedFixPatch(
                 source_slice=source_slice,
                 templated_slice=patch.templated_slice,
@@ -378,7 +400,7 @@ def fix_string(self) -> Tuple[Any, bool]:
                         enriched_patch,
                     )
                     continue
-                # We have a single occurrences of the thing we want to patch. This
+                # We have a single occurrence of the thing we want to patch. This
                 # means we can use its position to place our patch.
                 new_source_slice = slice(
                     enriched_patch.source_slice.start + positions[0],
diff --git a/src/sqlfluff/core/parser/segments/base.py b/src/sqlfluff/core/parser/segments/base.py
index bd03fa1b47c..500321368a6 100644
--- a/src/sqlfluff/core/parser/segments/base.py
+++ b/src/sqlfluff/core/parser/segments/base.py
@@ -971,6 +971,13 @@ def iter_patches(self, templated_str: str) -> Iterator[FixPatch]:
             return
 
         # If we're here, the segment doesn't match the original.
+        linter_logger.debug(
+            "%s at %s: Original: [%r] Fixed: [%r]",
+            type(self).__name__,
+            self.pos_marker.templated_slice,
+            templated_str[self.pos_marker.templated_slice],
+            self.raw,
+        )
 
         # If it's all literal, then we don't need to recurse.
         if self.pos_marker.is_literal():
@@ -980,7 +987,7 @@ def iter_patches(self, templated_str: str) -> Iterator[FixPatch]:
             )
         # Can we go deeper?
         elif not self.segments:
-            # It's not literal, but it's also a raw segment. If were going
+            # It's not literal, but it's also a raw segment. If we're going
             # to yield a change, we would have done it from the parent, so
             # we just abort from here.
             return  # pragma: no cover TODO?
diff --git a/src/sqlfluff/core/templaters/base.py b/src/sqlfluff/core/templaters/base.py
index e41857408bb..bb8905e560a 100644
--- a/src/sqlfluff/core/templaters/base.py
+++ b/src/sqlfluff/core/templaters/base.py
@@ -59,6 +59,7 @@ class RawFileSlice(NamedTuple):
     raw: str
     slice_type: str
     source_idx: int
+    slice_subtype: Optional[str] = None
 
     def end_source_idx(self):
         """Return the closing index of this slice."""
@@ -198,6 +199,24 @@ def _find_slice_indices_of_templated_pos(
             raise ValueError("Position Not Found")
         return first_idx, last_idx
 
+    def raw_slices_in_template_loop(self):
+        """Returns the raw slices in this file inside template loops."""
+        result = []
+        blocks = []
+        loop_level = 0
+        for idx, raw_slice in enumerate(self.raw_sliced):
+            if raw_slice.slice_type == "block_start":
+                blocks.append(raw_slice)
+                if raw_slice.slice_subtype == "loop":
+                    loop_level += 1
+            elif raw_slice.slice_type == "block_end":
+                exiting = blocks.pop()
+                if exiting.slice_subtype == "loop":
+                    loop_level -= 1
+            elif loop_level > 0:
+                result.append(raw_slice)
+        return result
+
     def raw_slices_spanning_source_slice(self, source_slice: slice):
         """Return a list of the raw slices spanning a set of indices."""
         # First find the start index
@@ -335,7 +354,7 @@ def is_source_slice_literal(self, source_slice: slice) -> bool:
         if source_slice.start == source_slice.stop:
             return True
         is_literal = True
-        for _, seg_type, seg_idx in self.raw_sliced:
+        for _, seg_type, seg_idx, _ in self.raw_sliced:
             # Reset if we find a literal and we're up to the start
             # otherwise set false.
             if seg_idx <= source_slice.start:
diff --git a/src/sqlfluff/core/templaters/jinja.py b/src/sqlfluff/core/templaters/jinja.py
index dd90a7dfbb5..c40e185d4de 100644
--- a/src/sqlfluff/core/templaters/jinja.py
+++ b/src/sqlfluff/core/templaters/jinja.py
@@ -339,6 +339,7 @@ def _slice_template(cls, in_str: str) -> Iterator[RawFileSlice]:
             # parts of the tag at a time.
             if elem_type.endswith("_end") or elem_type == "raw_begin":
                 block_type = block_types[elem_type]
+                block_subtype = None
                 # Handle starts and ends of blocks
                 if block_type == "block":
                     # Trim off the brackets and then the whitespace
@@ -350,7 +351,9 @@ def _slice_template(cls, in_str: str) -> Iterator[RawFileSlice]:
                         block_type = "block_mid"
                     else:
                         block_type = "block_start"
-                yield RawFileSlice(str_buff, block_type, idx)
+                        if trimmed_content.split()[0] == "for":
+                            block_subtype = "loop"
+                yield RawFileSlice(str_buff, block_type, idx, block_subtype)
                 idx += len(str_buff)
                 str_buff = ""
 
diff --git a/src/sqlfluff/core/templaters/python.py b/src/sqlfluff/core/templaters/python.py
index c354fa0b408..5ed254bcc9a 100644
--- a/src/sqlfluff/core/templaters/python.py
+++ b/src/sqlfluff/core/templaters/python.py
@@ -494,7 +494,7 @@ def _split_invariants(
         idx: Optional[int] = None
         templ_idx = 0
         # Loop through
-        for raw, token_type, raw_pos in raw_sliced:
+        for raw, token_type, raw_pos, _ in raw_sliced:
             if raw in invariants:
                 if buffer:
                     yield IntermediateFileSlice(
diff --git a/test/core/templaters/base_test.py b/test/core/templaters/base_test.py
index 71594a03dd0..1e2ea9c69b4 100644
--- a/test/core/templaters/base_test.py
+++ b/test/core/templaters/base_test.py
@@ -295,7 +295,10 @@ def test__templated_file_templated_slice_to_source_slice(
     file = TemplatedFile(
         source_str="Dummy String",
         sliced_file=file_slices,
-        raw_sliced=raw_slices,
+        raw_sliced=[
+            rs if isinstance(rs, RawFileSlice) else RawFileSlice(*rs)
+            for rs in raw_slices
+        ],
         fname="test",
     )
     source_slice = file.templated_slice_to_source_slice(in_slice)
diff --git a/test/core/templaters/jinja_test.py b/test/core/templaters/jinja_test.py
index 498bf384ed3..220f2c06c1a 100644
--- a/test/core/templaters/jinja_test.py
+++ b/test/core/templaters/jinja_test.py
@@ -185,11 +185,11 @@ def test__templater_jinja_slice_template(test, result):
         assert "".join(elem[0] for elem in resp) == test
         # check indices
         idx = 0
-        for literal, _, pos in resp:
+        for literal, _, pos, _ in resp:
             assert pos == idx
             idx += len(literal)
     # Check total result
-    assert resp == result
+    assert [r[:3] for r in resp] == result
 
 
 @pytest.mark.parametrize(
diff --git a/test/core/templaters/python_test.py b/test/core/templaters/python_test.py
index f9f9d73e69c..1f03f2781a5 100644
--- a/test/core/templaters/python_test.py
+++ b/test/core/templaters/python_test.py
@@ -195,7 +195,7 @@ def test__templater_python_slice_template(test, result):
     assert "".join(elem[0] for elem in resp) == test
     # check indices
     idx = 0
-    for literal, _, pos in resp:
+    for literal, _, pos, _ in resp:
         assert pos == idx
         idx += len(literal)
     # Check total result
diff --git a/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/after.sql b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/after.sql
new file mode 100644
index 00000000000..dc9f489d2ae
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/after.sql
@@ -0,0 +1,7 @@
+SELECT
+    {% for _ in [1, 2, 3] %}      {% endfor %}
+    10;
+
+SELECT
+    1,
+      {% for _ in [1, 2, 3] %}    2{%endfor %}
diff --git a/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/before.sql b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/before.sql
new file mode 100644
index 00000000000..e06e708abec
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/before.sql
@@ -0,0 +1,8 @@
+SELECT
+{% for _ in [1, 2, 3] %} 2,
+{% endfor %}
+    10;
+
+SELECT
+    1,
+  {% for _ in [1, 2, 3] %}    2{%endfor %}
diff --git a/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/test-config.yml b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/test-config.yml
new file mode 100644
index 00000000000..ed3320f47c8
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/016_no_fix_in_template_loops/test-config.yml
@@ -0,0 +1,5 @@
+test-config:
+  rules:
+    - L003
+    - L013
+    - L039
diff --git a/test/rules/std_fix_auto_test.py b/test/rules/std_fix_auto_test.py
index cf364530298..eee164de098 100644
--- a/test/rules/std_fix_auto_test.py
+++ b/test/rules/std_fix_auto_test.py
@@ -113,7 +113,7 @@ def auto_fix_test(dialect, folder, caplog):
 
     # We call the check_tuples here, even to makes sure any non-linting
     # violations are raised, and the test fails.
-    vs = set(res.check_tuples())
+    vs = set(res.check_tuples(raise_on_non_linting_violations=False))
     # If we have a violations structure, let's enforce it.
     if violations:
         # Format the violations file


Code review comments:
diff_hunk: @@ -314,6 +321,21 @@ def fix_string(self) -> Tuple[Any, bool]:
             )
             local_type_list = [slc.slice_type for slc in local_raw_slices]
 
+            if set(local_raw_slices).intersection(raw_slices_in_template_loop):
+                # The patch is being applied to a loop body. Sanity check it.
+                patch_split_list = patch.fixed_raw.split()
+                patch_split_set = set(patch.fixed_raw.split())
+                if len(patch_split_list) - len(patch_split_set) >= 2:
+                    # This is a heuristic. If the patch contains 2 or more
+                    # duplicate strings, assume it's a poorly computed patch
+                    # where the fixes to expanded code couldn't be applied
+                    # cleanly to the source code.
+                    linter_logger.info(
+                        "      - Skipping apparent bad patch inside template loop: %s",
+                        patch,
+                    )
+                    continue
path: src/sqlfluff/core/linter/linted_file.py
text: @user1:
Seems this could be prone to false positives (I.e. stuff that used to fix fine before but now doesn’t). Wonder what we can do to reduce that? Could we add lots of test cases?

@author:
Difficult to say. I think this will only trigger for loops that don't do any templating inside the loop, which, now that I think about it, is a **pretty strange thing to do**. I asked the author of the original issue what he's actually trying to do, because none of his examples generate useful looking code, e.g.:
```
SELECT 2, 2, 2, 2
```

Umm, what?

Ultimately, I think it's hard to solve this 100% robustly without getting deeper into Jinja. As Alan mentioned on the podcast, Jinja doesn't seem to support this. I have a _suspicion_ that if we look really closely at Jinja, we may be able to do some things to make this work, although they may be hacky at first. But if we find something promising, maybe we can submit an upstream PR.

Basically, if we could determine which sections of code came from different iterations of a loop, we could avoid these issues much more reliably and straightforwardly.

@author:
Some other random thoughts:
* Currently, SQLFluff sometimes corrupts the source code. That's a very severe bug. A code fixer should never _break_ the code.
* In the short run, I'm willing to have SQLFluff make some mistakes of omission (not changing code) or even poor formatting in order to avoid corrupting the code.
* In the long run, I expect we'll see user reports where this isn't quite behaving correctly, and we'll address those.

In a perfect world, I'd take a few engineers and a QA person and we'd go off and work on this for a few months in isolation. In practice, we have limited engineering time, and our users provide a lot of the QA. It's not perfect, but SQLFluff _does_ say it's in beta. I was wondering if we may want our next release to be a [pre-release](https://www.python.org/dev/peps/pep-0440/#pre-releases). That will prevent people from accidentally picking up unstable code, while still giving us a chance to get preliminary feedback from the community.

@user1:
Yeah I'm still veering towards the "if Jinja doesn't support this easily, then maybe we should gracefully bail out as say we can't fix for loop" argument I suggested on the other issue, rather than risk breaking things. I didn't 100% follow this code, but is that what this PR is basically attempting to do by checking this? In which cse maybe bailing out (even if it's more often than it used to) is the "right" thing to do.

I just never like the word "heuristics!" - to me it just suggests magic AI 😁

@user1:
> I was wondering if we may want our next release to be a [pre-release](https://www.python.org/dev/peps/pep-0440/#pre-releases). That will prevent people from accidentally picking up unstable code, while still giving us a chance to get preliminary feedback from the community.

Do we get much feedback for pre-releases? If not, then question the value of doing this. I think it makes sense for certain things (e.g. Alan is proposing a pre-release for moving dbt template to a plugin and can have some the maintainers test that), but for others it seems to just put off the release with the hope of gaining more confidence without any real knowledge that it will give us that confidence...

@author:
I had the same thought yesterday afternoon (don't do it!), but we have existing tests that rely on this, and if you look at those tests, they are pretty useful things to support. We have code like this at my job -- picture a "for" loop where each iteration expands to a 50-line SELECT that uses just a little templating -- a column name here, a FROM table there. So I see both sides of this -- the user side (I want!) and the engineer side (it's flaky!).

Basically, this code is looking for what appear to be duplicate strings in the expanded code, and if it sees them, discard the patches in that area.

I used the word heuristic somewhat deliberately. To me, it's a fancy word for "guess'. I leave the magic AI to the data scientists on the team I work with. 🤣 

@user1:
Resolving as you went a different approach in the end.