[red-knot] Do not panic when encountering string annotations #14091
Conversation
sharkdp
requested review from
carljm,
MichaReiser and
AlexWaygood
as code owners
| infer_deferred_types(db, definition) | ||
| .try_expression_ty(expr_id) | ||
| .unwrap_or( | ||
| // Some deferred cases like string annotations are not yet implemented | ||
| Type::Todo, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'm a bit concerned that will forget to remove this after adding support for stringified type annotations.
Should we instead insert the type TODO here
ruff/crates/red_knot_python_semantic/src/types/infer.rs
Lines 3748 to 3749 in 985f211
There was a problem hiding this comment.
Should we instead insert the type TODO here
985f211/crates/red_knot_python_semantic/src/types/infer.rs#L3748-L3749
😶
There was a problem hiding this comment.
Should we instead insert the type TODO here
We already have this branch:
// TODO: parse the expression and check whether it is a string annotation.
// https://typing.readthedocs.io/en/latest/spec/annotations.html#string-annotations
ast::Expr::StringLiteral(_literal) => Type::Todo,It does not prevent the panic.
But I think I know what's wrong. Will push a better fix (hopefully).
|
| ast::Expr::StringLiteral(_literal) => { | ||
| let expr_id = expression.scoped_ast_id(self.db, self.scope()); | ||
| self.types.expressions.insert(expr_id, Type::Todo); | ||
| Type::Todo | ||
| } |
There was a problem hiding this comment.
If we do not add the expression type into self.types.expressions here (like we do for the other branches), it will later lead to a panic here:
sharkdp
force-pushed
the
david/no-panic-for-stringified-annotations
branch
from
7febfdc to
0df668c
Compare
There was a problem hiding this comment.
nit: If all we're asserting is "make sure we don't panic", then an mdtest is arguably overkill here: this could be better as a snippet added to the corpus directory (we have a test that runs over all the snippets in that directory and checks that we can at least run red-knot over them without panicking)
There was a problem hiding this comment.
I thought we would eventually have a Markdown-based test for string annotations anyway, and this test (with a non-@Todo-assertion) could very well be a basic initial test. I agree, so far it does little except document the fact that we don't understand string annotations yet.
But I'm happy to turn it into a snippet, if you prefer that.
There was a problem hiding this comment.
Yeah, fair enough. I suppose the corpus directory becomes less and less useful as we expand our ambitions beyond "do not panic when checking Python" to "infer the correct types consistently everywhere when checking Python" 😆
| ast::Expr::StringLiteral(_literal) => { | ||
| self.store_expression_type(expression, Type::Todo); | ||
| Type::Todo | ||
| } | ||
|
|
||
| // Annotation expressions also get special handling for `*args` and `**kwargs`. | ||
| ast::Expr::Starred(starred) => self.infer_starred_expression(starred), |
There was a problem hiding this comment.
I suspect we have the same bug here, for starred expressions in an annotation.
In general the contract is supposed to be that infer_expression takes care of storing the expression type, so we don't have to do it separately for every kind of expression.
For annotations, the tricky thing is the split between annotation expressions (what is valid in an annotation) and type expressions (which is a subset of annotation expressions). Currently infer_type_expression stores the type, but infer_annotation_expression does not, because most of the time it just defers to infer_type_expression, and we don't want to double-store. But we should come up with some general way to ensure annotation expression types are always stored, without having to do it separately in each branch that we add here.
There was a problem hiding this comment.
Oh, I did look at infer_starred_expression and saw that it uses infer_expression (which stores the expression type), but that's probably not enough… I'll look into it!
There was a problem hiding this comment.
It uses infer_expression on a sub-part of the starred expression (the value), but never on the starred expression itself. This is the usual pattern for infer_* methods for particular node types, since the assumption is they are called from infer_expression which will store the type they return.
* main: (39 commits) Also remove trailing comma while fixing C409 and C419 (astral-sh#14097) Re-enable clippy `useless-format` (astral-sh#14095) Derive message formats macro support to string (astral-sh#14093) Avoid cloning `Name` when looking up function and class types (astral-sh#14092) Replace `format!` without parameters with `.to_string()` (astral-sh#14090) [red-knot] Do not panic when encountering string annotations (astral-sh#14091) [red-knot] Add MRO resolution for classes (astral-sh#14027) [red-knot] Remove `Type::None` (astral-sh#14024) Cached inference of all definitions in an unpacking (astral-sh#13979) Update dependency uuid to v11 (astral-sh#14084) Update Rust crate notify to v7 (astral-sh#14083) Update cloudflare/wrangler-action action to v3.11.0 (astral-sh#14080) Update dependency mdformat-mkdocs to v3.1.1 (astral-sh#14081) Update pre-commit dependencies (astral-sh#14082) Update dependency ruff to v0.7.2 (astral-sh#14077) Update NPM Development dependencies (astral-sh#14078) Update Rust crate thiserror to v1.0.67 (astral-sh#14076) Update Rust crate syn to v2.0.87 (astral-sh#14075) Update Rust crate serde to v1.0.214 (astral-sh#14074) Update Rust crate pep440_rs to v0.7.2 (astral-sh#14073) ...
## Summary - Store the expression type for annotations that are starred expressions (see [discussion here](#14091 (comment))) - Use `self.store_expression_type(…)` consistently throughout, as it makes sure that no double-insertion errors occur. closes #14115 ## Test Plan Added an invalid-syntax example to the corpus which leads to a panic on `main`. Also added a Markdown test with a valid-syntax example that would lead to a panic once we implement function parameter inference. --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Summary
Encountered this while running red-knot benchmarks on the
blackcodebase.Fixes two of the issues in #13478.
Test Plan
Added a regression test.