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

Make Binding::range() point to the range of a type parameter's name, not the full type parameter #15935

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Feb 4, 2025

Summary

A PEP-695 type parameter creates a binding that is tracked by Ruff's semantic model:

def f[S](): ...
    # ^ `S` symbol bound here

But if the type parameter has a bound or constraint, the range of the Binding recorded by Ruff's semantic model includes the bounds or constraints of the type parameter:

def f[T: int](): ...
    # ^^^^^^ Semantic model's `Binding` for `T` has this range

def g[U: (int, str)](): ...
    # ^^^^^^^^^^^^^ Semantic models' binding for `U` has this range

This is problematic, as S: int is not a valid Python identifier, and nor is T: (int, str). All other bindings in Ruff's semantic model have their range point to the name that is bound, not the full expression/statement that creates the binding. A specific effect that this has is that if you have a Binding x that points to a PEP-695 type parameter, calling x.name(source) does not actually return the name of the PEP-695 type parameter. Instead, if x pointed to the binding for T in the example above, calling x.name(source) would return "T: int", and if x pointed to the binding for U, calling x.name(source) would return "U: (int, str)".

This PR changes the range for PEP-695 bindings so that it points to the name that is bound by the type parameter. This makes these Bindings consistent with other bindings, makes Binding::name() work as expected in all situations, allows us to simplify some code in the implementation of the PYI019 rule, and unblocks #15862

Test Plan

cargo test -p ruff_linter --lib

Co-authored-by: Brent Westbrook 36778786+ntBre@users.noreply.github.com

@AlexWaygood AlexWaygood added internal An internal refactor or improvement linter Related to the linter labels Feb 4, 2025
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Haha nice find.

Do you expect that this breaks any diagnostic range? E.g. do you know if the binding range is used in any diagnostic / fix code? I know, this is probably hard to figure out, just something that I'm slightly worried about but relying on our tests/user reports might be fine to catch this.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member Author

Do you expect that this breaks any diagnostic range?

It's very hard to tell... :(

E.g. do you know if the binding range is used in any diagnostic / fix code? I know, this is probably hard to figure out, just something that I'm slightly worried about but relying on our tests/user reports might be fine to catch this.

Agreed, and this is partly why I wanted to separate this out as a separate PR from #15862 (see my comment in #15862 (comment)). This is also why I worked around the bug in #15888 rather than fixing it prior to making that PR... but then @ntBre ran into exactly the same bug the next day in #15862! And I don't think it'll be so easy to workaround the bug in his PR. Plus, Binding::name() not working consistently for all bindings is a massive footgun. So I think it's time to fix the bug.

The fact that none of our snapshots change and the ecosystem check is clean gives me confidence that, if this does change our user-facing behaviour in some cases, they're at least rare/edge cases. And PEP-695 type parameters are anyway new syntax that are still rarely used in the real world. So I think we're good here.

@AlexWaygood AlexWaygood merged commit f23802e into main Feb 4, 2025
21 checks passed
@AlexWaygood AlexWaygood deleted the alex/tvar-binding-ranges branch February 4, 2025 14:14
dcreager added a commit that referenced this pull request Feb 4, 2025
* main: (66 commits)
  [red-knot] Use ternary decision diagrams (TDDs) for visibility constraints (#15861)
  [`pyupgrade`] Rename private type parameters in PEP 695 generics (`UP049`) (#15862)
  Simplify the `StringFlags` trait (#15944)
  [`flake8-pyi`] Make `PYI019` autofixable for `.py` files in preview mode as well as stubs (#15889)
  Docs (`linter.md`): clarify that Python files are always searched for in subdirectories (#15882)
  [`flake8-pyi`] Make PEP-695 functions with multiple type parameters fixable by PYI019 again (#15938)
  [red-knot] Use unambiguous invalid-syntax-construct for suppression comment test (#15933)
  Make `Binding::range()` point to the range of a type parameter's name, not the full type parameter (#15935)
  Update black deviations (#15928)
  [red-knot] MDTest: Fix line numbers in error messages (#15932)
  Preserve triple quotes and prefixes for strings (#15818)
  [red-knot] Hand-written MDTest parser (#15926)
  [`pylint`] Fix missing parens in unsafe fix for `unnecessary-dunder-call` (`PLC2801`) (#15762)
  nit: docs for ignore & select (#15883)
  [airflow] `BashOperator` has been moved to `airflow.providers.standard.operators.bash.BashOperator` (AIR302) (#15922)
  [`flake8-logging`] `.exception()` and `exc_info=` outside exception handlers (`LOG004`, `LOG014`) (#15799)
  [red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line (#15890)
  [red-knot] Internal refactoring of visibility constraints API (#15913)
  [red-knot] Implicit instance attributes (#15811)
  [`flake8-comprehensions`] Handle extraneous parentheses around list comprehension (`C403`) (#15877)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement linter Related to the linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants