Skip to content

[flake8-annotations] Fix return type annotations to handle shadowed builtin symbols (ANN201, ANN202, ANN204, ANN205, ANN206) #20612

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 2, 2025
Merged

[flake8-annotations] Fix return type annotations to handle shadowed builtin symbols (ANN201, ANN202, ANN204, ANN205, ANN206) #20612

merged 4 commits into from
Oct 2, 2025

Conversation

danparizher
Copy link
Contributor

Summary

Fixes #20610

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 28, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

dscorbett
dscorbett reviewed Sep 30, 2025
View reviewed changes
crates/ruff_linter/src/rules/flake8_annotations/helpers.rs Outdated
PythonType::None => {
let (edit, binding) = checker
.importer()
.get_or_import_builtin_symbol("None", at, checker.semantic())
Copy link

Choose a reason for hiding this comment

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

None is a keyword: it shouldn’t be imported.

@ntBre ntBre added bug Something isn't working fixes Related to suggested fixes for violations labels Oct 1, 2025
ntBre
ntBre reviewed Oct 1, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good overall, just a couple of simplification suggestions and one question/possible missed case for the @override method.

crates/ruff_linter/src/rules/flake8_annotations/helpers.rs Outdated
.importer()
.get_or_import_builtin_symbol("str", at, checker.semantic())
.ok()?;
let expr = Expr::Name(ast::ExprName {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still preserve the original name helper? Possibly with a generic impl Into<Name> argument instead of &str? That seemed to help with some of the repetition here. It looks like we could possibly even include the get_or_import_builtin call in the helper.

@danparizher danparizher requested a review from ntBre October 1, 2025 23:14
ntBre
ntBre approved these changes Oct 2, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thank you! I pushed one commit moving the name helper back into type_expr just to keep it more consistent with the old code. I'm also not sure what I was thinking with the impl Into<Name> suggestion, so I just put that back to &str too 😅

crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs
)));
if let Some(return_type_str) = return_type {
// Convert the simple return type to a proper expression that handles shadowed builtins
let python_type = match return_type_str {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit awkward because we just did basically the same match inside of simple_magic_return_type. I tried moving simple_magic_return_type into this file and making it return a PythonType directly, though, and then we don't have a nice way to get back to the string representation. So I think this is fine as-is.

@ntBre ntBre enabled auto-merge (squash) October 2, 2025 22:40
@ntBre ntBre merged commit f9688bd into astral-sh:main Oct 2, 2025
35 checks passed
@danparizher danparizher deleted the fix-20610 branch October 2, 2025 22:48
dcreager added a commit that referenced this pull request Oct 3, 2025
@dcreager
Merge remote-tracking branch 'origin/main' into dcreager/non-inherited
407f7bb 
* origin/main:
  [`flake8-bugbear`] Include certain guaranteed-mutable expressions: tuples, generators, and assignment expressions (`B006`) (#20024)
  [`flake8-comprehensions`] Clarify fix safety documentation (`C413`) (#20640)
  [ty] improve base conda distinction from child conda (#20675)
  [`ruff`] Extend FA102 with listed PEP 585-compatible APIs (#20659)
  [`ruff`] Handle argfile expansion errors gracefully (#20691)
  [`flynt`] Fix f-string quoting for mixed quote joiners (`FLY002`) (#20662)
  [ty] Fix file root matching for `/`
  [ruff,ty] Enable tracing's `log` feature
  [`flake8-annotations`] Fix return type annotations to handle shadowed builtin symbols (`ANN201`, `ANN202`, `ANN204`, `ANN205`, `ANN206`) (#20612)
  Bump 0.13.3 (#20685)
  Update benchmarking CI for cargo-codspeed v4 (#20686)
  [ty] Support single-starred argument for overload call (#20223)
  [ty] `~T` should never be assignable to `T` (#20606)
  [`pylint`] Clarify fix safety to include left-hand hashability (`PLR6201`) (#20518)
  [ty] No union with `Unknown` for module-global symbols (#20664)
  [`ty`] Reject renaming files to start with slash in Playground (#20666)
  [ty] Enums: allow multiple aliases to point to the same member (#20669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntBre ntBre ntBre approved these changes

+1 more reviewer

@dscorbett dscorbett dscorbett left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ANN2 fixes can add return types that are shadowed
3 participants
@danparizher @dscorbett @ntBre