From 79e52c7fdf90597d933aea771a9cde0ad510bba6 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 16 Jan 2025 12:44:01 +0530 Subject: [PATCH] [`pyflakes`] Show syntax error message for `F722` (#15523) ## Summary Ref: https://github.com/astral-sh/ruff/pull/15387#discussion_r1917796907 This PR updates `F722` to show syntax error message instead of the string content. I think it's more useful to show the syntax error message than the string content. In the future, when the diagnostics renderer is more capable, we could even highlight the exact location of the syntax error along with the annotation string. This is also in line with how we show the diagnostic in red knot. ## Test Plan Update existing test snapshots. --- .../src/types/string_annotation.rs | 4 +- crates/ruff_linter/src/checkers/ast/mod.rs | 120 +++++++++--------- .../flake8_annotations/rules/definition.rs | 2 +- .../rules/forward_annotation_syntax_error.rs | 6 +- ..._rules__pyflakes__tests__F722_F722.py.snap | 23 +--- ...ules__pyflakes__tests__F722_F722_1.py.snap | 7 +- .../src/rules/ruff/rules/implicit_optional.rs | 2 +- crates/ruff_linter/src/rules/ruff/typing.rs | 2 +- 8 files changed, 79 insertions(+), 87 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types/string_annotation.rs b/crates/red_knot_python_semantic/src/types/string_annotation.rs index a3c4dbd956d282..d900db4c7e4c76 100644 --- a/crates/red_knot_python_semantic/src/types/string_annotation.rs +++ b/crates/red_knot_python_semantic/src/types/string_annotation.rs @@ -153,9 +153,7 @@ pub(crate) fn parse_string_annotation( } else if raw_contents(node_text) .is_some_and(|raw_contents| raw_contents == string_literal.as_str()) { - let parsed = - ruff_python_parser::parse_string_annotation(source.as_str(), string_literal); - match parsed { + match ruff_python_parser::parse_string_annotation(source.as_str(), string_literal) { Ok(parsed) => return Some(parsed), Err(parse_error) => context.report_lint( &INVALID_SYNTAX_IN_FORWARD_ANNOTATION, diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index c78ab5ff48967a..e456481f2cbcd8 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -49,7 +49,7 @@ use ruff_python_ast::{helpers, str, visitor, PySourceType}; use ruff_python_codegen::{Generator, Stylist}; use ruff_python_index::Indexer; use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind, ParsedAnnotation}; -use ruff_python_parser::{Parsed, Tokens}; +use ruff_python_parser::{ParseError, Parsed, Tokens}; use ruff_python_semantic::all::{DunderAllDefinition, DunderAllFlags}; use ruff_python_semantic::analyze::{imports, typing}; use ruff_python_semantic::{ @@ -234,7 +234,7 @@ impl<'a> Checker<'a> { #[allow(clippy::too_many_arguments)] pub(crate) fn new( parsed: &'a Parsed, - parsed_annotations_arena: &'a typed_arena::Arena, + parsed_annotations_arena: &'a typed_arena::Arena>, settings: &'a LinterSettings, noqa_line_for: &'a NoqaMapping, noqa: flags::Noqa, @@ -425,7 +425,7 @@ impl<'a> Checker<'a> { pub(crate) fn parse_type_annotation( &self, annotation: &ast::ExprStringLiteral, - ) -> Option<&'a ParsedAnnotation> { + ) -> Result<&'a ParsedAnnotation, &'a ParseError> { self.parsed_annotations_cache .lookup_or_parse(annotation, self.locator.contents()) } @@ -441,7 +441,7 @@ impl<'a> Checker<'a> { match_fn: impl FnOnce(&ast::Expr) -> bool, ) -> bool { if let ast::Expr::StringLiteral(string_annotation) = expr { - let Some(parsed_annotation) = self.parse_type_annotation(string_annotation) else { + let Some(parsed_annotation) = self.parse_type_annotation(string_annotation).ok() else { return false; }; match_fn(parsed_annotation.expression()) @@ -2318,58 +2318,66 @@ impl<'a> Checker<'a> { while !self.visit.string_type_definitions.is_empty() { let type_definitions = std::mem::take(&mut self.visit.string_type_definitions); for (string_expr, snapshot) in type_definitions { - let annotation_parse_result = self.parse_type_annotation(string_expr); - if let Some(parsed_annotation) = annotation_parse_result { - self.parsed_type_annotation = Some(parsed_annotation); + match self.parse_type_annotation(string_expr) { + Ok(parsed_annotation) => { + self.parsed_type_annotation = Some(parsed_annotation); - let annotation = string_expr.value.to_str(); - let range = string_expr.range(); + let annotation = string_expr.value.to_str(); + let range = string_expr.range(); - self.semantic.restore(snapshot); + self.semantic.restore(snapshot); - if self.semantic.in_annotation() && self.semantic.in_typing_only_annotation() { - if self.enabled(Rule::QuotedAnnotation) { - pyupgrade::rules::quoted_annotation(self, annotation, range); + if self.semantic.in_annotation() + && self.semantic.in_typing_only_annotation() + { + if self.enabled(Rule::QuotedAnnotation) { + pyupgrade::rules::quoted_annotation(self, annotation, range); + } } - } - if self.source_type.is_stub() { - if self.enabled(Rule::QuotedAnnotationInStub) { - flake8_pyi::rules::quoted_annotation_in_stub(self, annotation, range); + if self.source_type.is_stub() { + if self.enabled(Rule::QuotedAnnotationInStub) { + flake8_pyi::rules::quoted_annotation_in_stub( + self, annotation, range, + ); + } } - } - let type_definition_flag = match parsed_annotation.kind() { - AnnotationKind::Simple => SemanticModelFlags::SIMPLE_STRING_TYPE_DEFINITION, - AnnotationKind::Complex => { - SemanticModelFlags::COMPLEX_STRING_TYPE_DEFINITION - } - }; - - self.semantic.flags |= - SemanticModelFlags::TYPE_DEFINITION | type_definition_flag; - let parsed_expr = parsed_annotation.expression(); - self.visit_expr(parsed_expr); - if self.semantic.in_type_alias_value() { - // stub files are covered by PYI020 - if !self.source_type.is_stub() && self.enabled(Rule::QuotedTypeAlias) { - flake8_type_checking::rules::quoted_type_alias( - self, - parsed_expr, - string_expr, - ); + let type_definition_flag = match parsed_annotation.kind() { + AnnotationKind::Simple => { + SemanticModelFlags::SIMPLE_STRING_TYPE_DEFINITION + } + AnnotationKind::Complex => { + SemanticModelFlags::COMPLEX_STRING_TYPE_DEFINITION + } + }; + + self.semantic.flags |= + SemanticModelFlags::TYPE_DEFINITION | type_definition_flag; + let parsed_expr = parsed_annotation.expression(); + self.visit_expr(parsed_expr); + if self.semantic.in_type_alias_value() { + // stub files are covered by PYI020 + if !self.source_type.is_stub() && self.enabled(Rule::QuotedTypeAlias) { + flake8_type_checking::rules::quoted_type_alias( + self, + parsed_expr, + string_expr, + ); + } } + self.parsed_type_annotation = None; } - self.parsed_type_annotation = None; - } else { - self.semantic.restore(snapshot); - - if self.enabled(Rule::ForwardAnnotationSyntaxError) { - self.push_type_diagnostic(Diagnostic::new( - pyflakes::rules::ForwardAnnotationSyntaxError { - body: string_expr.value.to_string(), - }, - string_expr.range(), - )); + Err(parse_error) => { + self.semantic.restore(snapshot); + + if self.enabled(Rule::ForwardAnnotationSyntaxError) { + self.push_type_diagnostic(Diagnostic::new( + pyflakes::rules::ForwardAnnotationSyntaxError { + parse_error: parse_error.error.to_string(), + }, + string_expr.range(), + )); + } } } } @@ -2541,12 +2549,12 @@ impl<'a> Checker<'a> { } struct ParsedAnnotationsCache<'a> { - arena: &'a typed_arena::Arena, - by_offset: RefCell>>, + arena: &'a typed_arena::Arena>, + by_offset: RefCell>>, } impl<'a> ParsedAnnotationsCache<'a> { - fn new(arena: &'a typed_arena::Arena) -> Self { + fn new(arena: &'a typed_arena::Arena>) -> Self { Self { arena, by_offset: RefCell::default(), @@ -2557,17 +2565,15 @@ impl<'a> ParsedAnnotationsCache<'a> { &self, annotation: &ast::ExprStringLiteral, source: &str, - ) -> Option<&'a ParsedAnnotation> { + ) -> Result<&'a ParsedAnnotation, &'a ParseError> { *self .by_offset .borrow_mut() .entry(annotation.start()) .or_insert_with(|| { - if let Ok(annotation) = parse_type_annotation(annotation, source) { - Some(self.arena.alloc(annotation)) - } else { - None - } + self.arena + .alloc(parse_type_annotation(annotation, source)) + .as_ref() }) } diff --git a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs index cad9679a559aad..b1ff9f4f14a28e 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -519,7 +519,7 @@ fn check_dynamically_typed( { if let Expr::StringLiteral(string_expr) = annotation { // Quoted annotations - if let Some(parsed_annotation) = checker.parse_type_annotation(string_expr) { + if let Ok(parsed_annotation) = checker.parse_type_annotation(string_expr) { if type_hint_resolves_to_any( parsed_annotation.expression(), checker, diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/forward_annotation_syntax_error.rs b/crates/ruff_linter/src/rules/pyflakes/rules/forward_annotation_syntax_error.rs index 18852477563b8f..dd4a2a59a5ce63 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/forward_annotation_syntax_error.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/forward_annotation_syntax_error.rs @@ -24,13 +24,13 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// - [PEP 563 – Postponed Evaluation of Annotations](https://peps.python.org/pep-0563/) #[derive(ViolationMetadata)] pub(crate) struct ForwardAnnotationSyntaxError { - pub body: String, + pub parse_error: String, } impl Violation for ForwardAnnotationSyntaxError { #[derive_message_formats] fn message(&self) -> String { - let ForwardAnnotationSyntaxError { body } = self; - format!("Syntax error in forward annotation: `{body}`") + let ForwardAnnotationSyntaxError { parse_error } = self; + format!("Syntax error in forward annotation: {parse_error}") } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722.py.snap index 05de73ad5ca720..54646f4ff37d1f 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722.py.snap @@ -1,15 +1,14 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs -snapshot_kind: text --- -F722.py:9:12: F722 Syntax error in forward annotation: `///` +F722.py:9:12: F722 Syntax error in forward annotation: Expected an expression | 9 | def g() -> "///": | ^^^^^ F722 10 | pass | -F722.py:13:4: F722 Syntax error in forward annotation: `List[int]☃` +F722.py:13:4: F722 Syntax error in forward annotation: Got unexpected token ☃ | 13 | X: """List[int]"""'☃' = [] | ^^^^^^^^^^^^^^^^^^ F722 @@ -17,10 +16,7 @@ F722.py:13:4: F722 Syntax error in forward annotation: `List[int]☃` 15 | # Type annotations with triple quotes can contain newlines and indentation | -F722.py:30:11: F722 Syntax error in forward annotation: ` - int | -str) -` +F722.py:30:11: F722 Syntax error in forward annotation: Unexpected token at the end of an expression | 28 | """ 29 | @@ -34,10 +30,7 @@ str) 35 | invalid2: """ | -F722.py:35:11: F722 Syntax error in forward annotation: ` - int) | -str -` +F722.py:35:11: F722 Syntax error in forward annotation: Unexpected token at the end of an expression | 33 | """ 34 | @@ -51,9 +44,7 @@ str 40 | ((int) | -F722.py:39:11: F722 Syntax error in forward annotation: ` - ((int) -` +F722.py:39:11: F722 Syntax error in forward annotation: unexpected EOF while parsing | 37 | str 38 | """ @@ -66,9 +57,7 @@ F722.py:39:11: F722 Syntax error in forward annotation: ` 43 | (int | -F722.py:42:11: F722 Syntax error in forward annotation: ` - (int -` +F722.py:42:11: F722 Syntax error in forward annotation: unexpected EOF while parsing | 40 | ((int) 41 | """ diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722_1.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722_1.py.snap index c7c943161e0b36..1fd4f43abd0a5f 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722_1.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722_1.py.snap @@ -1,8 +1,7 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs -snapshot_kind: text --- -F722_1.py:8:22: F722 Syntax error in forward annotation: `this isn't python` +F722_1.py:8:22: F722 Syntax error in forward annotation: Unexpected token at the end of an expression | 6 | @no_type_check 7 | class C: @@ -11,7 +10,7 @@ F722_1.py:8:22: F722 Syntax error in forward annotation: `this isn't python` 9 | x: "this also isn't python" = 1 | -F722_1.py:8:46: F722 Syntax error in forward annotation: `this isn't python either` +F722_1.py:8:46: F722 Syntax error in forward annotation: Unexpected token at the end of an expression | 6 | @no_type_check 7 | class C: @@ -20,7 +19,7 @@ F722_1.py:8:46: F722 Syntax error in forward annotation: `this isn't python eith 9 | x: "this also isn't python" = 1 | -F722_1.py:9:12: F722 Syntax error in forward annotation: `this also isn't python` +F722_1.py:9:12: F722 Syntax error in forward annotation: Unexpected token at the end of an expression | 7 | class C: 8 | def f(self, arg: "this isn't python") -> "this isn't python either": diff --git a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs index 718a39d4093b1a..69af7d4bc19223 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs @@ -179,7 +179,7 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters) if let Expr::StringLiteral(string_expr) = annotation.as_ref() { // Quoted annotation. - if let Some(parsed_annotation) = checker.parse_type_annotation(string_expr) { + if let Ok(parsed_annotation) = checker.parse_type_annotation(string_expr) { let Some(expr) = type_hint_explicitly_allows_none( parsed_annotation.expression(), checker, diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index f7ed900719e0e8..3faea2fa86bec4 100644 --- a/crates/ruff_linter/src/rules/ruff/typing.rs +++ b/crates/ruff_linter/src/rules/ruff/typing.rs @@ -109,7 +109,7 @@ impl<'a> TypingTarget<'a> { Expr::NoneLiteral(_) => Some(TypingTarget::None), Expr::StringLiteral(string_expr) => checker .parse_type_annotation(string_expr) - .as_ref() + .ok() .map(|parsed_annotation| { TypingTarget::ForwardReference(parsed_annotation.expression()) }),