Skip to content

Comments

[ty] fix assigning a typevar to a union with itself#17910

Merged
AlexWaygood merged 3 commits intomainfrom
alex/tvassign
May 7, 2025
Merged

[ty] fix assigning a typevar to a union with itself#17910
AlexWaygood merged 3 commits intomainfrom
alex/tvassign

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 7, 2025

Summary

This is an alternative PR to #17901. It retains the tests from that PR but uses a more targeted fix.

I don't think it's necessary to refactor the Type::is_subtype_of() and Type::is_assignable_to() methods to the extent that #17901 does. As far as I can see, the problem is specific to unions and intersections containing TypeVars. I don't think there's any situation where a TypeVar T can be assignable to a type S without its upper bound being assignable to S unless S is a union that directly contains T; this is the premise that my alternative implementation is based on.

I also added a couple more tests on top of the ones @carljm added.

Test Plan

cargo test -p ty_python_semantic

Co-authored-by: Carl Meyer [email protected]

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label May 7, 2025
@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2025 •
edited
Loading

mypy_primer results

Changes were detected when running on open source projects 
kopf (https://github.com/nolar/kopf)
- error[lint:invalid-assignment] kopf/_cogs/aiokits/aioenums.py:54:9: Object of type `FlagReasonT | Unknown` is not assignable to `FlagReasonT | None`
- Found 227 diagnostics
+ Found 226 diagnostics

mkosi (https://github.com/systemd/mkosi)
- error[lint:invalid-return-type] mkosi/config.py:1092:16: Return type does not match returned value: Expected `SE | None`, found `SE`
- Found 327 diagnostics
+ Found 326 diagnostics

operator (https://github.com/canonical/operator)
- error[lint:invalid-argument-type] ops/pebble.py:1833:67: Argument to this function is incorrect: Expected `AnyStr | None`, found `AnyStr`
- Found 208 diagnostics
+ Found 207 diagnostics

setuptools (https://github.com/pypa/setuptools)
- error[lint:invalid-return-type] setuptools/_distutils/spawn.py:38:16: Return type does not match returned value: Expected `_MappingT | dict[str, str | int] | None`, found `_MappingT | None`
- Found 1209 diagnostics
+ Found 1208 diagnostics

arviz (https://github.com/arviz-devs/arviz)
- error[lint:invalid-return-type] arviz/data/inference_data.py:972:20: Return type does not match returned value: Expected `InferenceDataT | None`, found `InferenceDataT`
- error[lint:invalid-return-type] arviz/data/inference_data.py:1052:20: Return type does not match returned value: Expected `InferenceDataT | None`, found `InferenceDataT`
- Found 2602 diagnostics
+ Found 2600 diagnostics

streamlit (https://github.com/streamlit/streamlit)
- error[lint:invalid-return-type] lib/streamlit/elements/lib/options_selector_utils.py:121:16: Return type does not match returned value: Expected `E1 | E2`, found `E1`
- error[lint:invalid-return-type] lib/streamlit/elements/lib/options_selector_utils.py:131:16: Return type does not match returned value: Expected `E1 | E2`, found `E1`
- error[lint:invalid-return-type] lib/streamlit/elements/lib/options_selector_utils.py:150:16: Return type does not match returned value: Expected `E1 | E2`, found `E1`
- Found 3291 diagnostics
+ Found 3288 diagnostics

@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 7, 2025

The primer results are identical to #17901

@AlexWaygood AlexWaygood mentioned this pull request May 7, 2025
Closed
@AlexWaygood AlexWaygood changed the title [ty] fix assigning a typevar to a union with itself [ty] fix assigning a typevar to a union with itself (alternative version) May 7, 2025
carljm
carljm approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! I'd vaguely considered this option but didn't love adding special handling for unions and intersections outside of their dedicated arms -- but now that I see it worked through, it's really not bad at all. The DNF normalization really helps us here.

crates/ty_python_semantic/src/types.rs Outdated
@@ -1003,6 +1003,26 @@ impl<'db> Type<'db> {
self_typevar == other_typevar
Copy link
Contributor

@carljm carljm May 7, 2025

Choose a reason for hiding this comment

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

This arm is removable because we (now, as of this PR) handle it in equivalence, which is already checked above.

Copy link
Member Author

@AlexWaygood AlexWaygood May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, it was already handled prior to this PR in equivalence! The last branch of the match statement handles it in is_equivalent_to(); the if self == other check before the match handles it in is_gradual_equivalent_to. The branches that you and I both added in our separate PRs was redundant.

I added tests (as you suggested in https://github.com/astral-sh/ruff/pull/17910/files#r2077710287) that demonstrate this.

@carljm carljm changed the title [ty] fix assigning a typevar to a union with itself (alternative version) [ty] fix assigning a typevar to a union with itself May 7, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) May 7, 2025 15:49
@AlexWaygood AlexWaygood merged commit c6f4929 into main May 7, 2025
30 checks passed
@AlexWaygood AlexWaygood deleted the alex/tvassign branch May 7, 2025 15:50
@AlexWaygood AlexWaygood mentioned this pull request May 7, 2025
Merged
carljm pushed a commit that referenced this pull request May 7, 2025
@AlexWaygood
[ty] Ensure that T is disjoint from ~T even when T is a TypeVar (…
895b616 
…#17922)

Same as #17910 but for
disjointness
dcreager added a commit that referenced this pull request May 7, 2025
@dcreager
Merge remote-tracking branch 'origin/main' into dcreager/generic-subc…
d83790f 
…lass

* origin/main:
  [`pylint`] add fix safety section (`PLC2801`) (#17825)
  Add instructions on how to upgrade to a newer Rust version (#17928)
  [parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893)
  [ty] Ensure that `T` is disjoint from `~T` even when `T` is a TypeVar (#17922)
  [ty] Sort collected diagnostics before snapshotting them in mdtest (#17926)
  [ty] Add basic file watching to server (#17912)
  Make completions an opt-in LSP feature (#17921)
  Add link to `ty` issue tracker (#17924)
  [ty] Add support for `__all__` (#17856)
  [ty] fix assigning a typevar to a union with itself (#17910)
  [ty] Improve UX for `[duplicate-base]` diagnostics (#17914)
  Clean up some Ruff references in the ty server (#17920)
@LaBatata101 LaBatata101 mentioned this pull request May 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AlexWaygood @carljm