[ruff] Fix --add-noqa breaking shebangs#23577
Conversation
Fix astral-sh#14442. Adds a check on noqa application: If the first line is a shebang, errors with range 0..0 can be suppressed by a noqa on the second line.
|
amyreese
left a comment
There was a problem hiding this comment.
The ecosystem report show a large number of panics. Please take a look and resolve that.
|
This might just need a rebase onto latest main to clear up the panics. |
|
I think that's just a stale base branch from my earlier panic issue. I'll close and reopen to retrigger CI! |
amyreese
left a comment
There was a problem hiding this comment.
Overall I think there is a better approach that relies on existing mechanisms in the noqa system. We already build a NoqaMapping data structure for mapping #noqa from one line to apply to other lines. I would suggest looking at where we build that mapping (ruff_linter/src/directives.rs in extract_noqa_line_for()) and add a new mapping when the file contains a shebang. I believe that solution will be much smaller and more direct than what you have proposed.
| let has_any_shebang = comment_ranges | ||
| .iter() | ||
| .any(|range| ShebangDirective::try_extract(locator.slice(range)).is_some()); |
There was a problem hiding this comment.
Since a shebang can only occur on the first line, this should use something like comment_ranges.first().map(...), and verify that the comment range starts at 0, so that it's not iterating through every single comment in the file or giving a false positive if there's a "shebang" on any line other than the first one.
|
Alright, I wil look into this |
reverts change but not tests to try another way to fix
Fix astral-sh#14442. If an error is found in the first line AND this first line is a shebang, then the noqa corresponding is expected in the second line.
|
I have a working solution using NoqaMapping, but with the way it works now, the noqa directive is expected to be on the next line. Thus, a noqa on the same line than the shebang won't work. It is not really a problem because with a shebang, the noqa on the same line would be a problem. |
|
@MichaReiser we triaged this as a bug, but since using To be clear, this would only affect diagnostics:
But in those cases, if the user actually added a |
--add-noqa breaking shebangs
There was a problem hiding this comment.
It's nice that we can solve this generically in noqa mapping. The only downside is that we'll need the same treatment for every ignore comment style (e.g. ruff: ignore[] once added). I don't think we need to change this now but an alternative would be to add an extra field to Diagnostic that stores an additional allowed suppression range. This is a feature that has come up in other contexts where it's sometimes desired to allow suppressions on lines other than the primary diagnostic range. I don't mind getting forward with the approach taken here.
I consider this a bugfix.
Hello,
This PR fixes the bug #14442 about. It adds a check on noqa application:
If the first line is a shebang, errors with range 0..0 can be suppressed by a noqa on the second line.
Example:
For D100,
broke the shebang.
Thus, you can now suppress errors like this with:
This also works for errors like CPY001, ... (anything that creates errors on range 0..0).
Implementation details
When suppressing errors in
check_noqa, adds a check: if the range of the error to suppress is 0..0, a shebang is present, try to get noqas on the second line. This is done by another functionfind_noqa_on_line_mut, that gather noqas of a specific line (with a binary search, similarly to the used functionfind_line_with_directive_mut).Test Plan
The check_noqa is tested for this specific case with other cases in
linter.rs, in the function test_shebang_noqa. Several cases are tested, including cases where the range is not 0..0, the file doesn't begin with a shebang, or the noqa is on the third line. Cases where suppression works are also tested.The
find_noqa_on_line_mutis tested innoqa_on_certain_linein the file it is declared.