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] Distribute intersections on negation #13962

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

rtpg
Copy link
Contributor

@rtpg rtpg commented Oct 28, 2024

Summary

This does two things:

The second point was to enable writing a test for the first case using protocols defined in typing (here, Sized and Hashable). Was a bit worried about another test that could be susceptible to simplifications, and it was straightforward to pull in

Test Plan

cargo test

@rtpg
Copy link
Contributor Author

rtpg commented Oct 28, 2024

Sorry, in my rush to send this in with the test I added passing I had intro'd a regression on another test. Please ignore this PR until I get this green (unless you know what's actually wrong here of course)

@MichaReiser MichaReiser marked this pull request as draft October 28, 2024 11:56
@MichaReiser
Copy link
Member

@rtpg no worries. I'll convert the PR to a draft and you can mark it as ready when it's ready :)

Copy link
Contributor

github-actions bot commented Oct 28, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@rtpg rtpg marked this pull request as ready for review October 29, 2024 00:12
@rtpg
Copy link
Contributor Author

rtpg commented Oct 29, 2024

Alright this one is ready for review, all green etc.

IntersectionBuilder::new(&db)
.add_positive(it)
.add_negative(st)
.build(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a part of me wants something like Type::intersection(elts: Vec<Type<'db>>) for testing but I got pretty used to writing out the intersection builder at this point 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, you aren't the first to say this after working on IntersectionBuilder, cc @sharkdp 😆 It's not a problem at all to add test helpers to make tests less verbose, someone just needs to feel the pain sufficiently to actually do it!

@@ -1232,6 +1244,9 @@ impl<'db> KnownClass {
| Self::Set
| Self::Dict => module.name() == "builtins",
Self::GenericAlias | Self::ModuleType | Self::FunctionType => module.name() == "types",
Self::Sized | Self::Hashable => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically all of the "interesting" protocols in typing are deprecated aliases at this point to members of collections.abc.

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 great, thank you! Just a few suggested changes, main one being to remove Sized and Hashable from KnownClass

@@ -63,6 +65,13 @@ pub(crate) fn types_symbol_ty<'db>(db: &'db dyn Db, symbol: &str) -> Type<'db> {
core_module_symbol_ty(db, CoreStdlibModule::Types, symbol)
}

/// Lookup the type of `symbol` in the `typing` module namespace.
///
/// Returns `Unbound` if the `types` module isn't available for some reason.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns `Unbound` if the `types` module isn't available for some reason.
/// Returns `Unbound` if the `typing` module isn't available for some reason.

Comment on lines 1138 to 1140
// Typing
Sized,
Hashable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to catch this only after you'd done the work, but I don't see any reason for these to be known classes? We should only use KnownClass when either a) the type has special-cased behavior that is not reflected in its typeshed definition, or b) we will commonly need to conjure this type "from thin air" within the type inference code itself, usually because the type is builtin and sometimes created "from thin air" by the runtime (e.g. bool) or because it is closely related to other special-cased types (e.g. KnownClass::Int and Type::IntLiteral, etc.)

I don't think Sized or Hashable meet either of those criteria.

If you want to use these types (or any other typeshed type) in tests, you don't need it to be a KnownClass; that's just a minor convenience. You can just use typing_symbol_ty (added in this PR, and something we'll definitely want) directly in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't quite internalized that I only actually really need typing_symbol_ty, thanks for pointing that out. Will remove theKnownClass entries

let i0 = IntersectionBuilder::new(&db)
.add_positive(ta)
.add_negative(t1)
.build();
// intersection = int & ~(Any & ~Literal[1])
// -> int & (~Any | Literal[1])
// (~Any is treated as Any for adding purposes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// (~Any is treated as Any for adding purposes)
// (~Any is equivalent to Any)

// -> int & (~Any | Literal[1])
// (~Any is treated as Any for adding purposes)
// -> (int & Any) | (int & Literal[1])
// -> (int & Any) | Literal[1]
let intersection = IntersectionBuilder::new(&db)
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing to name this intersection when it is actually a union :)

Suggested change
let intersection = IntersectionBuilder::new(&db)
let union = IntersectionBuilder::new(&db)

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW another option here is to just assert on the displayed form of the type, build_negative_union_de_morgan does that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first wasn't a fan because of the display issues, but given it's DNF (I belive) it's unambiguous... will do that

.add_positive(ta)
.build();

assert_eq!(intersection.elements(&db), &[i_and_a, t1,]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(intersection.elements(&db), &[i_and_a, t1,]);
assert_eq!(union.elements(&db), &[i_and_a, t1,]);

let it = KnownClass::Int.to_instance(&db);
let st = KnownClass::Sized.to_instance(&db);
let ht = KnownClass::Hashable.to_instance(&db);
// sh_t: Sized & ~Hashable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// sh_t: Sized & ~Hashable
// s_not_h_t: Sized & ~Hashable

also clean up some other details
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, thank you!!

@carljm carljm enabled auto-merge (squash) October 29, 2024 02:55
@carljm carljm merged commit 2fe2032 into astral-sh:main Oct 29, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants