Skip to content

Commit

Permalink
[pyflakes] Show syntax error message for F722 (#15523)
Browse files Browse the repository at this point in the history 
## Summary

Ref: #15387 (comment)

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.
  • Loading branch information
@dhruvmanila
dhruvmanila authored Jan 16, 2025
1 parent cf4ab7c commit 79e52c7
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 87 deletions.
4 changes: 1 addition & 3 deletions crates/red_knot_python_semantic/src/types/string_annotation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
120 changes: 63 additions & 57 deletions crates/ruff_linter/src/checkers/ast/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -234,7 +234,7 @@ impl<'a> Checker<'a> {
#[allow(clippy::too_many_arguments)]
pub(crate) fn new(
parsed: &'a Parsed<ModModule>,
parsed_annotations_arena: &'a typed_arena::Arena<ParsedAnnotation>,
parsed_annotations_arena: &'a typed_arena::Arena<Result<ParsedAnnotation, ParseError>>,
settings: &'a LinterSettings,
noqa_line_for: &'a NoqaMapping,
noqa: flags::Noqa,
Expand Down Expand Up @@ -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())
}
Expand All @@ -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())
Expand Down Expand Up @@ -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(),
));
}
}
}
}
Expand Down Expand Up @@ -2541,12 +2549,12 @@ impl<'a> Checker<'a> {
}

struct ParsedAnnotationsCache<'a> {
arena: &'a typed_arena::Arena<ParsedAnnotation>,
by_offset: RefCell<FxHashMap<TextSize, Option<&'a ParsedAnnotation>>>,
arena: &'a typed_arena::Arena<Result<ParsedAnnotation, ParseError>>,
by_offset: RefCell<FxHashMap<TextSize, Result<&'a ParsedAnnotation, &'a ParseError>>>,
}

impl<'a> ParsedAnnotationsCache<'a> {
fn new(arena: &'a typed_arena::Arena<ParsedAnnotation>) -> Self {
fn new(arena: &'a typed_arena::Arena<Result<ParsedAnnotation, ParseError>>) -> Self {
Self {
arena,
by_offset: RefCell::default(),
Expand All @@ -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()
})
}

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ fn check_dynamically_typed<F>(
{
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,
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/rules/pyflakes/rules/forward_annotation_syntax_error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
}
}
23 changes: 6 additions & 17 deletions ...inter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722.py.snap
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,22 @@
---
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
14 |
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 |
Expand All @@ -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 |
Expand All @@ -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 | """
Expand All @@ -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 | """
Expand Down
7 changes: 3 additions & 4 deletions ...ter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F722_F722_1.py.snap
View file
Original file line number Diff line number Diff line change
@@ -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:
Expand All @@ -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:
Expand All @@ -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":
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/ruff/typing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}),
Expand Down

0 comments on commit 79e52c7

Please sign in to comment.