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

[perflint] Simplify finding the loop target in PERF401 #15025

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

w0nder1ng
Copy link
Contributor

Fixes #15012.

def f():
    # panics when the code can't find the loop variable
    values = [1, 2, 3]
    result = []
    for i in values:
        result.append(i + 1)
    del i

I'm not sure exactly why this test case panics, but I suspect the del i removes the binding from the semantic model's symbols.

I changed the code to search for the correct binding by directly iterating through the bindings. Since we know exactly which binding we want, this should find the loop variable without any complications.

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

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

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)


python-poetry/poetry (+0 -1 violations, +0 -0 fixes)

- tests/console/commands/test_show.py:127:5: B018 Found useless expression. Either assign it to a variable or remove it.

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
B018 1 0 1 0 0

Linter (preview)

✅ ecosystem check detected no linter changes.

@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 16, 2024

I'm not sure exactly why this test case panics, but I suspect the del i removes the binding from the semantic model's symbols.

Yep. This is the cause of a few cases of strange behavior... When the semantic model is called "as it's built" then this behavior is correct (because at the program point of interest, the delete has not yet ocurred), but it can be counter-intuitive when called (like in the current situation) on deferred scopes/for-loops/etc.

@dylwil3 dylwil3 changed the title [perflint] Simplify finding the loop target in perf401 [perflint] Simplify finding the loop target in PERF401 Dec 17, 2024
@dylwil3 dylwil3 added bug Something isn't working rule Implementing or modifying a lint rule labels Dec 17, 2024
.bindings
.iter()
.find(|binding| for_stmt.target.range() == binding.range)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Is this unwrap safe? If so, we should use expect but I'd possibly just do an early return if this is None.

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine. The rule only applies to for-loop targets that are names; a binding must exist.

@dhruvmanila dhruvmanila removed the rule Implementing or modifying a lint rule label Dec 17, 2024
@MichaReiser MichaReiser added the preview Related to preview mode features label Dec 17, 2024
@MichaReiser MichaReiser merged commit e22718f into astral-sh:main Dec 17, 2024
21 checks passed
dcreager added a commit that referenced this pull request Dec 17, 2024
* main:
  [red-knot] Explicitly test diagnostics are emitted for unresolvable submodule imports (#15035)
  Fix stale File status in tests (#15030)
  [red-knot] Basic support for other legacy `typing` aliases (#14998)
  feat(AIR302): extend the following rules (#15015)
  [`perflint`] Simplify finding the loop target in `PERF401` (#15025)
  [red-knot] Avoid undeclared path when raising conflicting declarations (#14958)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checking file with rule PERF401 cause panic
4 participants