Repository: sqlfluff/sqlfluff
Pull request: Prevent L019 plus L034 corrupting SQL
### Brief summary of the change made

Fixes #1734

### Are there any other side effects of this change that we should be aware of?

No.

### 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/dialects` (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:
TSQL linting fix: L019+L034 breaks the code
The following code breaks when fixed:

```
    SELECT
      [ID]
    , CASE
        WHEN [ID] = 1 THEN NULL
        ELSE 1
        END AS [CaseOutput]
    , [DataDate]
    FROM #temp1
```

## Expected Behaviour

Workable code.

## Observed Behaviour

```
SELECT
    [ID],
    [DataDate]
    CASE
        WHEN [ID] = 1 THEN NULL
        ELSE 1
    END AS [CaseOutput],
FROM #temp1
```

## Steps to Reproduce

Run fix on the above

## Dialect

TSQL

## Version
Include the output of `sqlfluff --version` along with your Python version

most recent main
Python 3.9.7

## Configuration
```
Include your SQLFluff configuration here
```

Hints from issue discussion:
Note: The output SQL is broken because it's missing the comma after `[DataDate]`.
And there's a comma after CaseOutput, which breaks the SQL for TSQL at least.
I did some preliminary investigation. I think L034 is running after L019's edits have already been applied, so when it attempts to reorder the columns, it's inadvertently moving the updated commas as well. You see see this in the L034 "edit" actions below. If I run L034 by itself (with L019 disabled), commas are not included in the edits.

```
INFO       0 -> 'SELECT\n  [ID]\n, CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput]\n, [DataDate]\nFROM #temp1\n'
DEBUG      [L019] !! Violation Found: 'Found leading comma. Expected only trailing.'
DEBUG      [L019] !! Fix Proposed: <LintFix: delete @[L:  3, P:  1] delete:','>
DEBUG      [L019] !! Fix Proposed: <LintFix: delete @[L:  3, P:  2] delete:' '>
DEBUG      [L019] !! Fix Proposed: <LintFix: edit @[L:  2, P:  3] edt:'[ID]'->'[ID],'>
DEBUG      Developer Note: Edit segment found with preset position marker. These should be unset and calculated later.
DEBUG      Developer Note: Edit segment found with preset position marker. These should be unset and calculated later.
DEBUG      [L019] !! Violation Found: 'Found leading comma. Expected only trailing.'
DEBUG      [L019] !! Fix Proposed: <LintFix: delete @[L:  7, P:  1] delete:','>
DEBUG      [L019] !! Fix Proposed: <LintFix: delete @[L:  7, P:  2] delete:' '>
DEBUG      [L019] !! Fix Proposed: <LintFix: edit @[L:  6, P: 12] edt:'[CaseOutput]'->'[CaseOutput],'>
INFO       Applying Fixes [L019]: [<LintFix: delete @[L:  3, P:  1] delete:','>, <LintFix: delete @[L:  3, P:  2] delete:' '>, <LintFix: edit @[L:  2, P:  3] edt:'[ID]'->'[ID],'>, <LintFix: delete @[L:  7, P:  1] delete:','>, <LintFix: delete @[L:  7, P:  2] delete:' '>, <LintFix: edit @[L:  6, P: 12] edt:'[CaseOutput]'->'[CaseOutput],'>]
DEBUG      Matched fix against segment: <LintFix: delete @[L:  3, P:  1] delete:','> -> <SymbolSegment: ([L:  3, P:  1]) ','>
DEBUG      Matched fix against segment: <LintFix: delete @[L:  3, P:  2] delete:' '> -> <WhitespaceSegment: ([L:  3, P:  2]) ' '>
DEBUG      Matched fix against segment: <LintFix: delete @[L:  7, P:  1] delete:','> -> <SymbolSegment: ([L:  7, P:  1]) ','>
DEBUG      Matched fix against segment: <LintFix: delete @[L:  7, P:  2] delete:' '> -> <WhitespaceSegment: ([L:  7, P:  2]) ' '>
DEBUG      Matched fix against segment: <LintFix: edit @[L:  2, P:  3] edt:'[ID]'->'[ID],'> -> <CodeSegment: ([L:  2, P:  3]) '[ID]'>
DEBUG      Matched fix against segment: <LintFix: edit @[L:  6, P: 12] edt:'[CaseOutput]'->'[CaseOutput],'> -> <CodeSegment: ([L:  6, P: 12]) '[CaseOutput]'>
DEBUG      Developer Note: Edit segment found with preset position marker. These should be unset and calculated later.
DEBUG      Developer Note: Edit segment found with preset position marker. These should be unset and calculated later.
DEBUG      [L034] !! Violation Found: 'Use wildcards then simple targets before calculations and aggregates in select statements.'
DEBUG      [L034] !! Fix Proposed: <LintFix: edit @[L:  3, P:  3] edt:'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'->'[DataDate]'>
DEBUG      [L034] !! Fix Proposed: <LintFix: edit @[L:  7, P:  3] edt:'[DataDate]'->'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'>
INFO       Applying Fixes [L034]: [<LintFix: edit @[L:  3, P:  3] edt:'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'->'[DataDate]'>, <LintFix: edit @[L:  7, P:  3] edt:'[DataDate]'->'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'>]
DEBUG      Matched fix against segment: <LintFix: edit @[L:  3, P:  3] edt:'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'->'[DataDate]'> -> <SelectClauseElementSegment: ([L:  3, P:  3])>
DEBUG      Matched fix against segment: <LintFix: edit @[L:  7, P:  3] edt:'[DataDate]'->'CASE\n    WHEN [ID] = 1 THEN NULL\n    ELSE 1\n    END AS [CaseOutput],'> -> <SelectClauseElementSegment: ([L:  7, P:  3])>
INFO       Fix loop complete. Stability achieved after 1/10 loops.
```

To fix this, we first need to determine where the bug is (and this could be a bit of a philosophical question):
* A bug in L034 (for not handling the new comma placement correctly)
* A bug in L019 (e.g. is it doing something wrong when moving the commas?)
* A bug in the core lint/fix engine (e.g. should it prevent multiple rules editing the same area of code in the same "linting round? If so, the same fix could still be generated and applied in a future round, separate from the round that made the L019 fixes)
I reproduced this bug in the ANSI dialect, so it's not specific to TSQL:
```
SELECT
  ID
, CASE
    WHEN ID = 1 THEN NULL
    ELSE 1
    END AS CaseOutput
, DataDate
FROM temp1
```

becomes

```
SELECT
  ID,
DataDate
CASE
    WHEN ID = 1 THEN NULL
    ELSE 1
    END AS CaseOutput,
FROM temp1
```
It also happens if I replace the `CASE` statement with a `COALESCE()` function call:
```
SELECT
  ID
, COALESCE(a, 1) AS CoalesceOutput
, DataDate
FROM temp1
```

becomes

```
SELECT
  ID,
DataDate
COALESCE(a, 1) AS CoalesceOutput,
FROM temp1
```

Not surprising, but I wanted to verify it happened with a syntactically simpler expression (function call vs `CASE`).
The bug is in L019. That rule appears to be literally editing the raw content of the `SelectClauseElementSegment` to add a comma. Correct behavior would be to insert a separate comma segment (an instance of `SymbolSegment`). Since L019 makes the comma "part of" the column, L034 moves the comma along with the column text. 🤢

For example, here's the ID column as L034 sees it (L019 has already run). Note the comma.
```
(Pdb) select_target_elements[0].segments[0].raw
'ID,'
```

If I run L034 with L019 disabled, the `select_target_elements` don't contain commas:
```
(Pdb) select_target_elements[0].segments[0].raw
'ID'
(Pdb) pp [s.raw for s in select_target_elements]
['ID', 'COALESCE(a, 1) AS CoalesceOutput', 'DataDate']
```

Instead, the commas are "siblings" of the `SelectClauseElementSegment`s, represented as separate `SymbolSegment` objects:
```
(Pdb) pp context.segment.segments
(<KeywordSegment: ([L:  1, P:  1]) 'SELECT'>,
 <Indent: ([L:  1, P:  7]) ''>,
 <NewlineSegment: ([L:  1, P:  7]) '\n'>,
 <WhitespaceSegment: ([L:  2, P:  1]) '  '>,
 <SelectClauseElementSegment: ([L:  2, P:  3])>,
 <NewlineSegment: ([L:  2, P:  5]) '\n'>,
 <SymbolSegment: ([L:  3, P:  1]) ','>,
 <WhitespaceSegment: ([L:  3, P:  2]) ' '>,
 <SelectClauseElementSegment: ([L:  3, P:  3])>,
 <NewlineSegment: ([L:  3, P: 35]) '\n'>,
 <SymbolSegment: ([L:  4, P:  1]) ','>,
 <WhitespaceSegment: ([L:  4, P:  2]) ' '>,
 <SelectClauseElementSegment: ([L:  4, P:  3])>)
```

Correction: L019 is creating a new `SelectClauseElementSegment` with two child segments:
```
(Pdb) !r.raw
'ID,'
(Pdb) !r.segments
(<CodeSegment: ([L:  2, P:  3]) 'ID'>, <SymbolSegment: ([L:  2, P:  3]) ','>)
```

This happens in `BaseSegment.apply_fixes()`. It seems that any "edit" fix replaces the specified segment with a new segment of the same type and the specified segments as **children**. But the comma should not be a child, it should be a sibling. Perhaps using `create` rather than `edit` will work?

This is similar to other subtle bugs we've seen previously: a rule makes a change that looks correct if you simply render the raw segment strings, but they "corrupt" the parse tree, leaving it in a state that is not possible or correct with respect to the dialect. Ideally, SQLFluff would detect this and either flag it as an internal error (rule bug) or find a way to "make it right", i.e. find another way of applying the edit that respects the dialect. See #1304 for more detail. @alanmcruickshank: FYI.

For now, I'll try and fix this by changing L019, as described above. We _may_ end up needing to make this deeper enhancement at some point -- it's not yet clear to me.

Commit message: L019 plus L034 corrupts SQL

Patch to review:
diff --git a/src/sqlfluff/rules/L019.py b/src/sqlfluff/rules/L019.py
index 7f439dc82dd..7580c4559ad 100644
--- a/src/sqlfluff/rules/L019.py
+++ b/src/sqlfluff/rules/L019.py
@@ -1,9 +1,8 @@
 """Implementation of Rule L019."""
 
-from typing import Dict, Any, Optional
-
-from sqlfluff.core.parser import WhitespaceSegment
+from typing import Any, Dict, Optional, Tuple
 
+from sqlfluff.core.parser import RawSegment, WhitespaceSegment
 from sqlfluff.core.rules.base import BaseRule, LintFix, LintResult, RuleContext
 from sqlfluff.core.rules.doc_decorators import (
     document_fix_compatible,
@@ -67,7 +66,7 @@ def _last_comment_seg(raw_stack):
         return None
 
     @staticmethod
-    def _last_code_seg(raw_stack):
+    def _last_code_seg(raw_stack: Tuple[RawSegment, ...]) -> Optional[RawSegment]:
         """Trace the raw stack back to the most recent code segment.
 
         A return value of `None` indicates no code segments preceding the current position.
@@ -77,6 +76,17 @@ def _last_code_seg(raw_stack):
                 return segment
         return None
 
+    @staticmethod
+    def _follows_seg(
+        raw_stack: Tuple[RawSegment, ...], segment: RawSegment
+    ) -> Optional[RawSegment]:
+        """Given a segment in raw_stack, return the segment following."""
+        idx = raw_stack.index(segment)
+        if idx < len(raw_stack):
+            return raw_stack[idx + 1]
+        else:
+            return None
+
     def _eval(self, context: RuleContext) -> Optional[LintResult]:
         """Enforce comma placement.
 
@@ -176,11 +186,11 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]:
                                 for d in memory["whitespace_deletions"]
                             ],
                             LintFix(
-                                "edit",
-                                last_code_seg,
-                                # Reuse the previous leading comma violation to
-                                # create a new trailing comma
-                                [last_code_seg, last_leading_comma_seg],
+                                "create",
+                                anchor=self._follows_seg(
+                                    context.raw_stack, last_code_seg
+                                ),
+                                edit=last_leading_comma_seg,
                             ),
                         ],
                     )
diff --git a/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/after.sql b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/after.sql
new file mode 100644
index 00000000000..1b252c17ac3
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/after.sql
@@ -0,0 +1,5 @@
+SELECT
+  ID,
+DataDate,
+COALESCE(a, 1) AS CoalesceOutput
+FROM temp1
diff --git a/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/before.sql b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/before.sql
new file mode 100644
index 00000000000..f36dd582050
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/before.sql
@@ -0,0 +1,5 @@
+SELECT
+  ID
+, COALESCE(a, 1) AS CoalesceOutput
+, DataDate
+FROM temp1
diff --git a/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/test-config.yml b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/test-config.yml
new file mode 100644
index 00000000000..01ed778dc7e
--- /dev/null
+++ b/test/fixtures/linter/autofix/ansi/022_l019_corrupts_parse_tree_and_causes_l034_to_corrupt_sql/test-config.yml
@@ -0,0 +1,4 @@
+test-config:
+  rules:
+    - L019
+    - L034


Code review comments:
diff_hunk: @@ -176,11 +186,11 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]:
                                 for d in memory["whitespace_deletions"]
                             ],
                             LintFix(
-                                "edit",
-                                last_code_seg,
-                                # Reuse the previous leading comma violation to
-                                # create a new trailing comma
-                                [last_code_seg, last_leading_comma_seg],
+                                "create",
path: src/sqlfluff/rules/L019.py
text: @author:
This is the fix. If we "edit", it corrupts the parse tree by making the newly placed comma segment a _child_ of the code segment. This is wrong. It should be a sibling. We achieve this by using "create" instead. This requires using a different anchor: the segment _following_ the last code segment, not the last code segment itself.

This bug is a bit similar to the #1304 bug from a month or so ago where lint fixes end up placing whitespace segments in weird places they don't belong. Noting this because there's an open question whether the core linter could help detect or avoid such issues. I suggest we continue fixing the individual rules while we can, but this deeper improvement may prove necessary at some point.