Skip to content

[flake8-pyi] Expand PYI018 to cover ParamSpecs and TypeVarTuples#9198

Merged
charliermarsh merged 4 commits intoastral-sh:mainfrom
AlexWaygood:pyi018-false-negs
Dec 20, 2023
Merged

[flake8-pyi] Expand PYI018 to cover ParamSpecs and TypeVarTuples#9198
charliermarsh merged 4 commits intoastral-sh:mainfrom
AlexWaygood:pyi018-false-negs

Conversation

@AlexWaygood
Copy link
Copy Markdown
Member

Summary

Part of #8771. flake8-pyi will emit a Y018 error for unused TypeVars, ParamSpecs or TypeVarTuples; Ruff currently only emits PYI018 for unused TypeVars.

This is my first "proper" Ruff PR -- let me know if there's a better way of doing this! Not sure if the repeated calls to match_typing_expr() are ideal.

Test Plan

I manually updated the fixtures to add some unused ParamSpecs and TypeVarTuples, and then updated the snapshots using cargo insta review. All tests then passed when run using cargo test.

@AlexWaygood AlexWaygood changed the title Pyi018 false negs Expand PYI018 to cover ParamSpecs and TypeVarTuples Dec 19, 2023
@AlexWaygood AlexWaygood changed the title Expand PYI018 to cover ParamSpecs and TypeVarTuples [flake8-pyi] Expand PYI018 to cover ParamSpecs and TypeVarTuples Dec 19, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 19, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Dec 19, 2023
_ => {
continue;
}
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might write this like:

let typevarlike_kind = if semantic.match_typing_expr(f, "TypeVar")  {
   "TypeVar"
} else if ...

But I don't know that what I'm proposing is actually better, I just hadn't seen this pattern before :)

Copy link
Copy Markdown
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@charliermarsh charliermarsh enabled auto-merge (squash) December 20, 2023 03:04
Some("TypeVar")
} else if semantic.match_typing_call_path(&call_path, "ParamSpec") {
Some("ParamSpec")
} else if semantic.match_typing_call_path(&call_path, "TypeVarTuple") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AlexWaygood - I made one change here per your comment in the summary. Each call to semantic.match_typing_expr does a lookup internally to map the expression to a "call path", like ["typing", "TypeVar"]. However, we can just do that lookup once via semantic.resolve_call_path, then pass the call path to semantic.match_typing_call_path. (semantic.match_typing_expr is just a wrapper around this process, so if we want to do multiple checks, it's more efficient to do the lookup outside of the semantic.match_typing_expr.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ahh nice, thanks! Yep, that sounds like it's exactly what I was looking for :)

@charliermarsh charliermarsh merged commit bc0bf6f into astral-sh:main Dec 20, 2023
@AlexWaygood AlexWaygood deleted the pyi018-false-negs branch December 20, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants