From 455ed346fc4b42a2fbfbb371d9f01bd0a46bac15 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 5 Oct 2022 20:58:22 -0500 Subject: [PATCH 1/7] support pep593 annotations --- resources/test/fixtures/F821.py | 12 ++++++++++++ src/check_ast.rs | 9 ++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/resources/test/fixtures/F821.py b/resources/test/fixtures/F821.py index 7d4d60acc811e..277acdd368cca 100644 --- a/resources/test/fixtures/F821.py +++ b/resources/test/fixtures/F821.py @@ -89,3 +89,15 @@ def update_tomato(): f'B' f'{B}' ) + +import sys +if sys.version_info >= (3, 9): + from typing import Annotated +else: + from typing_extensions import Annotated + +def arbitrary_callable() -> None: + ... + +class PEP593Test: + field: Annotated[int, "base64", arbitrary_callable()] diff --git a/src/check_ast.rs b/src/check_ast.rs index e2f214ad83bf8..05e162b4f41a0 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -59,6 +59,7 @@ pub struct Checker<'a> { in_f_string: Option, in_annotation: bool, in_literal: bool, + in_pep593_annotated: bool, seen_non_import: bool, seen_docstring: bool, futures_allowed: bool, @@ -91,6 +92,7 @@ impl<'a> Checker<'a> { in_f_string: None, in_annotation: false, in_literal: false, + in_pep593_annotated: false, seen_non_import: false, seen_docstring: false, futures_allowed: true, @@ -675,6 +677,7 @@ where let prev_in_f_string = self.in_f_string; let prev_in_literal = self.in_literal; let prev_in_annotation = self.in_annotation; + let prev_in_pep593_annotated = self.in_pep593_annotated; if self.in_annotation && self.annotations_future_enabled { self.deferred_annotations.push(( @@ -692,6 +695,9 @@ where if match_name_or_attr(value, "Literal") { self.in_literal = true; } + if match_name_or_attr(value, "Annotated") { + self.in_pep593_annotated = true; + } } ExprKind::Tuple { elts, ctx } | ExprKind::List { elts, ctx } => { if matches!(ctx, ExprContext::Store) { @@ -862,7 +868,7 @@ where ExprKind::Constant { value: Constant::Str(value), .. - } if self.in_annotation && !self.in_literal => { + } if self.in_annotation && !(self.in_literal || self.in_pep593_annotated) => { self.deferred_string_annotations .push((Range::from_located(expr), value)); } @@ -1039,6 +1045,7 @@ where self.in_annotation = prev_in_annotation; self.in_literal = prev_in_literal; + self.in_pep593_annotated = prev_in_pep593_annotated; self.in_f_string = prev_in_f_string; } From 8778a79a42637af03cd7e9eef4e1f681129cbc1e Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 5 Oct 2022 21:01:32 -0500 Subject: [PATCH 2/7] Remove condition since code is not run --- resources/test/fixtures/F821.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/resources/test/fixtures/F821.py b/resources/test/fixtures/F821.py index 277acdd368cca..6900b6a843bdc 100644 --- a/resources/test/fixtures/F821.py +++ b/resources/test/fixtures/F821.py @@ -90,11 +90,9 @@ def update_tomato(): f'{B}' ) -import sys -if sys.version_info >= (3, 9): - from typing import Annotated -else: - from typing_extensions import Annotated + +from typing import Annotated + def arbitrary_callable() -> None: ... From 62df5363753d6c9905a966e9d4dec556b32fd0e3 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 6 Oct 2022 01:49:48 -0500 Subject: [PATCH 3/7] handle more complex cases --- resources/test/fixtures/F821.py | 27 ++++++- src/check_ast.rs | 74 +++++++++++++++----- src/python/typing.rs | 5 ++ src/snapshots/ruff__linter__tests__f821.snap | 27 +++++++ 4 files changed, 115 insertions(+), 18 deletions(-) diff --git a/resources/test/fixtures/F821.py b/resources/test/fixtures/F821.py index 6900b6a843bdc..e7500ef24e542 100644 --- a/resources/test/fixtures/F821.py +++ b/resources/test/fixtures/F821.py @@ -91,11 +91,34 @@ def update_tomato(): ) -from typing import Annotated +from typing import Annotated, Literal # noqa: E402 def arbitrary_callable() -> None: ... + class PEP593Test: - field: Annotated[int, "base64", arbitrary_callable()] + field: Annotated[ + int, + "base64", + arbitrary_callable(), + 123, + (1, 2, 3), + ] + field_with_stringified_type: Annotated[ + "PEP593Test", + 123, + ] + field_with_undefined_stringified_type: Annotated[ + "PEP593Test123", + 123, + ] + field_with_nested_subscript: Annotated[ + dict[Literal["foo"], str], + 123, + ] + field_with_undefined_nested_subscript: Annotated[ + dict["foo", "bar"], # these should fail as undefined + 123, + ] diff --git a/src/check_ast.rs b/src/check_ast.rs index 05e162b4f41a0..fb4b07136c2da 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -110,11 +110,33 @@ fn match_name_or_attr(expr: &Expr, target: &str) -> bool { } } -fn is_annotated_subscript(expr: &Expr) -> bool { +#[derive(Clone, Copy)] +pub enum SubscriptKind { + AnnotatedSubscript, + PEP593AnnotatedSubscript, +} + +fn match_annotated_subscript(expr: &Expr) -> Option { match &expr.node { - ExprKind::Attribute { attr, .. } => typing::is_annotated_subscript(attr), - ExprKind::Name { id, .. } => typing::is_annotated_subscript(id), - _ => false, + ExprKind::Attribute { attr, .. } => { + if typing::is_annotated_subscript(attr) { + Some(SubscriptKind::AnnotatedSubscript) + } else if typing::is_pep593_annotated_subscript(attr) { + Some(SubscriptKind::PEP593AnnotatedSubscript) + } else { + None + } + } + ExprKind::Name { id, .. } => { + if typing::is_annotated_subscript(id) { + Some(SubscriptKind::AnnotatedSubscript) + } else if typing::is_pep593_annotated_subscript(id) { + Some(SubscriptKind::PEP593AnnotatedSubscript) + } else { + None + } + } + _ => None, } } @@ -695,9 +717,6 @@ where if match_name_or_attr(value, "Literal") { self.in_literal = true; } - if match_name_or_attr(value, "Annotated") { - self.in_pep593_annotated = true; - } } ExprKind::Tuple { elts, ctx } | ExprKind::List { elts, ctx } => { if matches!(ctx, ExprContext::Store) { @@ -868,9 +887,11 @@ where ExprKind::Constant { value: Constant::Str(value), .. - } if self.in_annotation && !(self.in_literal || self.in_pep593_annotated) => { - self.deferred_string_annotations - .push((Range::from_located(expr), value)); + } => { + if self.in_annotation && !(self.in_literal || self.in_pep593_annotated) { + self.deferred_string_annotations + .push((Range::from_located(expr), value)); + } } ExprKind::Lambda { args, .. } => { // Visit the arguments, but avoid the body, which will be deferred. @@ -1021,12 +1042,33 @@ where } } ExprKind::Subscript { value, slice, ctx } => { - if is_annotated_subscript(value) { - self.visit_expr(value); - self.visit_annotation(slice); - self.visit_expr_context(ctx); - } else { - visitor::walk_expr(self, expr); + match match_annotated_subscript(value) { + Some(subscript) => match subscript { + SubscriptKind::AnnotatedSubscript => { + self.visit_expr(value); + self.visit_annotation(slice); + self.visit_expr_context(ctx); + } + SubscriptKind::PEP593AnnotatedSubscript => { + // the first argument is a type (including forward references) + // the rest of the arguments are arbitrary python objects + self.visit_expr(value); + match &slice.node { + ExprKind::Tuple { elts, ctx } => { + let first = elts.first().unwrap(); + self.visit_expr(first); + self.in_annotation = false; + for nxt in elts.iter().skip(1) { + self.visit_expr(nxt); + } + self.in_annotation = true; + self.visit_expr_context(ctx); + } + _ => panic!(), // arguments can only be slices + }; + } + }, + None => visitor::walk_expr(self, expr), } } _ => visitor::walk_expr(self, expr), diff --git a/src/python/typing.rs b/src/python/typing.rs index 7ea9d464ffee5..171d5509e1513 100644 --- a/src/python/typing.rs +++ b/src/python/typing.rs @@ -7,6 +7,7 @@ static ANNOTATED_SUBSCRIPTS: Lazy> = Lazy::new(|| { "AbstractAsyncContextManager", "AbstractContextManager", "AbstractSet", + // "Annotated", "AsyncContextManager", "AsyncGenerator", "AsyncIterable", @@ -87,3 +88,7 @@ static ANNOTATED_SUBSCRIPTS: Lazy> = Lazy::new(|| { pub fn is_annotated_subscript(name: &str) -> bool { ANNOTATED_SUBSCRIPTS.contains(name) } + +pub fn is_pep593_annotated_subscript(name: &str) -> bool { + name == "Annotated" +} diff --git a/src/snapshots/ruff__linter__tests__f821.snap b/src/snapshots/ruff__linter__tests__f821.snap index cc94c3fa3440a..fa5c0624b4fd4 100644 --- a/src/snapshots/ruff__linter__tests__f821.snap +++ b/src/snapshots/ruff__linter__tests__f821.snap @@ -74,4 +74,31 @@ expression: checks row: 89 column: 9 fix: ~ +- kind: + UndefinedName: PEP593Test123 + location: + row: 114 + column: 10 + end_location: + row: 114 + column: 24 + fix: ~ +- kind: + UndefinedName: foo + location: + row: 122 + column: 15 + end_location: + row: 122 + column: 19 + fix: ~ +- kind: + UndefinedName: bar + location: + row: 122 + column: 22 + end_location: + row: 122 + column: 26 + fix: ~ From 4f819070f5d55a97939f1a71c4323851660778eb Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 6 Oct 2022 01:52:44 -0500 Subject: [PATCH 4/7] minimize changes --- src/check_ast.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/check_ast.rs b/src/check_ast.rs index fb4b07136c2da..fc79076a7c1cc 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -59,7 +59,6 @@ pub struct Checker<'a> { in_f_string: Option, in_annotation: bool, in_literal: bool, - in_pep593_annotated: bool, seen_non_import: bool, seen_docstring: bool, futures_allowed: bool, @@ -92,7 +91,6 @@ impl<'a> Checker<'a> { in_f_string: None, in_annotation: false, in_literal: false, - in_pep593_annotated: false, seen_non_import: false, seen_docstring: false, futures_allowed: true, @@ -699,7 +697,6 @@ where let prev_in_f_string = self.in_f_string; let prev_in_literal = self.in_literal; let prev_in_annotation = self.in_annotation; - let prev_in_pep593_annotated = self.in_pep593_annotated; if self.in_annotation && self.annotations_future_enabled { self.deferred_annotations.push(( @@ -888,7 +885,7 @@ where value: Constant::Str(value), .. } => { - if self.in_annotation && !(self.in_literal || self.in_pep593_annotated) { + if self.in_annotation && !self.in_literal { self.deferred_string_annotations .push((Range::from_located(expr), value)); } @@ -1087,7 +1084,6 @@ where self.in_annotation = prev_in_annotation; self.in_literal = prev_in_literal; - self.in_pep593_annotated = prev_in_pep593_annotated; self.in_f_string = prev_in_f_string; } From 5d61ae0eccd93d0f5793426933736960a2640f6e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 6 Oct 2022 09:13:04 -0400 Subject: [PATCH 5/7] Minor tweaks --- src/check_ast.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/check_ast.rs b/src/check_ast.rs index fc79076a7c1cc..0d38a3716ff35 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -109,7 +109,7 @@ fn match_name_or_attr(expr: &Expr, target: &str) -> bool { } #[derive(Clone, Copy)] -pub enum SubscriptKind { +enum SubscriptKind { AnnotatedSubscript, PEP593AnnotatedSubscript, } @@ -1041,28 +1041,30 @@ where ExprKind::Subscript { value, slice, ctx } => { match match_annotated_subscript(value) { Some(subscript) => match subscript { + // Ex) Optional[int] SubscriptKind::AnnotatedSubscript => { self.visit_expr(value); self.visit_annotation(slice); self.visit_expr_context(ctx); } + // Ex) Annotated[int, "Hello, world!"] SubscriptKind::PEP593AnnotatedSubscript => { - // the first argument is a type (including forward references) - // the rest of the arguments are arbitrary python objects + // First argument is a type (including forward references); the rest are + // arbitrary Python objects. self.visit_expr(value); - match &slice.node { - ExprKind::Tuple { elts, ctx } => { - let first = elts.first().unwrap(); - self.visit_expr(first); + if let ExprKind::Tuple { elts, ctx } = &slice.node { + if let Some(expr) = elts.first() { + self.visit_expr(expr); self.in_annotation = false; - for nxt in elts.iter().skip(1) { - self.visit_expr(nxt); + for expr in elts.iter().skip(1) { + self.visit_expr(expr); } self.in_annotation = true; self.visit_expr_context(ctx); } - _ => panic!(), // arguments can only be slices - }; + } else { + error!("Found non-ExprKind::Tuple argument to PEP 593 Annotation.") + } } }, None => visitor::walk_expr(self, expr), From def8174afd144564f0f04a0b046b0894d23a5da7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 6 Oct 2022 09:14:17 -0400 Subject: [PATCH 6/7] Tweak test case --- resources/test/fixtures/F821.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/resources/test/fixtures/F821.py b/resources/test/fixtures/F821.py index e7500ef24e542..7adf0684abb12 100644 --- a/resources/test/fixtures/F821.py +++ b/resources/test/fixtures/F821.py @@ -91,7 +91,7 @@ def update_tomato(): ) -from typing import Annotated, Literal # noqa: E402 +from typing import Annotated, Literal def arbitrary_callable() -> None: @@ -119,6 +119,6 @@ class PEP593Test: 123, ] field_with_undefined_nested_subscript: Annotated[ - dict["foo", "bar"], # these should fail as undefined + dict["foo", "bar"], # Expected to fail as undefined. 123, ] From 7f94aeac49601989561ab7660bb3277e98f02811 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 6 Oct 2022 09:14:43 -0400 Subject: [PATCH 7/7] Remove derive --- src/check_ast.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/check_ast.rs b/src/check_ast.rs index 0d38a3716ff35..28950245c9163 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -108,7 +108,6 @@ fn match_name_or_attr(expr: &Expr, target: &str) -> bool { } } -#[derive(Clone, Copy)] enum SubscriptKind { AnnotatedSubscript, PEP593AnnotatedSubscript,