Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unused-noqa (RUF100) - false negatives and strange behavior with multiple codes #15682

Open
DetachHead opened this issue Jan 23, 2025 · 5 comments · May be fixed by #12811
Open

unused-noqa (RUF100) - false negatives and strange behavior with multiple codes #15682

DetachHead opened this issue Jan 23, 2025 · 5 comments · May be fixed by #12811
Labels
bug Something isn't working help wanted Contributions especially welcome

Comments

@DetachHead
Copy link

DetachHead commented Jan 23, 2025

Description

i assume this is an unintentional bug in the logic that allows you to add additional information to noqa comments:

# noqa: D103 this function is self-documenting

but ruff should enforce a space between the code and any additional comments. currently it doesn't seem to do so. it also seems to allow certain completely invalid codes:

# no error even though these aren't valid codes:
def foo(): ... # noqa: D103a
# noqa: Z

interestingly, these are treated as two separate codes:

def foo(): ... # noqa: D103F811

i think it should instead be an error and force the user to separate them with a comma like this:

def foo(): ... # noqa: D103,F811

https://play.ruff.rs/2b3608e9-f97a-459a-960b-f702827ef4e4

@MichaReiser
Copy link
Member

Thanks, I think we miss a check for leading_space to be != 0 here

while let Some(code) = Self::lex_code(&text[codes_end + leading_space..]) {
codes_end += leading_space;
codes.push(Code {
code,
range: TextRange::at(
TextSize::try_from(codes_end).unwrap(),
code.text_len(),
)
.add(offset),
});
codes_end += code.len();
// Codes can be comma- or whitespace-delimited. Compute the length of the
// delimiter, but only add it in the next iteration, once we find the next
// code.
if let Some(space_between) =
text[codes_end..].find(|c: char| !(c.is_whitespace() || c == ','))
{
leading_space = space_between;
} else {
break;
}
}

@MichaReiser MichaReiser added bug Something isn't working help wanted Contributions especially welcome labels Jan 23, 2025
@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Jan 23, 2025

See also #12809 and #14229. I posted a comment at the latter, but perhaps Charlie never saw it.

@MichaReiser Considering what happened in the two aforementioned PRs, this will require a design decision. What do you say?

@MichaReiser
Copy link
Member

Can you tell me more about the design decisions you see?

These could also be multiple fixes:

  • D103F811: Should not be treated as two rules
  • D103a: Handle content after a rule code

@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Jan 23, 2025

@MichaReiser #12809 made it so that D103F811 is treated as one code for both file-level and line-level # noqas. #14229 reverted it for line-level, but not for file-level. The result is now inconsistent:

# This hides the two diagnostics
print(list(sorted()))  # noqa: T201C413
# This doesn't, and the joint code is not reported as invalid.
# ruff: noqa: T201C413
print(list(sorted()))

My question never got an answer, so I assume that it is desired to keep this inconsistency. If you say otherwise, I can work on a fix. The PR will have concrete examples as well as how to deal with those and we can go from there.

@MichaReiser
Copy link
Member

Oh, so the problem linked in #14229 is that we incorrectly parsed text coming after a code as a noqa code.

That means we have to define a heuristic of what we treat as codes and what as non-code.

I also just found #12811

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Contributions especially welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants