[red-knot] Fixup a few edge cases regarding type[]

[AlexWaygood]

Summary

There's a functional change here, and a few cosmetic changes.

The functional change is that we currently consider Literal[str] to be disjoint from type[Any]. But they're not disjoint: Any here could be materialized as str or object; in those materializations, the two types would clearly overlap, and therefore type[Any] overlaps with Literal[str]. This PR fixes our logic in Type::is_disjoint_from to reflect that.

Also included in this PR are cosmetic changes to Type::is_subtype_of. The main reason for these changes is that currently we have Type::SubclassOf(target_class)) branches in match statements where the target_class variable actually has type ClassBase rather than Class; this is quite confusing. A secondary reason for these changes is that it should be impossible for us to encounter Type::SubclassOf(SubclassOfType { base }) instances in this match statement where base is not a ClassBase::Class() variant, since all other ClassBase::Class() variants are not fully static exit, and therefore we immediately early in is_subtype_of() here if we encounter one of them:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 607 to 612 in 6e11086

	pub(crate) fn is_subtype_of(self, db: &'db dyn Db, target: Type<'db>) -> bool {
	if self.is_equivalent_to(db, target) {
	return true;
	}
	if !self.is_fully_static(db) || !target.is_fully_static(db) {
	return false;

The changes to Type::is_subtype_of() clarify that we shouldn't be dealing with any other ClassBase::Class() variants in this match statement; we should therefore be using Class::is_subclass_of() rather than Class:is_subclass_of_base().

As a result of the above changes, Class::is_subclass_of_base() actually becomes unused, so this PR removes it as well. I think this is probably also for the best: a ClassBase and a Class are two different things, and it feels like it blurs the two concepts to have a method that asks whether a class is a "subclass of" a ClassBase. If we need to ask this question again in the future, it's not that much more verbose to just do class.mro(db).contains(&base).

Test Plan

cargo test -p red_knot_python_semantic. The new test case for the is_not_disjoint_from test is the only one added in this PR that fails on main, but the other added test cases seem useful anyway.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[carljm]

Thanks. I think I do prefer always explicitly unpacking to a Class, and avoiding methods that blur the line between a ClassBase and a Class.

[AlexWaygood]

Okay... I tried to add some more comments to the function... and ended up writing #14924... which I will have to finish tomorrow...

[AlexWaygood]

I think tomorrow I'll rip out the is_subtype_of parts of this so it only deals with is_disjoint_from. And I'll continue work on #14924, which deals with the is_subtype_of questions in a more comprehensive way.

[carljm]

I think tomorrow I'll rip out the is_subtype_of parts of this so it only deals with is_disjoint_from. And I'll continue work on #14924, which deals with the is_subtype_of questions in a more comprehensive way.

Sounds fine, though I also think it would be fine if this PR at least removes the is_subtype_of case that we know is wrong and doesn't fail any tests if removed, since we've established that much here. Just makes the follow up PR on is_subtype_of that much easier to review.

[AlexWaygood]

Sure

[carljm]

Looks good!

[carljm]

minor nit, but it doesn't seem necessary to explode this onto multiple lines? Is rustfmt doing that because the trailing comma was added?

[AlexWaygood]

no idea why rustfmt is doing it! I can see if I can get rustfmt to undo it

[AlexWaygood]

I cannot get rustfmt to undo it. Ours not to reason why?