[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!
| self.try_to_class_literal(db) | ||
| .map(Type::ClassLiteral) | ||
| .unwrap_or_else(|lookup_error| { | ||
| if cfg!(test) { |
There was a problem hiding this comment.
Or I suppose this could be
| if cfg!(test) { | |
| if cfg!(debug_assertions) { |
?
There was a problem hiding this comment.
or instead of being an if/else, this could simply be debug_assert!(false, "{}", lookup_error.display(db, self));
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
from
bb7020d to
a8c91a9
Compare
AlexWaygood
force-pushed
the
alex/to-instance-option
branch
from
a8c91a9 to
3df97e1
Compare
| @@ -1127,7 +1131,9 @@ impl<'db> Type<'db> { | |||
| !class | |||
| .metaclass(db) | |||
There was a problem hiding this comment.
Shouldn't it be an invariant that a class' metaclass is always something instantiable? It seems like it should be, since the class itself should be an instance of its metaclass.
Should we have a method that gets the instance type for a class' metaclass and handles this internally with an unreachable! instead?
There was a problem hiding this comment.
interesting point -- yes, I think you're right!
| self.try_to_class_literal(db) | ||
| .map(Type::ClassLiteral) | ||
| .unwrap_or_else(|lookup_error| { | ||
| if cfg!(test) { |
There was a problem hiding this comment.
or instead of being an if/else, this could simply be debug_assert!(false, "{}", lookup_error.display(db, self));
| if matches!( | ||
| lookup_error, | ||
| KnownClassLookupError::ClassPossiblyUnbound { .. } | ||
| ) { | ||
| tracing::warn!("{}", lookup_error.display(db, self)); | ||
| } else { | ||
| tracing::warn!( | ||
| "{}. Falling back to `Unknown` for the symbol instead.", | ||
| lookup_error.display(db, self) | ||
| ); | ||
| } |
There was a problem hiding this comment.
FWIW, I'm not totally convinced that we need to / should warn on this at all. It seems natural that if you don't have a class in your custom typeshed, then any special behavior we have around that class won't occur. If someone is doing this intentionally, it could be irritating to have a non-silencable warning about it (and using a "minimalist" typeshed doesn't seem like a crazy thing for someone to do intentionally). It could be accidental, but there are tons of ways a custom typeshed could quietly break things, it seems odd to warn about this specific one.
I don't feel strongly about it, though, what you have here seems fine and we can adjust with more experience.
There was a problem hiding this comment.
hmmm... yeah, I originally suggested just logging it (so that the information would be available to us when debugging, but wouldn't be printed by default), but @MichaReiser suggested in #16302 (comment) that a warning might be better... I think I'll switch it to being a LOG rather than a warning, so that you only get it if you specify --verbose on the CLI
| @@ -1557,4 +1656,35 @@ mod tests { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| fn setup_db_with_broken_typeshed(builtins_file: &str) -> TestDb { | |||
There was a problem hiding this comment.
I guess this is a wording nit, sort of related to my "not sure we need to warn" comment above -- but I slightly object to the framing of this as a "broken" typeshed. I think we should aim to consider typesheds where certain symbols aren't present or aren't what we expect to still be "working", where some behaviors will just fall back to gradual typing. Maybe it doesn't really matter, but I'd prefer we don't accept the idea that it's OK for things to be "broken" because a minimal typeshed is used.
There was a problem hiding this comment.
Fair enough. FWIW, we do panic (Salsa cycles etc.) if there's no object class in builtins; we did before this PR, and will continue to with this PR. I'm not sure I have the appetite to fix that "bug" :P
There was a problem hiding this comment.
If it's salsa cycles, I expect the panics will go away with fixpoint iteration? Will check! Not having object basically means everything is Unknown (because everything inherits Unknown)...
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():testfeature is enabled, we panic. This should enable us to quickly spot and solve bugs in tests to do with broken custom typesheds.testfeature 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
KnownClass::to_instance()appropriately panics if thetestfeature is enabled and we fail to lookup aKnownClassin typeshedQUICKCHECK_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):