Skip to content

Fix E231 bug: Inconsistent catch compared to pycodestyle, such as when dict nested in list#10469

Merged
MichaReiser merged 4 commits intoastral-sh:mainfrom
augustelalande:e231-nested-bug
Mar 21, 2024
Merged

Fix E231 bug: Inconsistent catch compared to pycodestyle, such as when dict nested in list#10469
MichaReiser merged 4 commits intoastral-sh:mainfrom
augustelalande:e231-nested-bug

Conversation

@augustelalande
Copy link
Contributor

@augustelalande augustelalande commented Mar 19, 2024

Summary

Fix E231 bug: Inconsistent catch compared to pycodestyle, such as when dict nested in list. Resolves #10113.

Test Plan

Example from #10113 added to test fixture.

@augustelalande augustelalande force-pushed the e231-nested-bug branch 2 times, most recently from f446cd4 to 2837639 Compare March 19, 2024 04:55
@github-actions
Copy link
Contributor

github-actions bot commented Mar 19, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added the bug Something isn't working label Mar 19, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Mar 20, 2024

Thanks for looking into this. Using a stack here makes sense. I'm surprised that this isn't hitting performance more, considering that we're now allocating a new Vec for each logical line (actually two).

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 20, 2024 •
edited
Loading

Thanks for looking into this. Using a stack here makes sense. I'm surprised that this isn't hitting performance more, considering that we're now allocating a new Vec for each logical line (actually two).

Ya it's surprising. But is allocating a new Vec really significantly more expensive than allocating any other variable, if it's not populated?

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 21, 2024

Took inspiration from extraneous_whitespace.rs and reduced it to one stack

@MichaReiser
Copy link
Member

MichaReiser commented Mar 21, 2024 •
edited
Loading

Ya it's surprising. But is allocating a new Vec really significantly more expensive than allocating any other variable, if it's not populated?

That's correct: Creating an empty Vec is "free" because it doesn't allocate. But a Vec is different from e.g. a TextSize in that it allocates memory on the heap when you push the first element. That's different from a TextSize variable that is always stored on the stack (which is as cheap as memory can be)

Took inspiration from extraneous_whitespace.rs and reduced it to one stack

Nice, thank you

MichaReiser
MichaReiser approved these changes Mar 21, 2024
View reviewed changes
crates/ruff_linter/src/rules/pycodestyle/rules/logical_lines/missing_whitespace.rs
match (kind, next_token.kind()) {
(TokenKind::Colon, _)
if open_parentheses > 0 && prev_lsqb > prev_lbrace =>
if matches!(brackets.last(), Some(TokenKind::Lsqb)) =>
Copy link
Member

@MichaReiser MichaReiser Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this much easier to understand. Nice refactor

@MichaReiser MichaReiser merged commit d9ac170 into astral-sh:main Mar 21, 2024
@augustelalande augustelalande deleted the e231-nested-bug branch March 21, 2024 12:12
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.

[E231] Inconsistent catch compared to pycodestyle, such as when dict nested in list

2 participants

@augustelalande @MichaReiser