[pygrep_hooks] Fix blanket-noqa panic when last line has noqa with no newline (PGH004)#11108
Conversation
|
pygreb_hooks] Fix blanket-noqa panic when last line has noqa with no newline (PGH004)pygrep_hooks] Fix blanket-noqa panic when last line has noqa with no newline (PGH004)
| if let Directive::All(all) = &directive_line.directive { | ||
| let line = locator.slice(directive_line.range); | ||
| let offset = directive_line.range.start(); | ||
| let line = if directive_line.end() > locator.contents().text_len() { |
There was a problem hiding this comment.
This seems like a bug, if the .end() is greater than the contents... Have you looked into how that happens?
There was a problem hiding this comment.
I see the PR summary, but what happens if we remove it?
There was a problem hiding this comment.
Ya it generates a fixture failure with W292_1.py
There was a problem hiding this comment.
It seems the code mentioned in the PR summary was added by @MichaReiser so maybe he can shed some light.
|
Uff, I wrote this code like a year ago. But what I did is uncomment that specific code in the The problem is that we have fixes reported at the very end of the file, and there must be a way to suppress them. The way the noqa lines are found is by searching by a given offset: ruff/crates/ruff_linter/src/noqa.rs Lines 722 to 734 in de46a36 If we have two lines, than the I must admit, my "solution" is a hack and it would be nice if we could support end-of-file noqa comments in a better way. |
2ea2495 to
b1f451e
Compare
|
@MichaReiser @charliermarsh I implemented a fix which addresses the issue at its source. Let me know if you like it better. |
crates/ruff_linter/src/noqa.rs
Outdated
| Self { inner: directives } | ||
| Self { | ||
| inner: directives, | ||
| last_directive_includes_eof, |
There was a problem hiding this comment.
Nit: I think I would rename this to includes_end to make it clear that the comparison should include the end range.
crates/ruff_linter/src/noqa.rs
Outdated
| let index = self.inner.binary_search_by(|directive| { | ||
| if directive.range.end() < offset { | ||
| std::cmp::Ordering::Less | ||
| } else if directive.range.contains(offset) { | ||
| std::cmp::Ordering::Equal | ||
| } else { | ||
| std::cmp::Ordering::Greater | ||
| } | ||
| }); | ||
|
|
||
| match index { | ||
| Ok(index) => Some(index), | ||
| Err(index) => { | ||
| if self.last_directive_includes_eof && index == self.inner.len() - 1 { | ||
| Some(index) | ||
| } else { | ||
| std::cmp::Ordering::Greater | ||
| None | ||
| } | ||
| }) | ||
| .ok() | ||
| } | ||
| } |
There was a problem hiding this comment.
The search then becomes
self inner.binary_search_by(|directive| {
if directive.range.end() < offset {
std::cmp::Ordering::Less
} else if directive.range.start() > offset {
std::cmp::Ordering::Greater
}
// At this point, end >= offset, start <= offset
else if !directive.includes_end && directive.range.end() == offset {
std::cmp::Ordering::Less // I think or should it be greater?
} else {
std::cmp::Ordering::Equal
}
|
Done. I don't know what that wasm test failure is about. |
|
I think it's spurious, I'll re-run it. |
Summary
Resolves #11102
The error stems from these lines
ruff/crates/ruff_linter/src/noqa.rs
Lines 697 to 702 in f5c7a62
I don't really understand the purpose of incrementing the last index, but it makes the resulting range invalid for indexing into
contents.For now I just detect if the index is too high in
blanket_noqaand adjust it if necessary.Test Plan
Created fixture from issue example.