Bug fix: L003 should fix indentation for templated code

[barrywhart]

Brief summary of the change made

Are there any other side effects of this change that we should be aware of?

Pull Request checklist

• 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 tox -e generate-fixture-yml).
  • 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.

[codecov]

Codecov Report

Merging #2580 (faadd85) into main (d68a38f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #2580   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          163       163           
  Lines        11861     11862    +1     
=========================================
+ Hits         11861     11862    +1     

Impacted Files	Coverage Δ
src/sqlfluff/rules/L003.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d68a38f...faadd85. Read the comment docs.

[tunetheweb]

I presume this will not cause the whitespace inside templated sections to be linted and potentially corrected too, and only applies to the template code `{{...}}`` itself?

Wonder if there's any more rules we should consider turning this on for? L001, L002...etc.

[barrywhart]

I think so. Basically, this setting doesn't change what L003 does, which is: looking at the final, post-template code, it simply tells the core linter "don't throw away fixes from this rule even though they touch templated code".

See this code: https://github.com/sqlfluff/sqlfluff/blob/main/src/sqlfluff/core/linter/linter.py#L317L325

It seems a little odd to me that we need to set this, since we're not really modifying "templated" code, we're just adding space before templated code.

Let's hold off merging this PR for now -- I will take a deeper look to see if I can find a way to make the core linter smarter. I did some similar work 1-2 months ago.

[barrywhart]

@tunetheweb: I took a closer look at this, and I feel comfortable merging it now.

Here's what I learned:
AFAICT, there are no rules that change template code (stuff inside {{ }}). You might think that L046 does this, because it checks for spaces in templated code, but it only reports issues, it doesn't fix them.

The linter has two different checks regarding templated code:

• LintFix.has_template_conflicts() checks whether the fix is trying to change templated code. Such fixes are automatically rejected, regardless of the targets_templated setting.
• targets_templated checks whether the "anchor" of a lint error (not a fix) is templated code. If so, and if ignore_templated_areas is True, the lint error is discarded.

Because of the first check, it's impossible for a rule to do anything "unsafe". The second check is more of a user preference -- do I want to see lint errors in templated code. The default is to ignore them.

Thus, setting targets_templated allows a rule to return fixes where the anchor is templated code. This is a bit deceptive. For example, in the new L003 test cases, the anchor is templated code (the column that is not indented enough), but the fix is not (it's merely adjacent to the templated code, thus safe).

[tunetheweb]

Here's something interesting @barrywhart :

I wanted to dig into this a little more, and particularly my point about this:

Wonder if there's any more rules we should consider turning this on for? L001, L002...etc.

So I created this SQL in main:

with
first_join as (
    select
        {{ "c1" }}, 
        c2
    from helper
{{ "group by 1" }}
)

select * from first_join

Note there is a whitespace after the first column (represented by • here): {{ "c1" }}•

Linting it finds that error (but not the L003 error as this PR hasn't been merged yet):

% sqlfluff lint test.sql  
== [test.sql] FAIL                                                                                                                                                                           
L:   4 | P:  12 | L001 | Unnecessary trailing whitespace.
All Finished 📜 🎉!

But fixing it fixed both!

with
first_join as (
    select
        {{ "c1" }},
        c2
    from helper
    {{ "group by 1" }}
)

select * from first_join

This leads to two questions:

1. Why did L001 report the extra space after the templating, but L003 didn't report the error before templating? I guess cause it's fixing a non-templated space so doesn't fall afoul of the issue you described above. So it's only ADDing things to a templated section we need to fix. That's good as looks like only L003 needs to be in scope for this fix after all as it appears to be the only one that potentially adds stuff to the beginning of a line.

2. How could fix fix the spacing issue (on main branch), when it was fixing other issues, even when lint didn't find it? And if there is no other issue to fix, then it ignores that. Weird!

Anyway I'm happy to merge this, but thought I'd point that bit of weirdness out.

[barrywhart]

If a role doesn't specify "targets templated", the linter looks at the "anchor"to decide whether to keep it. Above, L001 says the issue is at column 12. Is 12 the space after? Because that's not templated. OTOH, with L003, the anchor is on the column itself, which is templated. This is an example of how arbitrary this check is. The more important checks are on the fixes, not the anchor, because those represent the actual check changes proposed.