Skip to content

Comments

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

Merged
AlexWaygood merged 2 commits intomainfrom
alex/future-proof-disjointness
Jan 5, 2025
Merged

[red-knot] Future-proof Type::is_disjoint_from()#15262
AlexWaygood merged 2 commits intomainfrom
alex/future-proof-disjointness

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 ty Multi-file analysis & type inference label Jan 4, 2025
@github-actions
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!

@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

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants