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] add Type::is_disjoint_from and intersection simplifications #13775

Merged
merged 38 commits into from
Oct 18, 2024

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Oct 16, 2024

Summary

  • Add Type::is_disjoint_from as a way to test whether two types overlap
  • Add a first set of simplification rules for intersection types
    • S & T = S for S <: T
    • S & ~T = Never for S <: T
    • ~S & ~T = ~T for S <: T
    • A & ~B = A for A disjoint from B
    • A & B = Never for A disjoint from B
    • bool & ~Literal[bool] = Literal[!bool]

resolves one item in #12694

Open questions:

  • Can we somehow leverage the (anti) symmetry between positive and negative contributions? I could imagine that there would be a way if we had Type::Not(type)/Type::Negative(type), but with the positive/negative architecture, I'm not sure. Note that there is a certain duplication in the add_positive/add_negative functions (e.g. S & ~T = Never is implemented twice), but other rules are actually not perfectly symmetric: S & T = S vs ~S & ~T = ~T.
  • I'm not particularly proud of the way add_positive/add_negative turned out. They are long imperative-style functions with some mutability mixed in (to_remove). I'm happy to look into ways to improve this code if we decide to go with this approach of implementing a set of ad-hoc rules for simplification.
  • Is it useful to perform simplifications eagerly in add_positive/add_negative? (@carljm) This is what I did for now.

Test Plan

  • Unit tests for Type::is_disjoint_from
  • Observe changes in Markdown-based tests
  • Unit tests for IntersectionBuilder::build()

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Oct 16, 2024
Copy link
Contributor

github-actions bot commented Oct 16, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@sharkdp sharkdp force-pushed the david/dnf-type-simplifications branch from cd903c3 to e94ba83 Compare October 16, 2024 14:43
@sharkdp sharkdp force-pushed the david/dnf-type-simplifications branch from e94ba83 to 2bd1789 Compare October 17, 2024 11:24
@sharkdp sharkdp force-pushed the david/dnf-type-simplifications branch from 215ef0c to bf4af40 Compare October 17, 2024 12:40
@carljm
Copy link
Contributor

carljm commented Oct 17, 2024

Can we somehow leverage the (anti) symmetry between positive and negative contributions? I could imagine that there would be a way if we had Type::Not(type)/Type::Negative(type), but with the positive/negative architecture, I'm not sure.

FWIW, I'm quite open to the idea that we discard the positive/negative approach in favor of a Type::Not, if it makes intersection simplification easier.

The downside is we then have to add handling for Type::Not throughout the type system, but I suspect that may actually not be too difficult, since in many places we can just treat it as object, which is less precise but never unsound.

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.

This is great! I suspect that this approach will carry us a long way; I'm not feeling any particular need to explore more radical approaches. (I'm not 100% sure, but I suspect that the lazy-BDD approach I linked you to may just not be workable anyway for the complexity of Python's type system; particularly inheritance and multiple inheritance.)

crates/red_knot_python_semantic/src/types.rs Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/builder.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/builder.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/builder.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/builder.rs Outdated Show resolved Hide resolved
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 like this is still in-process, so didn't do a full re-review of the code, just a few responses to inline comments.

Comment on lines 487 to 490
(ty @ (Type::Function(..) | Type::Module(..) | Type::Class(..)), other)
| (other, ty @ (Type::Function(..) | Type::Module(..) | Type::Class(..))) => {
ty != other
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It just occurred to me now on second read-through that this case is incorrect if it comes before the union case, because the union of two Type::Function is not disjoint from one of those same Type::Function.

I think this case is really the same as the None and Literal types case below, and could probably just be merged with it?

Would be good to add a test case that would have caught this (to create a union of function or class types, define them in if/else branches.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just occurred to me now on second read-through that this case is incorrect if it comes before the union case, because the union of two Type::Function is not disjoint from one of those same Type::Function.

Ah, you're right.

I think this case is really the same as the None and Literal types case below, and could probably just be merged with it?

👍

Would be good to add a test case that would have caught this (to create a union of function or class types, define them in if/else branches.)

Yes. I added a test that failed with the previous version.

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Show resolved Hide resolved
@sharkdp sharkdp force-pushed the david/dnf-type-simplifications branch from c2930da to 34b0383 Compare October 18, 2024 20:15
(Type::BooleanLiteral(..), Type::Instance(class_type))
| (Type::Instance(class_type), Type::BooleanLiteral(..)) => !matches!(
class_type.known(db),
Some(KnownClass::Bool | KnownClass::Object)
Copy link
Contributor

@carljm carljm Oct 18, 2024

Choose a reason for hiding this comment

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

Because bool is a subtype of int in Python (sadly), we must also include KnownClass::Int here.

There was already a test for this case, but it asserted wrongly that int and a BooleanLiteral were disjoint; I switched it to assert the opposite.

There was also an IntersectionBuilder test relying on this assumption of bool/int disjointness; I fixed it by using a StringLiteral instead of a BooleanLiteral.

@carljm carljm force-pushed the david/dnf-type-simplifications branch from e5af7c9 to b3b0ffb Compare October 18, 2024 21:29
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.

This is excellent work, thank you!!

@carljm carljm enabled auto-merge (squash) October 18, 2024 21:31
@carljm carljm merged commit 6964eef into main Oct 18, 2024
18 checks passed
@carljm carljm deleted the david/dnf-type-simplifications branch October 18, 2024 21:34
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.

4 participants