Repository: sqlfluff/sqlfluff
Pull request: L052: Redefine semi-colon newline to multiline newline
<!--Firstly, thanks for adding this feature! Secondly, please check the key steps against the checklist below to make your contribution easy to merge.-->

<!--Please give the Pull Request a meaningful title (including the dialect this PR is for if it is dialect specific), as this will automatically be added to the release notes, and then the Change Log.-->

### Brief summary of the change made
<!--If there is an open issue for this, then please include `fixes #XXXX` or `closes #XXXX` replacing `XXXX` with the issue number and it will automatically close the issue when the pull request is merged. Alternatively if not fully closed you can say `makes progress on #XXXX` to create a link on that issue without closing it.-->
Fixes #2013. As discussed in the linked issue I have redefined the `semicolon_newline` option on L052 to `multiline_newline` such that, with this option enabled, if the statement is one-line then snap semi-colon to end of statement, else snap to newline after multiline statement.

Think this is more reflective of how the standard user will use this rule and have added several more unit test to validate the new behaviour.

### Are there any other side effects of this change that we should be aware of?
No, although people will need to rename `semicolon_newline` to `multiline_newline` in their config.

### 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:
L052 - Add option for newline semicolon just for multiline queries
We use newline'd semicolons, but only for multiline queries. A config option for this would be great.

Eg.
```
SELECT foo FROM bar;
```

But also:
```
SELECT foo
FROM bar
;
```

Hints from issue discussion:
Where would you propose adding the semi-colon for something like

```sql
with cte as (
    select a from b
)

select * from cte
```
Under it. It'd just be useful for the multiline gang if there was a config option for snapping inline to single line queries only
makes sense, just checking 😄 Do you know if we have a method of reliably determining the which lines belong to the current query? My first thought would be find the parent StatementSegment and check if it spans multiple lines, but not sure if that also applies to tsql?
@WittierDinosaur got this working well now 👍 On my point above, looking at the grammar I think we should be fine to use the parent StatementSegment for all dialects including tsql. tsql basically just optionally wraps StatementSegment in begin/end blocks but that doesn't impact this logic.

In terms of config I propose we change the current `semicolon_newline` option to `multiline_newline` and it works as we described here. I think most people will want the semi_colon on the same line for one line segments so it makes sense to redefine the option to be "Should semi-colons be placed on a new line after the multi-line statements?" 😄

Commit message: Merge branch 'main' into L052_multiline_newline

Patch to review:
diff --git a/src/sqlfluff/core/default_config.cfg b/src/sqlfluff/core/default_config.cfg
index 1a4b2dbf985..197b1610059 100644
--- a/src/sqlfluff/core/default_config.cfg
+++ b/src/sqlfluff/core/default_config.cfg
@@ -91,7 +91,7 @@ prefer_count_1 = False
 prefer_count_0 = False
 
 [sqlfluff:rules:L052]  # Semi-colon formatting approach.
-semicolon_newline = False
+multiline_newline = False
 require_final_semicolon = False
 
 [sqlfluff:rules:L054]  # GROUP BY/ORDER BY column references.
diff --git a/src/sqlfluff/core/rules/config_info.py b/src/sqlfluff/core/rules/config_info.py
index 553dce55ede..3adefd91f9d 100644
--- a/src/sqlfluff/core/rules/config_info.py
+++ b/src/sqlfluff/core/rules/config_info.py
@@ -103,10 +103,10 @@
             "Should alias have an explict AS or is implicit aliasing required?"
         ),
     },
-    "semicolon_newline": {
+    "multiline_newline": {
         "validation": [True, False],
         "definition": (
-            "Should semi-colons be placed on a new line after the statement end?"
+            "Should semi-colons be placed on a new line after multi-line statements?"
         ),
     },
     "require_final_semicolon": {
diff --git a/src/sqlfluff/rules/L052.py b/src/sqlfluff/rules/L052.py
index f69c485b4d8..a59280b0dad 100644
--- a/src/sqlfluff/rules/L052.py
+++ b/src/sqlfluff/rules/L052.py
@@ -44,7 +44,7 @@ class Rule_L052(BaseRule):
         FROM foo;
     """
 
-    config_keywords = ["semicolon_newline", "require_final_semicolon"]
+    config_keywords = ["multiline_newline", "require_final_semicolon"]
 
     @staticmethod
     def _handle_preceding_inline_comments(pre_semicolon_segments, anchor_segment):
@@ -93,10 +93,32 @@ def _handle_trailing_inline_comments(context, anchor_segment):
 
         return anchor_segment
 
+    @staticmethod
+    def _is_one_line_statement(context, segment):
+        """Check if the statement containing the provided segment is one line."""
+        # Find statement segment containing the current segment.
+        statement_segment = next(
+            (
+                s
+                for s in context.parent_stack[0].path_to(segment)
+                if s.is_type("statement")
+            ),
+            None,
+        )
+        if statement_segment is None:  # pragma: no cover
+            # If we can't find a parent statement segment then don't try anything special.
+            return False
+
+        if not list(statement_segment.recursive_crawl("newline")):
+            # Statement segment has no newlines therefore starts and ends on the same line.
+            return True
+
+        return False
+
     def _eval(self, context: RuleContext) -> Optional[LintResult]:
         """Statements must end with a semi-colon."""
         # Config type hints
-        self.semicolon_newline: bool
+        self.multiline_newline: bool
         self.require_final_semicolon: bool
 
         # First we can simply handle the case of existing semi-colon alignment.
@@ -105,9 +127,11 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]:
             # Locate semicolon and iterate back over the raw stack
             # to find the end of the preceding statement.
             anchor_segment = context.segment
+            is_one_line = False
             pre_semicolon_segments = []
             for segment in context.raw_stack[::-1]:
                 if segment.is_code:
+                    is_one_line = self._is_one_line_statement(context, segment)
                     break
                 elif not segment.is_meta:
                     pre_semicolon_segments.append(segment)
@@ -122,9 +146,10 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]:
                     break
                 whitespace_deletions.append(segment)
 
-            # Semi-colon on same line.
-            if not self.semicolon_newline:
+            semicolon_newline = self.multiline_newline if not is_one_line else False
 
+            # Semi-colon on same line.
+            if not semicolon_newline:
                 if len(pre_semicolon_segments) >= 1:
                     # If preceding segments are found then delete the old
                     # semi-colon and its preceding whitespace and then insert
@@ -208,21 +233,25 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]:
             # if the final semi-colon is already present.
             anchor_segment = context.segment
             semi_colon_exist_flag = False
+            is_one_line = False
             pre_semicolon_segments = []
             for segment in complete_stack[::-1]:  # type: ignore
                 if segment.name == "semicolon":
                     semi_colon_exist_flag = True
                 elif segment.is_code:
+                    is_one_line = self._is_one_line_statement(context, segment)
                     break
                 elif not segment.is_meta:
                     pre_semicolon_segments.append(segment)
                 anchor_segment = segment
 
+            semicolon_newline = self.multiline_newline if not is_one_line else False
+
             if not semi_colon_exist_flag:
                 # Create the final semi-colon if it does not yet exist.
 
                 # Semi-colon on same line.
-                if not self.semicolon_newline:
+                if not semicolon_newline:
                     fixes = [
                         LintFix(
                             "edit",
diff --git a/test/fixtures/rules/std_rule_cases/L052.yml b/test/fixtures/rules/std_rule_cases/L052.yml
index b3213564e2b..d2f0f8af746 100644
--- a/test/fixtures/rules/std_rule_cases/L052.yml
+++ b/test/fixtures/rules/std_rule_cases/L052.yml
@@ -4,21 +4,39 @@ test_pass_semi_colon_same_line_default:
   pass_str: |
     SELECT a FROM foo;
 
+test_pass_semi_colon_custom_newline:
+  pass_str: |
+    SELECT a FROM foo;
+  configs:
+    rules:
+      L052:
+        multiline_newline: True
+
 test_fail_semi_colon_same_line_custom_newline:
   fail_str: |
-    SELECT a FROM foo;
+    SELECT a
+    FROM foo;
   fix_str: |
-    SELECT a FROM foo
+    SELECT a
+    FROM foo
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_pass_no_semi_colon_default:
   pass_str: |
     SELECT a FROM foo
 
+test_pass_no_semi_colon_custom_newline:
+  pass_str: |
+    SELECT a FROM foo
+  configs:
+    rules:
+      L052:
+        multiline_newline: True
+
 test_fail_no_semi_colon_custom_require:
   fail_str: |
     SELECT a FROM foo
@@ -29,36 +47,58 @@ test_fail_no_semi_colon_custom_require:
       L052:
         require_final_semicolon: True
 
-test_fail_no_semi_colon_custom_require_newline:
+test_fail_no_semi_colon_custom_require_oneline:
   fail_str: |
     SELECT a FROM foo
   fix_str: |
-    SELECT a FROM foo
+    SELECT a FROM foo;
+  configs:
+    rules:
+      L052:
+        require_final_semicolon: True
+        multiline_newline: True
+
+test_fail_no_semi_colon_custom_require_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo
+  fix_str: |
+    SELECT a
+    FROM foo
     ;
   configs:
     rules:
       L052:
         require_final_semicolon: True
-        semicolon_newline: True
+        multiline_newline: True
 
 test_pass_multi_statement_semi_colon_default:
   pass_str: |
     SELECT a FROM foo;
     SELECT b FROM bar;
 
-test_fail_multi_statement_semi_colon_custom_newline:
-  fail_str: |
+test_pass_multi_statement_semi_colon_custom_oneline:
+  pass_str: |
     SELECT a FROM foo;
     SELECT b FROM bar;
+
+test_fail_multi_statement_semi_colon_custom_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo;
+    SELECT b
+    FROM bar;
   fix_str: |
-    SELECT a FROM foo
+    SELECT a
+    FROM foo
     ;
-    SELECT b FROM bar
+    SELECT b
+    FROM bar
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_pass_multi_statement_no_trailing_semi_colon_default:
   pass_str: |
@@ -77,18 +117,37 @@ test_pass_multi_statement_no_trailing_semi_colon_custom_require:
       L052:
         require_final_semicolon: True
 
-test_fail_multi_statement_no_trailing_semi_colon_custom_require_newline:
+test_pass_multi_statement_no_trailing_semi_colon_custom_require_oneline:
   fail_str: |
     SELECT a FROM foo;
     SELECT b FROM bar
   fix_str: |
-    SELECT a FROM foo
+    SELECT a FROM foo;
+    SELECT b FROM bar;
+  configs:
+    rules:
+      L052:
+        require_final_semicolon: True
+        multiline_newline: True
+
+test_fail_multi_statement_no_trailing_semi_colon_custom_require_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo;
+    SELECT b
+    FROM bar
+  fix_str: |
+    SELECT a
+    FROM foo
+    ;
+    SELECT b
+    FROM bar
     ;
-    SELECT b FROM bar
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        require_final_semicolon: True
+        multiline_newline: True
 
 test_fail_space_semi_colon_default:
   fail_str: |
@@ -105,12 +164,13 @@ test_fail_newline_semi_colon_default:
 
 test_pass_newline_semi_colon_custom_newline:
   pass_str: |
-    SELECT a FROM foo
+    SELECT a
+    FROM foo
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_multi_statement_semi_colon_default:
   fail_str: |
@@ -122,6 +182,27 @@ test_fail_multi_statement_semi_colon_default:
     SELECT a FROM foo;
     SELECT b FROM bar;
 
+test_fail_multi_statement_semi_colon_custom_require_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo
+    
+    ;
+    SELECT b
+    FROM bar    ;
+  fix_str: |
+    SELECT a
+    FROM foo
+    ;
+    SELECT b
+    FROM bar
+    ;
+  configs:
+    rules:
+      L052:
+        require_final_semicolon: True
+        multiline_newline: True
+
 test_fail_multiple_newlines_semi_colon_custom_require_newline:
   fail_str: |
     SELECT a
@@ -136,7 +217,7 @@ test_fail_multiple_newlines_semi_colon_custom_require_newline:
     rules:
       L052:
         require_final_semicolon: True
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_final_semi_colon_same_line_inline_comment:
   fail_str: |
@@ -148,17 +229,30 @@ test_fail_final_semi_colon_same_line_inline_comment:
       L052:
         require_final_semicolon: True
 
-test_fail_final_semi_colon_newline_inline_comment:
+test_fail_final_semi_colon_same_line_inline_comment_custom_oneline:
   fail_str: |
     SELECT a FROM foo -- inline comment
   fix_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a FROM foo; -- inline comment
+  configs:
+    rules:
+      L052:
+        require_final_semicolon: True
+        multiline_newline: True
+
+test_fail_final_semi_colon_newline_inline_comment_custom_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo -- inline comment
+  fix_str: |
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
         require_final_semicolon: True
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_same_line_inline_comment:
   fail_str: |
@@ -180,27 +274,30 @@ test_fail_same_line_multiple_inline_comment:
 
 test_pass_newline_inline_comment:
   pass_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_inline_comment:
   fail_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     
     ;
   fix_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
-test_fail_newline_multiple_inline_comments:
+test_fail_newline_multiple_inline_comments_custom_oneline:
   fail_str: |
     SELECT a FROM foo -- inline comment
     
@@ -208,27 +305,62 @@ test_fail_newline_multiple_inline_comments:
 
     ;
   fix_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a FROM foo; -- inline comment
+
+    -- inline comment
+  configs:
+    rules:
+      L052:
+        multiline_newline: True
+
+test_fail_newline_multiple_inline_comments_custom_multiline:
+  fail_str: |
+    SELECT a
+    FROM foo -- inline comment
+    
+    -- inline comment
+
+    ;
+  fix_str: |
+    SELECT a
+    FROM foo -- inline comment
     ;
 
     -- inline comment
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_trailing_inline_comment:
   fail_str: |
-    SELECT a FROM foo ; -- inline comment
+    SELECT a
+    FROM foo ; -- inline comment
   fix_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
+
+test_fail_newline_preceding_block_comment_custom_oneline:
+  fail_str: |
+    SELECT foo FROM bar /* multiline
+    comment
+    */
+    ;
+  fix_str: |
+    SELECT foo FROM bar; /* multiline
+    comment
+    */
+  configs:
+    rules:
+      L052:
+        multiline_newline: True
 
-test_fail_newline_preceding_block_comment:
+test_fail_newline_preceding_block_comment_custom_multiline:
   fail_str: |
     SELECT foo
     FROM bar /* multiline
@@ -244,7 +376,7 @@ test_fail_newline_preceding_block_comment:
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_trailing_block_comment:
   fail_str: |
@@ -261,7 +393,7 @@ test_fail_newline_trailing_block_comment:
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_block_comment_semi_colon_before:
   fail_str: |
@@ -280,7 +412,7 @@ test_fail_newline_block_comment_semi_colon_before:
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_block_comment_semi_colon_after:
   fail_str: |
@@ -300,4 +432,4 @@ test_fail_newline_block_comment_semi_colon_after:
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True


Code review comments:
diff_hunk: @@ -77,18 +117,37 @@ test_pass_multi_statement_no_trailing_semi_colon_custom_require:
       L052:
         require_final_semicolon: True
 
-test_fail_multi_statement_no_trailing_semi_colon_custom_require_newline:
+test_pass_multi_statement_no_trailing_semi_colon_custom_require_oneline:
path: test/fixtures/rules/std_rule_cases/L052.yml
text: @user1:
What's happening here? The test name says "pass", but it provides a "fail_str" and a "fix_str"? FWIW, the behavior looks correct; only the test _name_ seems wrong.

@author:
ah sorry about that, easy to one of these when making all the various permutations 😄

diff_hunk: @@ -93,10 +93,32 @@ def _handle_trailing_inline_comments(context, anchor_segment):
 
         return anchor_segment
 
+    @staticmethod
+    def _is_one_line_statement(context, segment):
+        """Check if the statement containing the provided segment is one line."""
+        # Find statement segment containing the current segment.
+        statement_segment = next(
+            (
+                s
+                for s in context.parent_stack[0].path_to(segment)
+                if s.is_type("statement")
+            ),
+            None,
+        )
+        if statement_segment is None:  # pragma: no cover
+            # If we can't find a parent statement segment then don't try anything special.
+            return False
+
+        if not list(statement_segment.recursive_crawl("newline")):
path: src/sqlfluff/rules/L052.py
text: @user1:
Performance nitpick:  Since `recursive_crawl()` is a generator, I think you could change this from `list()` to `any()`, then it would stop early if it finds a newline.

diff_hunk: @@ -180,55 +274,93 @@ test_fail_same_line_multiple_inline_comment:
 
 test_pass_newline_inline_comment:
   pass_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
 test_fail_newline_inline_comment:
   fail_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     
     ;
   fix_str: |
-    SELECT a FROM foo -- inline comment
+    SELECT a
+    FROM foo -- inline comment
     ;
   configs:
     rules:
       L052:
-        semicolon_newline: True
+        multiline_newline: True
 
-test_fail_newline_multiple_inline_comments:
+test_fail_newline_multiple_inline_comments_custom_oneline:
   fail_str: |
     SELECT a FROM foo -- inline comment
path: test/fixtures/rules/std_rule_cases/L052.yml
text: @user1:
Nitpick: The two identical comments initially confused me. What would you think about changing them to something like:
```
-- inline comment #1
-- inline comment #2
```

@author:
This is a good point, better to explicitly show that there's no crazy switching round of comment positions going on 👍