Skip to content

Commit

Permalink
[red-knot] Add a new Type::KnownInstanceType variant (#14155)
Browse files Browse the repository at this point in the history
## 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`.

---------

Co-authored-by: Carl Meyer <carl@astral.sh>
  • Loading branch information
AlexWaygood and carljm authored Nov 7, 2024
1 parent 5b6169b commit 4b08d17
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 148 deletions.
Loading

0 comments on commit 4b08d17

Please sign in to comment.