Skip to content

Conversation

@AlexWaygood
Copy link
Member

Summary

This PR moves red-knot's InstanceType struct out of class.rs and into its own submodule. The InstanceType::class field is made private; the only way of creating InstanceTypes is now through the Type::instance() method (which is also moved to the new instance.rs submodule).

The motivation for this change is that it helps pave the way for adding a new Type::ProtocolInstance variant. When we have such a variant, I'm envisaging that the Type::instance() method would change to look something like this:

impl<'db> Type<'db> {
    pub fn instance(db: &'db dyn Db, class: ClassType<'db>) -> Self {
        if class.is_protocol(db) {
            Self::ProtocolInstance(ProtocolInstanceType { class })
        } else {
            Self::Instance(InstanceType { class })
        }
    }
}

Since the Type::instance() method would be the only public constructor for both InstanceType and ProtocolInstanceType, this would guarantee the invariant that nominal instance types would always be represented in our model using Type::Instance variants, and Protocol instance types would always be represented using Type::ProtocolInstance variants.

I also moved the KnownInstanceType enum to its own submodule in this PR, as it has very little to do with the other types in class.rs, and has equally little to do with either InstanceType or the ProtocolInstanceType that live (/will live) in instance.rs.

This PR is best reviewed commit by commit! I tried to include helpful commit messages explaining what each commit does.

Test Plan

cargo test -p red_knot_python_semantic

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 21, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 21, 2025

mypy_primer results

No ecosystem changes detected ✅

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.

Changes here look fine to me either way. I'm curious to learn more about the pros and cons of a separate Type::ProtocolInstance vs reusing Type::Instance.

@dcreager
Copy link
Member

Changes here look fine to me either way. I'm curious to learn more about the pros and cons of a separate Type::ProtocolInstance vs reusing Type::Instance.

To make sure I understand the alternative you're suggesting: InstanceType would become an enum, and ProtocolInstance would live there instead of in Type?

@carljm
Copy link
Contributor

carljm commented Apr 21, 2025

To make sure I understand the alternative you're suggesting: InstanceType would become an enum, and ProtocolInstance would live there instead of in Type?

No, I was asking why we need an enum or ProtocolInstance at any level. It seems possible in principle for protocols to live entirely within Type::Instance and InstanceType, and differentiate internally based on class.is_protocol() of the wrapped class.

I do think there are likely advantages in clarity (and maybe even in performance) to having a separate variant, because it means we don't have to check is_protocol() as often, and because the behavior of many methods will be entirely different for protocols vs other instances, and because semantically protocol types are a very different thing from instance types. So I'm not trying to say it would be better not to have a separate variant. I'm just interested in whether there are reasons I'm not seeing why it is necessary.

@sharkdp sharkdp removed their request for review April 22, 2025 07:03
…struct an instance of `InstanceType` is now through the `Type::instance()` method.
@AlexWaygood AlexWaygood force-pushed the alex/move-instancetype branch 2 times, most recently from e1d5369 to 2f5d2dd Compare April 22, 2025 11:15
@AlexWaygood
Copy link
Member Author

I'm curious to learn more about the pros and cons of a separate Type::ProtocolInstance vs reusing Type::Instance.

Ah, sorry. Perhaps I should have written up a short design doc for this. I remember discussing this plan with you in a 1:1, but that was probably a couple of weeks ago by now, and that anyway wasn't a conversation that was visible to the rest of the team :-(

The first reason why I think it makes sense for ProtocolInstance to be a separate variant is just that it will be much less bug-prone than trying to combine nominal and structural subtyping in one variant. Protocol instances will have fundamentally different logic to nominal instances in many of our core type-relational methods such as Type::is_subtype_of, Type::is_assignable_to, Type::is_equivalent_to and Type::is_gradual_equivalent_to. We saw in #17126 that there were several bugs in our core type-relational methods to do with the four types nested under Type::Callable, mostly to do with the fact that we were often treating the four types as though they were the same when they were not. Nesting the types in the same variant made these mistakes easy to make and hard to spot: some of the bugs I fixed in that PR I knew about before I started working on the PR, but most of them I only discovered through the process of flattening out the nested type into four Type variants. I think having a separate ProtocolInstance variant from the start will be a sounder foundation on which to build structural subtyping.

A second reason is that, as we've discussed in the past, I think we will have to support synthesized protocols as well as protocols that point to a specific class definition. This will be important for normalizing protocols into types that share the same Salsa ID if they are in fact equivalent to each other, which is necessary for our implementation of Type::is_equivalent_to for union types. I.e., we need to ensure that this test eventually passes:

And unions containing equivalent protocols are recognised as equivalent, even when the order is not
identical:
```py
class HasY(Protocol):
y: str
class AlsoHasY(Protocol):
y: str
class A: ...
class B: ...
# TODO: this should pass
static_assert(is_equivalent_to(A | HasX | B | HasY, B | AlsoHasY | AlsoHasX | A)) # error: [static-assert-error]

So I'm imagining that the Type::ProtocolInstance variant will wrap a type that looks something like this:

enum ProtocolInstanceType<'db> {
    FromClass(ClassType<'db>),
    Synthesized(SynthesizedProtocolType<'db>),
}

#[salsa::tracked]
struct SynthesizedProtocolType<'db> {
    members: Box<[ProtocolMember<'db>]>,
}

As we've also discussed in the past, this also has the advantage that we will in the future be able to consider removing the Type::Callable variant and instead represent Callable types as synthesized protocols with a single member (__call__).

@AlexWaygood AlexWaygood force-pushed the alex/move-instancetype branch from 2f5d2dd to b0f07a1 Compare April 22, 2025 11:31
@AlexWaygood AlexWaygood enabled auto-merge (squash) April 22, 2025 11:31
@AlexWaygood AlexWaygood merged commit ae6fde1 into main Apr 22, 2025
19 checks passed
@AlexWaygood AlexWaygood deleted the alex/move-instancetype branch April 22, 2025 11:34
dcreager added a commit that referenced this pull request Apr 22, 2025
* main: (37 commits)
  [red-knot] Add list of failing/slow ecosystem projects (#17474)
  [red-knot] mypy_primer: extend ecosystem checks (#17544)
  [red-knot] Move `InstanceType` to its own submodule (#17525)
  [red-knot] mypy_primer: capture backtraces (#17543)
  [red-knot] mypy_primer: Use upstream repo (#17500)
  [red-knot] `typing.dataclass_transform` (#17445)
  Update dependency react-resizable-panels to v2.1.8 (#17513)
  Update dependency smol-toml to v1.3.3 (#17505)
  Update dependency uuid to v11.1.0 (#17517)
  Update actions/setup-node action to v4.4.0 (#17514)
  [red-knot] Fix variable name (#17532)
  [red-knot] Add basic subtyping between class literal and callable (#17469)
  [`pyupgrade`] Add fix safety section to docs (`UP030`) (#17443)
  [`perflint`] Allow list function calls to be replaced with a comprehension (`PERF401`) (#17519)
  Update pre-commit dependencies (#17506)
  [red-knot] Simplify visibility constraint handling for `*`-import definitions (#17486)
  [red-knot] Detect (some) invalid protocols (#17488)
  [red-knot] Correctly identify protocol classes (#17487)
  Update dependency ruff to v0.11.6 (#17516)
  Update Rust crate shellexpand to v3.1.1 (#17512)
  ...
@carljm carljm mentioned this pull request Apr 24, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants