[red-knot] Add a new Type::ProtocolInstance variant (take 2!)
#17682
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
AlexWaygood
force-pushed
the
alex/protocol-variant-2
branch
from
2430d95 to
4a7cc03
Compare
Member
Author
|
Here's a minimized repro of the from typing import Protocol
class A(Protocol):
def x(self) -> "B | A": ...
class B(Protocol):
def y(self): ...
obj = record
if isinstance(obj, (B, A)):
objEDIT: I've now fixed this panic, and added the snippet as a regression test. |
AlexWaygood
force-pushed
the
alex/protocol-variant-2
branch
4 times, most recently
from
5bff579 to
4b957ea
Compare
Member
Author
This isn't a new false positive (just an error message changing slightly), but it's worth some further comment. The code is here: https://github.com/strawberry-graphql/strawberry/blob/fc1ab18a3334784e71e178de7063d4fbb90dce55/strawberry/experimental/pydantic/conversion.py#L104-L109. The reason we emit the error is that |
Member
Author
|
A 1-2% regression on the red-knot benchmarks: https://codspeed.io/astral-sh/ruff/branches/alex%2Fprotocol-variant-2 |
Member
Author
|
I spotted this line in the mypy_primer logs (it didn't make it into the comment posted by the bot): - error[lint:invalid-argument-type] zerver/tests/test_decorators.py:1609:25: Argument to this function is incorrect: Expected `_GetResponseCallable | _AsyncGetResponseCallable`, found `(request) -> Unknown`
+ error[lint:invalid-argument-type] zerver/tests/test_decorators.py:1609:25: Argument to this function is incorrect: Expected `_GetResponseCallable`, found `(request) -> Unknown`It's again not a new false positive, but it deserves comment for why the existing false positive isn't going away. We don't yet understand any I haven't addressed this in my PR, as:
I added failing tests for this outstanding issue in 6609ecc. |
carljm
approved these changes
Apr 30, 2025
| /// The set of Python objects with the given class in their __class__'s method resolution order. | ||
| /// Construct this variant using the `Type::instance` constructor function. | ||
| Instance(InstanceType<'db>), | ||
| NominalInstance(NominalInstanceType<'db>), |
Contributor
There was a problem hiding this comment.
As a favor to reviewers, next time you might consider splitting out a mass rename like this into its own PR :)
Member
Author
There was a problem hiding this comment.
Sorry about that :-(
It didn't feel to me like the change made sense as a standalone PR; the renaming is only a logical change in the context of us adding a new Instance variant alongside NominalInstance. And it wasn't clear to me that adding a new variant would work out well until this branch was finally compiling and panic-free.
I originally planned to try to keep a clean commit history with this PR branch (the renaming change is its own commit on this branch) so that it could be reviewed commit-by-commit. But the commit history on this branch ended up getting pretty messy due to playing Salsa-panic-whackamole over the last few days. I probably should have spent a bit longer cleaning up the commit history before moving it out of draft mode, but I've already had to fix merge conflicts several times due to the size of the branch, so I (selfishly) just wanted to open it up for review as soon as it was panic-free.
Will try to do better next time!
Contributor
There was a problem hiding this comment.
No worries, just a thought! Since you raised the question, I think the multi-PR sequencing that I was imagining is that when you had the idea that maybe we should rename Instance to NominalInstance if we add ProtocolInstance, there's still really no pressing need to do that rename right away. Having both Instance and ProtocolInstance is maybe confusing (though I think tbh it would probably be fine even if left that way) but otherwise not a problem. So it could be left to a later follow-up.
AlexWaygood
force-pushed
the
alex/protocol-variant-2
branch
from
83aa7c0 to
44c347e
Compare
… to `NominalInstanceType`
AlexWaygood
force-pushed
the
alex/protocol-variant-2
branch
from
44c347e to
2425b02
Compare
dcreager
added a commit
that referenced
this pull request
Apr 30, 2025
* main: [red-knot] Use 'full' salsa backtrace output that includes durability and revisions (#17735) [red-knot] Initial support for protocol types (#17682) [red-knot] Computing a type ordering for two non-normalized types is meaningless (#17734) [red-knot] Include salsa backtrace in check and mdtest panic messages (#17732) [red-knot] Fix control flow for `assert` statements (#17702) [red-knot] Fix recording of negative visibility constraints (#17731) [red-knot] Update salsa (#17730) [red-knot] Support overloads for callable equivalence (#17698) [red-knot] Run py-fuzzer in CI to check for new panics (#17719) Upload red-knot binaries in CI on completion of linux tests (#17720) [`flake8-use-pathlib`] Fix `PTH123` false positive when `open` is passed a file descriptor from a function call (#17705)
AlexWaygood
added a commit
that referenced
this pull request
May 3, 2025
## Summary
Currently this assertion fails on `main`, because we do not synthesize a
`__call__` attribute for Callable types:
```py
from typing import Protocol, Callable
from knot_extensions import static_assert, is_assignable_to
class Foo(Protocol):
def __call__(self, x: int, /) -> str: ...
static_assert(is_assignable_to(Callable[[int], str], Foo))
```
This PR fixes that.
See previous discussion about this in
#16493 (comment) and
#17682 (comment)
## Test Plan
Existing mdtests updated; a couple of new ones added.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds initial support for protocol types to red-knot.
Specifically, it does the following things:
Type::ProtocolInstancevariant. AProtocolInstanceTyperepresents "the set of all possible runtime objects that would satisfy a given protocol".Type::Instance/InstanceTypetoType::NominalInstance/NominalInstanceType, to differentiate them fromType::ProtocolInstance/ProtocolInstanceType.Type::instance()constructor: it now checks whether the class passed into the constructor is a protocol class. If it is, it returns aType::ProtocolInstance(); if not, it returns aType::NominalInstanceTypevariants altogether.typing.SupportsIndexandtyping.Sized. This special-casing is now redundant.KnownClass::Sizedvariant altogether: its only purpose was to achieve the special-cased support for this protocol in red-knot.There is still much to be done, and many TODOs remain. The most obvious piece of outstanding work is that we still do not consider the types of the members declared on a protocol class when we consider whether another type is assignable to a protocol type. I.e., this assertion currently fails on this branch, when it should pass due to the fact that
Bar.xhas typestrbutFoodemands that inhabitants of the type must have anxattribute of typeint:As such, this PR gets rid of many false-positive diagnostics associated with protocols. But it comes at the cost of introducing many potential false negatives. These will be tackled in followup PRs.
Test Plan
Displayimplementation of synthesized protocolsuvx --from=./python/py-fuzzer fuzz 0-2000 --only-new-bugs --baseline-executable=./target/main/debug/red_knot --bin=red_knotto verify that this branch doesn't introduce any new panics detectable by thepy-fuzzerscriptQUICKCHECK_TESTS=100000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stableto check that the property tests still pass