Skip to content

Fix E30X panics on blank lines with trailing white spaces#9907

Merged
MichaReiser merged 2 commits intoastral-sh:mainfrom
hoel-bagard:hoel/fix_E30X_panics
Feb 9, 2024
Merged

Fix E30X panics on blank lines with trailing white spaces#9907
MichaReiser merged 2 commits intoastral-sh:mainfrom
hoel-bagard:hoel/fix_E30X_panics

Conversation

@hoel-bagard
Copy link
Contributor

@hoel-bagard hoel-bagard commented Feb 9, 2024 •
edited
Loading

Summary

closes #9906
The #9906 issue is triggered by blank lines with trailing white spaces. The trailing spaces cause the assert_eq! that was added in this discussion to fail.

This PR removes the assert_eq!.

Test Plan

I tested the change on the python files provided in #9906 (on top of cargo test).
I also added a few fixtures with blank lines with trailing white spaces.

Issue example/explaination

The issue is triggered by blank lines with trailing white spaces.
For example, by creating a blank file with echo "\n \n" >> temp.py, we get the snippet given in the issue:

Running ruff on this, we get the following tokens:

(NonLogicalNewline, 0..1)
(NonLogicalNewline, 6..7)
(NonLogicalNewline, 7..8)

We can see that the end of the first token range's end and the second token range's start do not match because of the 5 spaces.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+33 -0 violations, +0 -0 fixes in 1 projects; 42 projects unchanged)

aws/aws-sam-cli (+33 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tests/integration/testdata/start_api/lambda_authorizers/app.py:122:17: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:123:15: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:131:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:132:9: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:133:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:134:10: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:135:9: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:136:11: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:138:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:140:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:14:1: I001 [*] Import block is un-sorted or un-formatted
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:159:5: E303 [*] Too many blank lines (2)
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:17:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:203:30: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:204:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:208:30: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:209:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:212:9: PLR6301 Method `_getEmptyStatement` could be a function, class method, or static method
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:284:26: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:285:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:286:26: E203 [*] Whitespace before ':'
... 8 additional changes omitted for rule E203
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:34:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:44:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:47:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:59:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:68:1: E302 [*] Expected 2 blank lines, found 1

Changes by rule (6 rules affected)

code total + violation - violation + fix - fix
E203 16 16 0 0 0
E221 7 7 0 0 0
E302 7 7 0 0 0
I001 1 1 0 0 0
E303 1 1 0 0 0
PLR6301 1 1 0 0 0

@hoel-bagard hoel-bagard marked this pull request as ready for review February 9, 2024 09:46
@MichaReiser
Copy link
Member

MichaReiser commented Feb 9, 2024

Thanks for looking into the panic. I'm reluctant to just remove the assertion without better understanding the reason why it is incorrect (I added it to guarantee it's only called on two directly following lines). Would you mind adding an example explaining why the two ranges don't end and start at the same position to the PR summary?

@MichaReiser MichaReiser added the bug Something isn't working label Feb 9, 2024
@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Feb 9, 2024

@MichaReiser This happens because of the whitespaces, I added an explanation to the PR summary.

@MichaReiser MichaReiser merged commit 12a91f4 into astral-sh:main Feb 9, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Feb 9, 2024

Thanks for the explanation. That makes sense

@hoel-bagard hoel-bagard mentioned this pull request Feb 9, 2024
Closed
@Porkepix Porkepix mentioned this pull request Feb 18, 2024
Merged
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
@hoel-bagard @nkxxll
Fix E30X panics on blank lines with trailing white spaces (astral-s…
37717bc 
…h#9907)
@johnslavik johnslavik mentioned this pull request Mar 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rules E30X panics with almost empty files

2 participants

@hoel-bagard @MichaReiser