[red-knot] Rework Type::to_instance() to return Option<Type>
#16428
Conversation
AlexWaygood
force-pushed
the
alex/reject-bad-type-exprs
branch
3 times, most recently
from
2809aa4 to
e1bb7b1
Compare
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
3 times, most recently
from
afe0b34 to
bb7020d
Compare
| Self::SpecialForm | ||
| | Self::TypeVar | ||
| | Self::TypeAliasType | ||
| | Self::StdlibAlias | ||
| | Self::SupportsIndex => KnownModule::Typing, | ||
| Self::SpecialForm | Self::TypeVar | Self::StdlibAlias | Self::SupportsIndex => { | ||
| KnownModule::Typing | ||
| } | ||
| Self::TypeAliasType => KnownModule::TypingExtensions, |
There was a problem hiding this comment.
The change here is that the canonical module for KnownClass::TypeAliasType is now KnownModule::TypingExtensions rather than KnownModule::Typing. Running the property tests on this branch instantly crashed without this change, as it was revealed that until now KnownClass::TypeAliasType.to_instance(db) has been silently resolving to Type::Unknown all this time, due to the fact that it was only added to the typing module in Python 3.12 but our default Python version is 3.9.
AlexWaygood
requested review from
carljm,
MichaReiser and
sharkdp
as code owners
| // a cache of the `KnownClass`es that we have already failed to lookup in typeshed | ||
| // (and therefore that we've already logged a warning for) | ||
| static MESSAGES: LazyLock<Mutex<FxHashSet<KnownClass>>> = LazyLock::new(Mutex::default); |
There was a problem hiding this comment.
this approach was inspired by the existing Ruff macro warn_user_once_by_message!() here:
ruff/crates/ruff_linter/src/logging.rs
Lines 37 to 55 in 0c7c001
I couldn't find any tests for that macro, and I wasn't sure how to test this (since it deliberately works differently depending on whether the test feature is enabled, in order to prevent bugs silently going unnoticed in tests!). If anybody has any ideas, I'd be interested!
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
2 times, most recently
from
a8c91a9 to
3df97e1
Compare
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
3 times, most recently
from
d0e4f2c to
7b4eec3
Compare
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
2 times, most recently
from
b6aeb56 to
abc5605
Compare
crates/red_knot_python_semantic/resources/mdtest/mdtest_custom_typeshed.md
Outdated
Show resolved
Hide resolved
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
2 times, most recently
from
89c845a to
a3d8df4
Compare
|
I've rebased this, addressed review comments and improved some docs. @carljm it's changed a bit since you last took a look; you might want to give it a quick once-over again to check you're still okay with it. |
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
from
a3d8df4 to
c6a6cfe
Compare
|
@MichaReiser persuaded me in my 1:1 earlier that I should remove the panicking behaviour from |
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
from
c6a6cfe to
8f3f674
Compare
|
…'t unexpectedly fallback to `Type::Unknown` with full typeshed stubs (#16608) ## Summary One of the motivations in #16428 for panicking when the `test` or `debug_assertions` features are enabled and a lookup of a `KnownClass` fails is that we've had some latent bugs in our code where certain variants have been silently falling back to `Unknown` in every typeshed lookup without us realising. But that in itself isn't a great motivation for panicking in `KnownClass::to_instance()`, since we can fairly easily add some tests that assert that we don't unexpectedly fallback to `Unknown` for any `KnownClass` variant. This PR adds those tests. ## Test Plan `cargo test -p red_knot_python_semantic`
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
from
8f3f674 to
dadcd48
Compare
|
I think I've addressed/removed all the controversial points in this, and Carl approved an earlier version -- scheduling automerge now |
| tracing::info!("{}", lookup_error.display(db, self)); | ||
| } else { | ||
| tracing::info!( | ||
| "{}. Falling back to `Unknown` for the symbol instead.", | ||
| lookup_error.display(db, self) | ||
| ); |
There was a problem hiding this comment.
I made this tracing::info!() rather than tracing::warn!() or tracing::debug!().
tracing::warn!()is always showntracing::info!()is shown with--verbosebut not otherwise.tracing::debug!()is only shown with--verbose --verbose, and a lot of other tracing events are emitted if you specify that level of verbosity
* main: [red-knot] Rework `Type::to_instance()` to return `Option<Type>` (#16428) [red-knot] Add tests asserting that `KnownClass::to_instance()` doesn't unexpectedly fallback to `Type::Unknown` with full typeshed stubs (#16608) [red-knot] Handle gradual intersection types in assignability (#16611) [red-knot] mypy_primer: split installation and execution (#16622) [red-knot] mypy_primer: pipeline improvements (#16620) [red-knot] Infer `lambda` expression (#16547) [red-knot] mypy_primer: strip ANSI codes (#16604) [red-knot] mypy_primer: comment on PRs (#16599)
Summary
This PR fixes #16302.
The PR reworks
Type::to_instance()to returnOption<Type>rather thanType. This reflects more accurately the fact that some variants cannot be "turned into an instance", since they already represent instances of some kind. Onmain, we silently fallback toUnknownfor these variants, but this implicit behaviour can be somewhat surprising and lead to unexpected bugs.Returning
Option<Type>rather thanTypemeans that each callsite has to account for the possibility that the type might already represent an instance, and decide what to do about it.In general, I think this increases the robustness of the code. Working on this PR revealed two latent bugs in the code:
I added special handling to
KnownClass::to_instance(): If we fail to find one of these classes and thetestfeature is not enabled, we log a warning to the terminal saying that we failed to find the class in typeshed and that we will be falling back toType::Unknown. A cache is maintained so that we record all classes that we have already logged a warning for; we only log a warning for failing to lookup aKnownClassif we know that it's the first time we're looking it up.Test Plan
QUICKCHECK_TESTS=1000000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stableI also manually checked that warnings are appropriately printed to the terminal when
KnownClass::to_instance()falls back toUnknownand thetestfeature is not enabled. To do this, I applied this diff to the PR branch:Patch deleting `int` and `str` from buitins
And then ran red-knot on my typeshed-stats project using the command
I observed that the following logs were printed to the terminal, but that each warning was only printed once (the desired behaviour):