sqlfluff: Improve how "sqlfluff fix" handles parse errors

Currently, sqlfluff fix applies fixes even if there are parse errors. It merely prints a warning and exits with status code 1.

We should make it “harder” to apply fixes when there are parse errors, because this can easily cause worse problems (i.e. introducing new errors into the SQL code, because rules often don’t handle bad code very well, and this is likely to remain the case).

Some ideas:

If not in “force” mode, mention there were parse errors when asking whether to apply fixes.
Should we provide a way to let the user apply fixes to the files that parsed successfully, skipping only the files that had parse errors?
Should we consider changing “force” mode to a “counter” option, so -f would fail on parse errors without applying fixes, but -ff would go ahead and apply fixes anyway?

Note: This issue is a related follow-on to #2345.

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 21 (21 by maintainers)

Most upvoted comments

Test cases:

1

SELCT my_col
FROM my_schema.my_table

Run: sqlfluff fix test.sql Fixes? No. Exit code: 1

2

SELCT my_col -- noqa: PRS
FROM my_schema.my_table

Run: sqlfluff fix test.sql Fixes? No. Exit code: 0

3

SELCT my_col -- noqa: PRS
FROM my_schema.my_table

Run: sqlfluff fix --FIX-EVEN-UNPARSABLE test.sql Fixes? Yes. Exit code: 0

barrywhart on Feb 2, 2022

Based on the conversation this afternoon I think the behaviour we settled on was:

sqlfluff lint:

Same behaviour as now.

sqlfluff fix:

Only fix if no unparsable segments found in the file. If any are found, no fixes happen even if parsing errors are supressed using noqa or --ignore cli options. The only way to override this is with the --FIX-EVEN-UNPARSABLE option as above.
If there are still parse errors after applying “noqa” directives, exit 1. Otherwise, allow exit 0.

alanmcruickshank on Feb 1, 2022

I’m planning to work on this issue.

I think I’ve found a reasonable middle ground that we can move forward with, hopefully with reasonable safety and flexibility, and without any major objections. 🙏🏽

Notes:

Since we initially discussed this, #2509 enhances noqa to support ignoring template (TMP) and parse (PRS) errors. In the context of this issue, we can use that capability to let users “acknowledge” parse errors, thus allowing SQLFluff to report success and not block users from committing (i.e. if they are using SQLFluff with pre-commit).

Proposed behavior:

sqlfluff lint
- If there’s a parse error, print a warning message. Default to “don’t lint”, even if the parse error is ignored with “noqa”. The message should be clear that SQLFluff won’t lint even if they ignore parse errors. The only way to lint in the presence of parse errors is to use the new command-line option.
- If there are still parse errors after applying “noqa” directives, exit 1 (failure). Otherwise, allow exit 0 (success).
- Add a new lint option, --LINT-EVEN-UNPARSABLE: If specified, lints even on parse error. It’s in ALL CAPS to try and make it clear it’s a bit “out there”. Prints a warning message (in red?) to tell users this option may result in undefined behavior, and that we likely won’t fix any issues encountered when using it.
sqlfluff fix
- If there’s a parse error, print a warning message. Default to “don’t lint/fix”, even if the parse error is ignored with “noqa”. The message should be clear that SQLFluff won’t lint/fix even if they ignore parse errors. The only way to lint/fix in the presence of parse errors is to use the new command-line option.
- If there are still parse errors after applying “noqa” directives, exit 1. Otherwise, allow exit 0.
- Add a new fix option, --FIX-EVEN-UNPARSABLE: If specified, lints/fixes even on parse error. It’s in ALL CAPS to try and make it clear it’s a bit “out there”. Prints a warning message (in red?) to tell users this option may result in undefined behavior, and that we likely won’t fix any issues encountered when using it.

barrywhart on Jan 31, 2022