Repository: sqlfluff/sqlfluff

Issue:
`sqlfluff fix` keeps adding blank lines
_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._

## Expected Behaviour

After a finite number of iterations (ideally 1 :slightly_smiling_face:), `sqlfluff fix` should eventually converge on an output that it is happy with.

## Observed Behaviour

`sqlfluff fix` keeps adding a blank line at the end of the file (which makes it challenging to use as, e.g., a pre-commit hook).

## Steps to Reproduce

```console
$ printf 'SELECT 100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n' | sqlfluff fix - | sqlfluff fix - | sqlfluff fix - | sqlfluff fix - | sqlfluff fix -
Unfixable violations detected.
Unfixable violations detected.
Unfixable violations detected.
Unfixable violations detected.
SELECT
    100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000




$
```

(the number of blank lines at the end correllates with the number of `sqlfluff fix` passes)

## Dialect

None specified

## Version
SQLFluff ce4e5a344526f7ee61a0950adc079e4d3b5af438, Python 3.9.7

## Configuration

None

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) Report
> Merging [#1427](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) (1392d78) into [main](https://codecov.io/gh/sqlfluff/sqlfluff/commit/ce4e5a344526f7ee61a0950adc079e4d3b5af438?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) (ce4e5a3) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427/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/1427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff)

```diff
@@            Coverage Diff            @@
##              main     #1427   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          127       127           
  Lines         8603      8619   +16     
=========================================
+ Hits          8603      8619   +16     
```


| [Impacted Files](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff) | Coverage Δ | |
|---|---|---|
| [src/sqlfluff/rules/L036.py](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff#diff-c3JjL3NxbGZsdWZmL3J1bGVzL0wwMzYucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427?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/1427?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sqlfluff). Last update [ce4e5a3...1392d78](https://codecov.io/gh/sqlfluff/sqlfluff/pull/1427?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: The conversation about `is_type()` versus checking the `type` field directly got hidden by GitHub. IIUC, the `is_type()` function is a bit smarter and will handle some cases where the stored `type` of a segment does not match its actual type.

There are two implementations of `is_type()`. Here is the more interesting one:
```
    def is_type(self, *seg_type):
        """Extend the parent class method with the surrogate types."""
        if self._surrogate_type and self._surrogate_type in seg_type:
            return True
        return self.class_is_type(*seg_type)
```

Alan would know how this code is used, but you can see how this will forward to another function in some cases, so I think this is always the recommended way to do it.

Patch:
diff --git a/src/sqlfluff/rules/L036.py b/src/sqlfluff/rules/L036.py
index b334ce6bc..274669f4e 100644
--- a/src/sqlfluff/rules/L036.py
+++ b/src/sqlfluff/rules/L036.py
@@ -143,14 +143,17 @@ class Rule_L036(BaseRule):
             # Do we have a modifier?
             modifier = select_clause.get_child("select_clause_modifier")
 
-            # there is a newline between select and select target
+            # Prepare the select clause which will be inserted
+            # In most (but not all) case we'll want to replace the newline with
+            # the statement and a newline, but in some cases however (see #1424)
+            # we don't need the final newline.
+            copy_with_newline = True
             insert_buff = [
                 WhitespaceSegment(),
                 select_clause.segments[select_targets_info.first_select_target_idx],
-                NewlineSegment(),
             ]
 
-            # Also move any modifiers if present
+            # Check if the modifier is one we care about
             if modifier:
                 # If it's already on the first line, ignore it.
                 if (
@@ -158,26 +161,38 @@ class Rule_L036(BaseRule):
                     < select_targets_info.first_new_line_idx
                 ):
                     modifier = None
-                # Otherwise we need to move it too
-                else:
-                    insert_buff = [WhitespaceSegment(), modifier] + insert_buff
-
             fixes = [
-                # Replace "newline" with <<select_target>>, "newline".
-                LintFix(
-                    "edit",
-                    select_clause.segments[select_targets_info.first_new_line_idx],
-                    insert_buff,
-                ),
                 # Delete the first select target from its original location.
+                # We'll add it to the right section at the end, once we know
+                # what to add.
                 LintFix(
                     "delete",
                     select_clause.segments[select_targets_info.first_select_target_idx],
                 ),
             ]
 
-            # Also delete the original modifier if present
+            start_idx = 0
+
+            # If we have a modifier to move:
             if modifier:
+
+                # Add it to the insert
+                insert_buff = [WhitespaceSegment(), modifier] + insert_buff
+
+                modifier_idx = select_clause.segments.index(modifier)
+                # Delete the whitespace after it (which is two after, thanks to indent)
+                if (
+                    len(select_clause.segments) > modifier_idx + 1
+                    and select_clause.segments[modifier_idx + 2].is_whitespace
+                ):
+                    fixes += [
+                        LintFix(
+                            "delete",
+                            select_clause.segments[modifier_idx + 2],
+                        ),
+                    ]
+
+                # Delete the modifier itself
                 fixes += [
                     LintFix(
                         "delete",
@@ -185,25 +200,14 @@ class Rule_L036(BaseRule):
                     ),
                 ]
 
-            if (
-                select_targets_info.first_select_target_idx
-                - select_targets_info.first_new_line_idx
-                == 2
-                and select_clause.segments[
-                    select_targets_info.first_new_line_idx + 1
-                ].is_whitespace
-            ):
-                # If the select target is preceded by a single whitespace
-                # segment, delete that as well. This addresses the bug fix
-                # tested in L036.yml's "test_cte" scenario.
-                fixes.append(
-                    LintFix(
-                        "delete",
-                        select_clause.segments[
-                            select_targets_info.first_new_line_idx + 1
-                        ],
-                    ),
-                )
+                # Set the position marker for removing the preceeding
+                # whitespace and newline, which we'll use below.
+                start_idx = modifier_idx
+            else:
+                # Set the position marker for removing the preceeding
+                # whitespace and newline, which we'll use below.
+                start_idx = select_targets_info.first_select_target_idx
+
             if parent_stack and parent_stack[-1].is_type("select_statement"):
                 select_stmt = parent_stack[-1]
                 select_clause_idx = select_stmt.segments.index(select_clause)
@@ -213,11 +217,106 @@ class Rule_L036(BaseRule):
                         # The select_clause is immediately followed by a
                         # newline. Delete the newline in order to avoid leaving
                         # behind an empty line after fix.
-                        fixes.append(
-                            LintFix(
-                                "delete", select_stmt.segments[after_select_clause_idx]
+                        delete_last_newline = True
+
+                        # Since, we're deleting the newline, we should also delete all
+                        # whitespace before it or it will add random whitespace to
+                        # following statements. So walk back through the segment
+                        # deleting whitespace until you get the previous newline, or
+                        # something else.
+                        idx = 1
+                        while start_idx - idx < len(select_clause.segments):
+                            # Delete any whitespace
+                            if select_clause.segments[start_idx - idx].is_type(
+                                "whitespace"
+                            ):
+                                fixes += [
+                                    LintFix(
+                                        "delete",
+                                        select_clause.segments[start_idx - idx],
+                                    ),
+                                ]
+
+                            # Once we see a newline, then we're done,
+                            if select_clause.segments[start_idx - idx].is_type(
+                                "newline",
+                            ):
+                                break
+
+                            # Once we see anything other than whitespace,
+                            # then we're done, but in this case we want to
+                            # keep the final newline.
+                            if not select_clause.segments[start_idx - idx].is_type(
+                                "whitespace", "newline"
+                            ):
+                                delete_last_newline = False
+                                break
+
+                            idx += 1
+
+                        # Finally delete the newline, unless we've decided not to
+                        if delete_last_newline:
+                            fixes.append(
+                                LintFix(
+                                    "delete",
+                                    select_stmt.segments[after_select_clause_idx],
+                                )
                             )
-                        )
+
+                    elif select_stmt.segments[after_select_clause_idx].is_type(
+                        "whitespace"
+                    ):
+                        # The select_clause has stuff after (most likely a comment)
+                        # Delete the whitespace immeadiately after the select clause
+                        # so the other stuff aligns nicely based on where the select
+                        # clause started
+                        fixes += [
+                            LintFix(
+                                "delete",
+                                select_stmt.segments[after_select_clause_idx],
+                            ),
+                        ]
+                    elif select_stmt.segments[after_select_clause_idx].is_type(
+                        "dedent"
+                    ):
+                        # The end of the select statement, so this is the one
+                        # case we don't want the newline added to end of
+                        # select_clause (see #1424)
+                        copy_with_newline = False
+
+                        # Again let's strip back the whitespace, bnut simpler
+                        # as don't need to worry about new line so just break
+                        # if see non-whitespace
+                        idx = 1
+                        start_idx = select_clause_idx - 1
+                        while start_idx - idx < len(select_clause.segments):
+                            # Delete any whitespace
+                            if select_clause.segments[start_idx - idx].is_type(
+                                "whitespace"
+                            ):
+                                fixes += [
+                                    LintFix(
+                                        "delete",
+                                        select_clause.segments[start_idx - idx],
+                                    ),
+                                ]
+                            else:
+                                break
+                            idx += 1
+
+            if copy_with_newline:
+                insert_buff = insert_buff + [NewlineSegment()]
+
+            fixes += [
+                # Insert the select_clause in place of the first newlin in the
+                # Select statement
+                LintFix(
+                    "edit",
+                    select_clause.segments[select_targets_info.first_new_line_idx],
+                    insert_buff,
+                ),
+            ]
+
             return LintResult(
                 anchor=select_clause,
                 fixes=fixes,


Test patch:
diff --git a/test/fixtures/rules/std_rule_cases/L036.yml b/test/fixtures/rules/std_rule_cases/L036.yml
index 831269802..928b717e9 100644
--- a/test/fixtures/rules/std_rule_cases/L036.yml
+++ b/test/fixtures/rules/std_rule_cases/L036.yml
@@ -48,12 +48,10 @@ test_comment_between_select_and_single_select_target:
         user_id
     FROM
         safe_user
-  # In a normal "sqlfluff fix" run, the weird "FROM" indenting left over after the fix
-  # would be fixed by L003.
   fix_str: |
     SELECT user_id
         -- This is the user's ID.
-        FROM
+    FROM
         safe_user
 
 test_multiple_select_targets_some_newlines_missing_1:
@@ -153,7 +151,60 @@ test_distinct_single_fail_b:
     SELECT
       distinct a
     FROM my_table
-  # NOTE: This doesn't yet reflow nicely. We should solve that elsewhere.
   fix_str: |
     SELECT distinct a
-       FROM my_table
+    FROM my_table
+
+test_single_select_with_no_from:
+  fail_str: "SELECT\n   10000000\n"
+  fix_str: "SELECT 10000000\n"
+
+test_single_select_with_comment_after_column:
+  fail_str: |
+    SELECT
+      1 -- this is a comment
+    FROM
+      my_table
+  fix_str: |
+    SELECT 1
+      -- this is a comment
+    FROM
+      my_table
+
+test_single_select_with_comment_after_column_no_space:
+  fail_str: |
+    SELECT
+      1-- this is a comment
+    FROM
+      my_table
+  fix_str: |
+    SELECT 1
+      -- this is a comment
+    FROM
+      my_table
+
+test_single_select_with_multiple_mixed_comments:
+  fail_str: |
+    SELECT
+      -- previous comment
+      1 -- this is a comment
+    FROM
+      my_table
+  fix_str: |
+    SELECT 1
+      -- previous comment
+      -- this is a comment
+    FROM
+      my_table
+
+test_single_select_with_comment before:
+  fail_str: |
+    SELECT
+      /* comment before */ 1
+    FROM
+      my_table
+  fix_str: |
+    SELECT 1
+      /* comment before */
+    FROM
+      my_table
diff --git a/test/rules/std_L016_L36_combo.py b/test/rules/std_L016_L36_combo.py
new file mode 100644
index 000000000..e69ea81f3
--- /dev/null
+++ b/test/rules/std_L016_L36_combo.py
@@ -0,0 +1,31 @@
+"""Tests the combination of L016 (no long lines) and L036 (single selects should eb on SELECT line)."""
+
+import sqlfluff
+
+
+def test__rules__std_L016_L036_long_line_lint():
+    """Verify that a long line that causes a clash between L016 and L036 is not changed."""
+    sql = "SELECT\n1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n"
+    result = sqlfluff.lint(sql)
+    assert "L016" in [r["code"] for r in result]
+    assert "L036" in [r["code"] for r in result]
+
+
+def test__rules__std_L016_L036_long_line_fix():
+    """Verify that a long line that causes a clash between L016 and L036 does not add multiple newlines (see #1424)."""
+    sql = "SELECT 1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n"
+    result = sqlfluff.fix(sql)
+    assert (
+        result
+        == "SELECT\n    1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n"
+    )
+
+
+def test__rules__std_L016_L036_long_line_fix2():
+    """Verify that a long line that causes a clash between L016 and L036 does not add multiple newlines (see #1424)."""
+    sql = "SELECT\n    1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n"
+    result = sqlfluff.fix(sql)
+    assert (
+        result
+        == "SELECT 1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\n"
+    )