Ignore end-of-line file exemption comments (#6160)

## Summary

This PR protects against code like:

```python
from typing import Optional

import bar  # ruff: noqa
import baz

class Foo:
    x: Optional[str] = None
```

In which the user wrote `# ruff: noqa` to ignore a specific error, not
realizing that it was a file-level exemption that thus turned off all
lint rules.

Specifically, if a `# ruff: noqa` directive is not at the start of a
line, we now ignore it and warn, since this is almost certainly a
mistake.
This commit is contained in:
Charlie Marsh 2023-07-28 20:40:32 -04:00 committed by GitHub
parent e0d5c7564f
commit 646ff6497c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 86 additions and 26 deletions

View file

@ -0,0 +1,5 @@
import os # ruff: noqa: F401
def f():
x = 1

View file

@ -12,6 +12,7 @@ use ruff_python_ast::Ranged;
use ruff_text_size::{TextLen, TextRange, TextSize};
use ruff_diagnostics::Diagnostic;
use ruff_python_trivia::indentation_at_offset;
use ruff_source_file::{LineEnding, Locator};
use crate::codes::NoqaCode;
@ -248,10 +249,20 @@ impl FileExemption {
let path_display = relativize_path(path);
warn!("Invalid `# ruff: noqa` directive at {path_display}:{line}: {err}");
}
Ok(Some(ParsedFileExemption::All)) => {
Ok(Some(exemption)) => {
if indentation_at_offset(range.start(), locator).is_none() {
#[allow(deprecated)]
let line = locator.compute_line_index(range.start());
let path_display = relativize_path(path);
warn!("Unexpected `# ruff: noqa` directive at {path_display}:{line}. File-level suppression comments must appear on their own line.");
continue;
}
match exemption {
ParsedFileExemption::All => {
return Some(Self::All);
}
Ok(Some(ParsedFileExemption::Codes(codes))) => {
ParsedFileExemption::Codes(codes) => {
exempt_codes.extend(codes.into_iter().filter_map(|code| {
if let Ok(rule) = Rule::from_code(get_redirect_target(code).unwrap_or(code))
{
@ -260,11 +271,13 @@ impl FileExemption {
#[allow(deprecated)]
let line = locator.compute_line_index(range.start());
let path_display = relativize_path(path);
warn!("Invalid code provided to `# ruff: noqa` at {path_display}:{line}: {code}");
warn!("Invalid rule code provided to `# ruff: noqa` at {path_display}:{line}: {code}");
None
}
}));
}
}
}
Ok(None) => {}
}
}

View file

@ -167,9 +167,9 @@ mod tests {
}
#[test]
fn ruff_noqa() -> Result<()> {
fn ruff_noqa_all() -> Result<()> {
let diagnostics = test_path(
Path::new("ruff/ruff_noqa.py"),
Path::new("ruff/ruff_noqa_all.py"),
&settings::Settings::for_rules(vec![Rule::UnusedImport, Rule::UnusedVariable]),
)?;
assert_messages!(diagnostics);
@ -177,9 +177,19 @@ mod tests {
}
#[test]
fn ruff_targeted_noqa() -> Result<()> {
fn ruff_noqa_codes() -> Result<()> {
let diagnostics = test_path(
Path::new("ruff/ruff_targeted_noqa.py"),
Path::new("ruff/ruff_noqa_codes.py"),
&settings::Settings::for_rules(vec![Rule::UnusedImport, Rule::UnusedVariable]),
)?;
assert_messages!(diagnostics);
Ok(())
}
#[test]
fn ruff_noqa_invalid() -> Result<()> {
let diagnostics = test_path(
Path::new("ruff/ruff_noqa_invalid.py"),
&settings::Settings::for_rules(vec![Rule::UnusedImport, Rule::UnusedVariable]),
)?;
assert_messages!(diagnostics);

View file

@ -1,7 +1,7 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
---
ruff_targeted_noqa.py:8:5: F841 [*] Local variable `x` is assigned to but never used
ruff_noqa_codes.py:8:5: F841 [*] Local variable `x` is assigned to but never used
|
7 | def f():
8 | x = 1

View file

@ -0,0 +1,32 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
---
ruff_noqa_invalid.py:1:8: F401 [*] `os` imported but unused
|
1 | import os # ruff: noqa: F401
| ^^ F401
|
= help: Remove unused import: `os`
Fix
1 |-import os # ruff: noqa: F401
2 1 |
3 2 |
4 3 | def f():
ruff_noqa_invalid.py:5:5: F841 [*] Local variable `x` is assigned to but never used
|
4 | def f():
5 | x = 1
| ^ F841
|
= help: Remove assignment to unused variable `x`
Suggested fix
2 2 |
3 3 |
4 4 | def f():
5 |- x = 1
5 |+ pass

View file

@ -12,7 +12,7 @@ pub fn indentation<'a, T>(locator: &'a Locator, located: &T) -> Option<&'a str>
where
T: Ranged,
{
indentation_at_offset(locator, located.start())
indentation_at_offset(located.start(), locator)
}
/// Return the end offset at which the empty lines following a statement.

View file

@ -297,7 +297,7 @@ fn handle_own_line_comment_between_branches<'a>(
// It depends on the indentation level of the comment if it is a leading comment for the
// following branch or if it a trailing comment of the previous body's last statement.
let comment_indentation = indentation_at_offset(locator, comment.slice().range().start())
let comment_indentation = indentation_at_offset(comment.slice().range().start(), locator)
.unwrap_or_default()
.len();
@ -402,7 +402,7 @@ fn handle_match_comment<'a>(
let next_case = match_stmt.cases.get(current_case_index + 1);
let comment_indentation = indentation_at_offset(locator, comment.slice().range().start())
let comment_indentation = indentation_at_offset(comment.slice().range().start(), locator)
.unwrap_or_default()
.len();
let match_case_indentation = indentation(locator, match_case).unwrap().len();
@ -480,7 +480,7 @@ fn handle_own_line_comment_after_branch<'a>(
// We only care about the length because indentations with mixed spaces and tabs are only valid if
// the indent-level doesn't depend on the tab width (the indent level must be the same if the tab width is 1 or 8).
let comment_indentation = indentation_at_offset(locator, comment.slice().range().start())
let comment_indentation = indentation_at_offset(comment.slice().range().start(), locator)
.unwrap_or_default()
.len();
@ -493,7 +493,7 @@ fn handle_own_line_comment_after_branch<'a>(
// # Trailing if comment
// ```
// Here we keep the comment a trailing comment of the `if`
let preceding_indentation = indentation_at_offset(locator, preceding_node.start())
let preceding_indentation = indentation_at_offset(preceding_node.start(), locator)
.unwrap_or_default()
.len();
if comment_indentation == preceding_indentation {

View file

@ -2,7 +2,7 @@ use ruff_source_file::Locator;
use ruff_text_size::{TextRange, TextSize};
/// Extract the leading indentation from a line.
pub fn indentation_at_offset<'a>(locator: &'a Locator, offset: TextSize) -> Option<&'a str> {
pub fn indentation_at_offset<'a>(offset: TextSize, locator: &'a Locator) -> Option<&'a str> {
let line_start = locator.line_start(offset);
let indentation = &locator.contents()[TextRange::new(line_start, offset)];