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 a new Type::KnownInstanceType variant #14155

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

Fixes #14114. I don't think I can really describe the problems with our current architecture (and therefore the motivations for this PR) any better than @carljm did in that issue, so I'll just copy it out here!


We currently represent "known instances" (e.g. special forms like typing.Literal, which are an instance of typing._SpecialForm, but need to be handled differently from other instances of typing._SpecialForm) as an InstanceType with a known field that is Some(...).

This makes it easy to handle a known instance as if it were a regular instance type (by ignoring the known field), and in some cases (e.g. Type::member) that is correct and convenient. But in other cases (e.g. Type::is_equivalent_to) it is not correct, and we currently have a bug that we would consider the known-instance type of typing.Literal as equivalent to the general instance type for typing._SpecialForm, and we would fail to consider it a singleton type or a single-valued type (even though it is both.)

An instance type with known.is_some() is semantically quite different from an instance type with known.is_none(). The former is a singleton type that represents exactly one runtime object; the latter is an open type that represents many runtime objects, including instances of unknown subclasses. It is too error-prone to represent these very-different types as a single Type variant. We should instead introduce a dedicated Type::KnownInstance variant and force ourselves to handle these explicitly in all Type variant matches.

Possible followups

There is still a little bit of awkwardness in our current design in some places, in that we first infer the symbol typing.Literal as a _SpecialForm instance, and then later convert that instance-type into a known-instance-type. We could also use this KnownInstanceType enum to account for other special runtime symbols such as builtins.Ellipsis or builtins.NotImplemented.

I think these might be worth pursuing, but I didn't do them here as they didn't seem essential right now, and I wanted to keep the diff relatively minimal.

Test Plan

cargo test -p red_knot_python_semantic. New unit tests added for Type::is_subtype_of.

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Nov 7, 2024
@AlexWaygood AlexWaygood changed the title [red-knot] Add a new Type::KnownInstanceType variant [red-knot] Add a new Type::KnownInstanceType variant Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

freedomofpress/securedrop (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

Failed to pull: error: RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly: CANCEL (err 8)
error: 6437 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output

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!!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
Comment on lines +356 to +358
Type::Instance(InstanceType {
class: class_literal.class,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We do this a lot in this PR, I wonder about a Class::to_instance_type() method for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be considered separately as a follow-up if it's worth it

Copy link
Member Author

Choose a reason for hiding this comment

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

@carljm
Copy link
Contributor

carljm commented Nov 7, 2024

I think I might resolve conflicts and land this, if that's OK with you? I'm realizing that I think I want to represent a TypeVar (not as a type, but the type of the typevar's own symbol, e.g. T in def f[T](): ...) as a KnownInstanceType with fallback type typing.TypeVar.

@AlexWaygood
Copy link
Member Author

Go for it! It's late here and I'm still travelling back from choir 👍

@carljm carljm enabled auto-merge (squash) November 7, 2024 22:04
@carljm carljm merged commit 4b08d17 into main Nov 7, 2024
17 checks passed
@carljm carljm deleted the alex/known-instance-type branch November 7, 2024 22:07
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.

[red-knot] fix handling of known instances in Type relation methods
2 participants