Skip to content

[red-knot] No panic for illegal self-referential f-string annotations #14629

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

Closed
sharkdp wants to merge 2 commits into from
Closed

[red-knot] No panic for illegal self-referential f-string annotations #14629

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
4aba390
[red-knot] No panic for illegal self-referential f-string annotations
sharkdp Nov 27, 2024
36ba970
Remove TODO message
sharkdp Nov 27, 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
1 change: 0 additions & 1 deletion crates/red_knot_python_semantic/resources/mdtest/annotations/string.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ def f7() -> "\x69nt":
def f8() -> """int""":
return 1

# error: [annotation-byte-string] "Type expressions cannot use bytes literal"
def f9() -> "b'int'":
return 1

Expand Down
2 changes: 2 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/call/function.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ from typing import Callable
def foo() -> int:
return 42

# TODO: This should be resolved once we understand `Callable` annotations
# error: [annotation-with-invalid-expression]
def decorator(func) -> Callable[[], int]:
return foo

Expand Down
1 change: 1 addition & 0 deletions crates/red_knot_python_semantic/resources/mdtest/literal/literal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ Literal: _SpecialForm
```py
from other import Literal

# error: [annotation-with-invalid-expression]
a1: Literal[26]

def f():
Expand Down
229 changes: 121 additions & 108 deletions 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 @@ -31,6 +31,7 @@ use std::num::NonZeroU32;
use itertools::Itertools;
use ruff_db::files::File;
use ruff_db::parsed::parsed_module;
use ruff_python_ast::visitor::{self, Visitor};
use ruff_python_ast::{self as ast, AnyNodeRef, Expr, ExprContext, UnaryOp};
use rustc_hash::{FxHashMap, FxHashSet};
use salsa;
Expand Down Expand Up @@ -4214,25 +4215,6 @@ impl<'db> TypeInferenceBuilder<'db> {
// Annotation expressions also get special handling for `*args` and `**kwargs`.
ast::Expr::Starred(starred) => self.infer_starred_expression(starred),

ast::Expr::BytesLiteral(bytes) => {
self.diagnostics.add(
bytes.into(),
"annotation-byte-string",
format_args!("Type expressions cannot use bytes literal"),
);
Type::Unknown
}

ast::Expr::FString(fstring) => {
self.diagnostics.add(
fstring.into(),
"annotation-f-string",
format_args!("Type expressions cannot use f-strings"),
);
self.infer_fstring_expression(fstring);
Type::Unknown
}

// All other annotation expressions are (possibly) valid type expressions, so handle
// them there instead.
type_expr => self.infer_type_expression_no_store(type_expr),
Expand All @@ -4243,6 +4225,72 @@ impl<'db> TypeInferenceBuilder<'db> {
annotation_ty
}

/// Walk child expressions of the given AST node and store the given type for each of them.
/// Does not store a type for the root expression. Resets to normal type inference for all
/// expressions with a nested scope (lambda, named expression, comprehensions, generators).
fn store_type_for_sub_expressions_of(&mut self, expression: &ast::Expr, ty: Type<'db>) {
struct StoreTypeVisitor<'a, 'db> {
builder: &'a mut TypeInferenceBuilder<'db>,
ty: Type<'db>,
is_root_node: bool,
}

impl<'a, 'db> StoreTypeVisitor<'a, 'db> {
fn new(builder: &'a mut TypeInferenceBuilder<'db>, ty: Type<'db>) -> Self {
Self {
builder,
ty,
is_root_node: true,
}
}

fn store(&mut self, expr: &ast::Expr) {
if self.is_root_node {
self.is_root_node = false;
} else {
self.builder.store_expression_type(expr, self.ty);
}
}
}

impl<'a, 'db, 'ast> Visitor<'ast> for StoreTypeVisitor<'a, 'db> {
fn visit_expr(&mut self, expr: &'ast Expr) {
match expr {
ast::Expr::Lambda(lambda) => {
self.builder.infer_lambda_expression(lambda);
self.store(expr);
}
ast::Expr::Named(named) => {
self.builder.infer_named_expression(named);
self.store(expr);
}
ast::Expr::ListComp(list_comp) => {
self.builder.infer_list_comprehension_expression(list_comp);
self.store(expr);
}
ast::Expr::SetComp(set_comp) => {
self.builder.infer_set_comprehension_expression(set_comp);
self.store(expr);
}
ast::Expr::DictComp(dict_comp) => {
self.builder.infer_dict_comprehension_expression(dict_comp);
self.store(expr);
}
ast::Expr::Generator(generator) => {
self.builder.infer_generator_expression(generator);
self.store(expr);
}
_ => {
self.store(expr);
visitor::walk_expr(self, expr);
}
}
}
}

StoreTypeVisitor::new(self, ty).visit_expr(expression);
}

/// Infer the type of a string annotation expression.
fn infer_string_annotation_expression(&mut self, string: &ast::ExprStringLiteral) -> Type<'db> {
match parse_string_annotation(self.db, self.file, string) {
Expand Down Expand Up @@ -4324,13 +4372,6 @@ impl<'db> TypeInferenceBuilder<'db> {
// expressions, but is meaningful in the context of a number of special forms.
ast::Expr::EllipsisLiteral(_literal) => todo_type!(),

// Other literals do not have meaningful values in the annotation expression context.
// However, we will we want to handle these differently when working with special forms,
// since (e.g.) `123` is not valid in an annotation expression but `Literal[123]` is.
ast::Expr::BytesLiteral(_literal) => todo_type!(),
ast::Expr::NumberLiteral(_literal) => todo_type!(),
ast::Expr::BooleanLiteral(_literal) => todo_type!(),

ast::Expr::Subscript(subscript) => {
let ast::ExprSubscript {
value,
Expand Down Expand Up @@ -4379,90 +4420,62 @@ impl<'db> TypeInferenceBuilder<'db> {
// string annotation, as they are not present in the semantic index.
_ if self.deferred_state.in_string_annotation() => Type::Unknown,

// Forms which are invalid in the context of annotation expressions: we infer their
// nested expressions as normal expressions, but the type of the top-level expression is
// always `Type::Unknown` in these cases.
ast::Expr::BoolOp(bool_op) => {
self.infer_boolean_expression(bool_op);
Type::Unknown
}
ast::Expr::Named(named) => {
self.infer_named_expression(named);
Type::Unknown
}
ast::Expr::UnaryOp(unary) => {
self.infer_unary_expression(unary);
Type::Unknown
}
ast::Expr::Lambda(lambda_expression) => {
self.infer_lambda_expression(lambda_expression);
Type::Unknown
}
ast::Expr::If(if_expression) => {
self.infer_if_expression(if_expression);
Type::Unknown
}
ast::Expr::Dict(dict) => {
self.infer_dict_expression(dict);
Type::Unknown
}
ast::Expr::Set(set) => {
self.infer_set_expression(set);
Type::Unknown
}
ast::Expr::ListComp(listcomp) => {
self.infer_list_comprehension_expression(listcomp);
Type::Unknown
}
ast::Expr::SetComp(setcomp) => {
self.infer_set_comprehension_expression(setcomp);
Type::Unknown
}
ast::Expr::DictComp(dictcomp) => {
self.infer_dict_comprehension_expression(dictcomp);
Type::Unknown
}
ast::Expr::Generator(generator) => {
self.infer_generator_expression(generator);
Type::Unknown
}
ast::Expr::Await(await_expression) => {
self.infer_await_expression(await_expression);
Type::Unknown
}
ast::Expr::Yield(yield_expression) => {
self.infer_yield_expression(yield_expression);
Type::Unknown
}
ast::Expr::YieldFrom(yield_from) => {
self.infer_yield_from_expression(yield_from);
Type::Unknown
}
ast::Expr::Compare(compare) => {
self.infer_compare_expression(compare);
Type::Unknown
}
ast::Expr::Call(call_expr) => {
self.infer_call_expression(call_expr);
Type::Unknown
}
ast::Expr::FString(fstring) => {
self.infer_fstring_expression(fstring);
Type::Unknown
}
ast::Expr::List(list) => {
self.infer_list_expression(list);
Type::Unknown
}
ast::Expr::Tuple(tuple) => {
self.infer_tuple_expression(tuple);
Type::Unknown
}
ast::Expr::Slice(slice) => {
self.infer_slice_expression(slice);
// Forms which are invalid in the context of annotation expressions: we store a type of
// `Type::Unknown` for these expressions (and their sub-expressions) to avoid problems
// with invalid-syntax examples like `x: f"{x}"` or `x: lambda y: x`, for which we can
// not infer a meaningful type for the inner `x` expression. The top-level expression
// is also `Type::Unknown` in these cases.
ast::Expr::BoolOp(_)
| ast::Expr::Named(_)
| ast::Expr::UnaryOp(_)
| ast::Expr::Lambda(_)
| ast::Expr::If(_)
| ast::Expr::Dict(_)
| ast::Expr::Set(_)
| ast::Expr::ListComp(_)
| ast::Expr::SetComp(_)
| ast::Expr::DictComp(_)
| ast::Expr::Generator(_)
| ast::Expr::Await(_)
| ast::Expr::Yield(_)
| ast::Expr::YieldFrom(_)
| ast::Expr::Compare(_)
| ast::Expr::Call(_)
| ast::Expr::FString(_)
| ast::Expr::List(_)
| ast::Expr::Tuple(_)
| ast::Expr::Slice(_)
| ast::Expr::IpyEscapeCommand(_)
| ast::Expr::BytesLiteral(_)
| ast::Expr::NumberLiteral(_)
| ast::Expr::BooleanLiteral(_) => {
match expression {
ast::Expr::BytesLiteral(bytes) => {
self.diagnostics.add(
bytes.into(),
"annotation-byte-string",
format_args!("Type expressions cannot use bytes literal"),
);
}
ast::Expr::FString(fstring) => {
self.diagnostics.add(
fstring.into(),
"annotation-f-string",
format_args!("Type expressions cannot use f-strings"),
);
}
_ => {
self.diagnostics.add(
expression.into(),
"annotation-with-invalid-expression",
format_args!("Invalid expression in type expression"),
);
}
}

self.store_type_for_sub_expressions_of(expression, Type::Unknown);
Type::Unknown
}
ast::Expr::IpyEscapeCommand(_) => todo!("Implement Ipy escape command support"),
}
}

Expand Down
3 changes: 0 additions & 3 deletions crates/red_knot_workspace/tests/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,4 @@ const KNOWN_FAILURES: &[(&str, bool, bool)] = &[
("crates/ruff_linter/resources/test/fixtures/pyupgrade/UP039.py", true, false),
// related to circular references in type aliases (salsa cycle panic):
("crates/ruff_python_parser/resources/inline/err/type_alias_invalid_value_expr.py", true, true),
// related to circular references in f-string annotations (invalid syntax)
("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_15.py", true, true),
("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_14.py", false, true),
];
4 changes: 4 additions & 0 deletions crates/ruff_benchmark/benches/red_knot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@ static EXPECTED_DIAGNOSTICS: &[&str] = &[
// We don't support `*` imports yet:
"error[unresolved-import] /src/tomllib/_parser.py:7:29 Module `collections.abc` has no member `Iterable`",
// We don't support terminal statements in control flow yet:
"error[annotation-with-invalid-expression] /src/tomllib/_parser.py:57:71 Invalid expression in type expression",
Copy link
Member

@AlexWaygood AlexWaygood Nov 27, 2024

Choose a reason for hiding this comment

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

There are several type annotations on this line, and it's hard to tell which annotation this is referring to without counting the columns. Could we improve the error message so it specifies which variable or annotation the diagnostic is complaining about?

Copy link
Member

@MichaReiser MichaReiser Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Including arbitrary expressions is something that I'm always worried about because it can lead to very long messages, but including the name might be an option or David can come up with something that avoids this.

What should help here is when we have rich diagnostics that also show a code frame

Copy link
Contributor Author

@sharkdp sharkdp Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When I wrote that error message, I was looking for something like a ast::Expr::human_readable_name() method that would return "lambda expression" for a Expr::Lambda etc., but didn't find anything.

I'm happy to improve this error message though once we agree that this is even the right approach to begin with.

What should help here is when we have rich diagnostics that also show a code frame

👍

Copy link
Member

@AlexWaygood AlexWaygood Nov 27, 2024

Choose a reason for hiding this comment

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

What should help here is when we have rich diagnostics that also show a code frame

That should help for sure, but it's also useful to have important information in the summary line for when a user is using --output-format=concise. I found --output-format=concise to be very useful when filing PRs to upgrade projects to Ruff 0.8, because there were many new violations on some codebases, and it was very hard to scroll through them all when using the default verbose output.

"error[possibly-unresolved-reference] /src/tomllib/_parser.py:66:18 Name `s` used when possibly not defined",
"error[annotation-with-invalid-expression] /src/tomllib/_parser.py:69:66 Invalid expression in type expression",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:98:12 Name `char` used when possibly not defined",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:101:12 Name `char` used when possibly not defined",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:104:14 Name `char` used when possibly not defined",
"error[conflicting-declarations] /src/tomllib/_parser.py:108:17 Conflicting declared types for `second_char`: Unknown, str | None",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:115:14 Name `char` used when possibly not defined",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:126:12 Name `char` used when possibly not defined",
"error[annotation-with-invalid-expression] /src/tomllib/_parser.py:145:27 Invalid expression in type expression",
"error[annotation-with-invalid-expression] /src/tomllib/_parser.py:196:25 Invalid expression in type expression",
"error[conflicting-declarations] /src/tomllib/_parser.py:267:9 Conflicting declared types for `char`: Unknown, str | None",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:348:20 Name `nest` used when possibly not defined",
"error[possibly-unresolved-reference] /src/tomllib/_parser.py:353:5 Name `nest` used when possibly not defined",
Expand Down