Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Do not panic when encountering string annotations #14091

Merged
merged 2 commits into from
Nov 4, 2024
Merged

[red-knot] Do not panic when encountering string annotations #14091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fb8ca7
[red-knot] Do not panic when encountering string annotations
sharkdp Nov 4, 2024
0df668c
[red-knot] Add Type::Todo to expression types for string annotations
sharkdp Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/annotations/string_annotations.md
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" 😆

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# String annotations

```py
def f() -> "int":
return 1

# TODO: We do not support string annotations, but we should not panic if we encounter them
reveal_type(f()) # revealed: @Todo
```
5 changes: 4 additions & 1 deletion crates/red_knot_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3655,7 +3655,10 @@ impl<'db> TypeInferenceBuilder<'db> {
// TODO: parse the expression and check whether it is a string annotation, since they
// can be annotation expressions distinct from type expressions.
// https://typing.readthedocs.io/en/latest/spec/annotations.html#string-annotations
ast::Expr::StringLiteral(_literal) => Type::Todo,
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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
Loading