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

[red-knot] Future-proof Type::is_disjoint_from() #15262

Merged
merged 2 commits into from
Jan 5, 2025

Conversation

AlexWaygood
Copy link
Member

(This PR is stacked on top of #15261; review that first.)

Summary

Currently we hardcode a lot of knowledge about various classes' MROs into Type::is_disjoint_from(). This is undesirable for two reasons:

  • If typeshed changes a class's MRO to reflect that it changed in a future Python version, or typeshed changes a class's MRO for some other reason, bugs will be silently introduced into this function without us noticing it.
  • Once we add support for generics, we'll want red-knot to understand that Literal["foo"] is a subtype of typing.Sequence[str] and typing.Iterable[str]; with the current way is_disjoint_from() is written, we would have to manually update the function to take account of that.

This PR reworks Type::is_disjoint_from() to use generic is_subclass_of() methods rather than hardcoding knowledge of various classes' MROs. This fixes both the above issues, and also makes Type::is_disjoint_from() more extensible for future improvements, such as incorporating knowledge of @final classes into the method.

Test Plan

  • No new unit tests added; this should be largely a pure refactor with no significant change to current behaviour.
  • I ran QUICKCHECK_TESTS=200000 cargo test -p red_knot_python_semantic -- --ignored types::property_tests::stable to verify that this doesn't introduce any new failures in the property tests

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Jan 4, 2025
Copy link
Contributor

github-actions bot commented Jan 4, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood force-pushed the alex/known-instance-disjointness branch from ab69315 to c44a2e7 Compare January 5, 2025 15:16
@AlexWaygood AlexWaygood force-pushed the alex/future-proof-disjointness branch from ee934fa to a89c77b Compare January 5, 2025 15:17
@AlexWaygood AlexWaygood force-pushed the alex/known-instance-disjointness branch from c44a2e7 to 8cad5be Compare January 5, 2025 15:23
@AlexWaygood AlexWaygood force-pushed the alex/future-proof-disjointness branch from a89c77b to 79e5743 Compare January 5, 2025 15:24
@AlexWaygood AlexWaygood force-pushed the alex/known-instance-disjointness branch from 8cad5be to deac0e4 Compare January 5, 2025 21:54
@AlexWaygood AlexWaygood force-pushed the alex/future-proof-disjointness branch from 79e5743 to cdb3a78 Compare January 5, 2025 21:55
Base automatically changed from alex/known-instance-disjointness to main January 5, 2025 22:49
@AlexWaygood AlexWaygood force-pushed the alex/future-proof-disjointness branch from cdb3a78 to bee1e59 Compare January 5, 2025 22:50
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.

Great!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood enabled auto-merge (squash) January 5, 2025 22:51
@AlexWaygood AlexWaygood merged commit 6097fd9 into main Jan 5, 2025
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/future-proof-disjointness branch January 5, 2025 22:56
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (60 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (29 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants