From 490301f9fe20f70b9ef62d30a649d329786d78cf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 12 May 2023 17:05:59 -0400 Subject: [PATCH] Replace `macro_rules!` visitors with dedicated methods (#4402) --- crates/ruff/src/checkers/ast/mod.rs | 166 ++++++++++++++-------------- 1 file changed, 80 insertions(+), 86 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 0b80e9c1c4..3686799e28 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -137,48 +137,6 @@ impl<'a> Checker<'a> { } } -/// Visit an body of [`Stmt`] nodes within a type-checking block. -macro_rules! visit_type_checking_block { - ($self:ident, $body:expr) => {{ - let snapshot = $self.ctx.flags; - $self.ctx.flags |= ContextFlags::TYPE_CHECKING_BLOCK; - $self.visit_body($body); - $self.ctx.flags = snapshot; - }}; -} - -/// Visit an [`Expr`], and treat it as a type definition. -macro_rules! visit_type_definition { - ($self:ident, $expr:expr) => {{ - let snapshot = $self.ctx.flags; - $self.ctx.flags |= ContextFlags::TYPE_DEFINITION; - $self.visit_expr($expr); - $self.ctx.flags = snapshot; - }}; -} - -/// Visit an [`Expr`], and treat it as _not_ a type definition. -macro_rules! visit_non_type_definition { - ($self:ident, $expr:expr) => {{ - let snapshot = $self.ctx.flags; - $self.ctx.flags -= ContextFlags::TYPE_DEFINITION; - $self.visit_expr($expr); - $self.ctx.flags = snapshot; - }}; -} - -/// Visit an [`Expr`], and treat it as a boolean test. This is useful for detecting whether an -/// expressions return value is significant, or whether the calling context only relies on -/// its truthiness. -macro_rules! visit_boolean_test { - ($self:ident, $expr:expr) => {{ - let snapshot = $self.ctx.flags; - $self.ctx.flags |= ContextFlags::BOOLEAN_TEST; - $self.visit_expr($expr); - $self.ctx.flags = snapshot; - }}; -} - impl<'a, 'b> Visitor<'b> for Checker<'a> where 'b: 'a, @@ -1919,7 +1877,7 @@ where for arg in &args.posonlyargs { if let Some(expr) = &arg.node.annotation { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -1928,7 +1886,7 @@ where for arg in &args.args { if let Some(expr) = &arg.node.annotation { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -1937,7 +1895,7 @@ where if let Some(arg) = &args.vararg { if let Some(expr) = &arg.node.annotation { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -1946,7 +1904,7 @@ where for arg in &args.kwonlyargs { if let Some(expr) = &arg.node.annotation { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -1955,7 +1913,7 @@ where if let Some(arg) = &args.kwarg { if let Some(expr) = &arg.node.annotation { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -1963,7 +1921,7 @@ where } for expr in returns { if runtime_annotation { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_annotation(expr); }; @@ -2170,13 +2128,13 @@ where }; if runtime_annotation { - visit_type_definition!(self, annotation); + self.visit_type_definition(annotation); } else { self.visit_annotation(annotation); } if let Some(expr) = value { if self.ctx.match_typing_expr(annotation, "TypeAlias") { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } else { self.visit_expr(expr); } @@ -2184,25 +2142,25 @@ where self.visit_expr(target); } StmtKind::Assert(ast::StmtAssert { test, msg }) => { - visit_boolean_test!(self, test); + self.visit_boolean_test(test); if let Some(expr) = msg { self.visit_expr(expr); } } StmtKind::While(ast::StmtWhile { test, body, orelse }) => { - visit_boolean_test!(self, test); + self.visit_boolean_test(test); self.visit_body(body); self.visit_body(orelse); } StmtKind::If(ast::StmtIf { test, body, orelse }) => { - visit_boolean_test!(self, test); + self.visit_boolean_test(test); if flake8_type_checking::helpers::is_type_checking_block(&self.ctx, test) { if self.settings.rules.enabled(Rule::EmptyTypeCheckingBlock) { flake8_type_checking::rules::empty_type_checking_block(self, stmt, body); } - visit_type_checking_block!(self, body); + self.visit_type_checking_block(body); } else { self.visit_body(body); } @@ -2245,7 +2203,7 @@ where fn visit_annotation(&mut self, expr: &'b Expr) { let flags_snapshot = self.ctx.flags; self.ctx.flags |= ContextFlags::ANNOTATION; - visit_type_definition!(self, expr); + self.visit_type_definition(expr); self.ctx.flags = flags_snapshot; } @@ -3663,7 +3621,7 @@ where self.deferred.lambdas.push((expr, self.ctx.snapshot())); } ExprKind::IfExp(ast::ExprIfExp { test, body, orelse }) => { - visit_boolean_test!(self, test); + self.visit_boolean_test(test); self.visit_expr(body); self.visit_expr(orelse); } @@ -3705,7 +3663,7 @@ where Some(Callable::Bool) => { self.visit_expr(func); if !args.is_empty() { - visit_boolean_test!(self, &args[0]); + self.visit_boolean_test(&args[0]); } for expr in args.iter().skip(1) { self.visit_expr(expr); @@ -3714,7 +3672,7 @@ where Some(Callable::Cast) => { self.visit_expr(func); if !args.is_empty() { - visit_type_definition!(self, &args[0]); + self.visit_type_definition(&args[0]); } for expr in args.iter().skip(1) { self.visit_expr(expr); @@ -3723,21 +3681,21 @@ where Some(Callable::NewType) => { self.visit_expr(func); for expr in args.iter().skip(1) { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } } Some(Callable::TypeVar) => { self.visit_expr(func); for expr in args.iter().skip(1) { - visit_type_definition!(self, expr); + self.visit_type_definition(expr); } for keyword in keywords { let KeywordData { arg, value } = &keyword.node; if let Some(id) = arg { if id == "bound" { - visit_type_definition!(self, value); + self.visit_type_definition(value); } else { - visit_non_type_definition!(self, value); + self.visit_non_type_definition(value); } } } @@ -3755,8 +3713,8 @@ where ExprKind::List(ast::ExprList { elts, .. }) | ExprKind::Tuple(ast::ExprTuple { elts, .. }) => { if elts.len() == 2 { - visit_non_type_definition!(self, &elts[0]); - visit_type_definition!(self, &elts[1]); + self.visit_non_type_definition(&elts[0]); + self.visit_type_definition(&elts[1]); } } _ => {} @@ -3770,7 +3728,7 @@ where // Ex) NamedTuple("a", a=int) for keyword in keywords { let KeywordData { value, .. } = &keyword.node; - visit_type_definition!(self, value); + self.visit_type_definition(value); } } Some(Callable::TypedDict) => { @@ -3780,10 +3738,10 @@ where if args.len() > 1 { if let ExprKind::Dict(ast::ExprDict { keys, values }) = &args[1].node { for key in keys.iter().flatten() { - visit_non_type_definition!(self, key); + self.visit_non_type_definition(key); } for value in values { - visit_type_definition!(self, value); + self.visit_type_definition(value); } } } @@ -3791,7 +3749,7 @@ where // Ex) TypedDict("a", a=int) for keyword in keywords { let KeywordData { value, .. } = &keyword.node; - visit_type_definition!(self, value); + self.visit_type_definition(value); } } Some(Callable::MypyExtension) => { @@ -3799,23 +3757,23 @@ where if let Some(arg) = args.first() { // Ex) DefaultNamedArg(bool | None, name="some_prop_name") - visit_type_definition!(self, arg); + self.visit_type_definition(arg); for arg in args.iter().skip(1) { - visit_non_type_definition!(self, arg); + self.visit_non_type_definition(arg); } for keyword in keywords { let KeywordData { value, .. } = &keyword.node; - visit_non_type_definition!(self, value); + self.visit_non_type_definition(value); } } else { // Ex) DefaultNamedArg(type="bool", name="some_prop_name") for keyword in keywords { let KeywordData { value, arg } = &keyword.node; if arg.as_ref().map_or(false, |arg| arg == "type") { - visit_type_definition!(self, value); + self.visit_type_definition(value); } else { - visit_non_type_definition!(self, value); + self.visit_non_type_definition(value); } } } @@ -3826,11 +3784,11 @@ where // any strings as deferred type definitions). self.visit_expr(func); for arg in args { - visit_non_type_definition!(self, arg); + self.visit_non_type_definition(arg); } for keyword in keywords { let KeywordData { value, .. } = &keyword.node; - visit_non_type_definition!(self, value); + self.visit_non_type_definition(value); } } } @@ -3856,7 +3814,7 @@ where // Ex) Optional[int] SubscriptKind::AnnotatedSubscript => { self.visit_expr(value); - visit_type_definition!(self, slice); + self.visit_type_definition(slice); self.visit_expr_context(ctx); } // Ex) Annotated[int, "Hello, world!"] @@ -3870,7 +3828,7 @@ where if let Some(expr) = elts.first() { self.visit_expr(expr); for expr in elts.iter().skip(1) { - visit_non_type_definition!(self, expr); + self.visit_non_type_definition(expr); } self.visit_expr_context(ctx); } @@ -3921,7 +3879,7 @@ where self.visit_expr(&comprehension.iter); self.visit_expr(&comprehension.target); for expr in &comprehension.ifs { - visit_boolean_test!(self, expr); + self.visit_boolean_test(expr); } } @@ -4220,6 +4178,50 @@ where } impl<'a> Checker<'a> { + /// Visit a [`Module`]. Returns `true` if the module contains a module-level docstring. + fn visit_module(&mut self, python_ast: &'a Suite) -> bool { + if self.settings.rules.enabled(Rule::FStringDocstring) { + flake8_bugbear::rules::f_string_docstring(self, python_ast); + } + let docstring = docstrings::extraction::docstring_from(python_ast); + docstring.is_some() + } + + /// Visit an body of [`Stmt`] nodes within a type-checking block. + fn visit_type_checking_block(&mut self, body: &'a [Stmt]) { + let snapshot = self.ctx.flags; + self.ctx.flags |= ContextFlags::TYPE_CHECKING_BLOCK; + self.visit_body(body); + self.ctx.flags = snapshot; + } + + /// Visit an [`Expr`], and treat it as a type definition. + pub fn visit_type_definition(&mut self, expr: &'a Expr) { + let snapshot = self.ctx.flags; + self.ctx.flags |= ContextFlags::TYPE_DEFINITION; + self.visit_expr(expr); + self.ctx.flags = snapshot; + } + + /// Visit an [`Expr`], and treat it as _not_ a type definition. + pub fn visit_non_type_definition(&mut self, expr: &'a Expr) { + let snapshot = self.ctx.flags; + self.ctx.flags -= ContextFlags::TYPE_DEFINITION; + self.visit_expr(expr); + self.ctx.flags = snapshot; + } + + /// Visit an [`Expr`], and treat it as a boolean test. This is useful for detecting whether an + /// expressions return value is significant, or whether the calling context only relies on + /// its truthiness. + pub fn visit_boolean_test(&mut self, expr: &'a Expr) { + let snapshot = self.ctx.flags; + self.ctx.flags |= ContextFlags::BOOLEAN_TEST; + self.visit_expr(expr); + self.ctx.flags = snapshot; + } + + /// Add a [`Binding`] to the current scope, bound to the given name. fn add_binding(&mut self, name: &'a str, binding: Binding<'a>) { let binding_id = self.ctx.bindings.next_id(); if let Some((stack_index, existing_binding_id)) = self @@ -4790,14 +4792,6 @@ impl<'a> Checker<'a> { )); } - fn visit_module(&mut self, python_ast: &'a Suite) -> bool { - if self.settings.rules.enabled(Rule::FStringDocstring) { - flake8_bugbear::rules::f_string_docstring(self, python_ast); - } - let docstring = docstrings::extraction::docstring_from(python_ast); - docstring.is_some() - } - fn check_deferred_future_type_definitions(&mut self) { while !self.deferred.future_type_definitions.is_empty() { let type_definitions = std::mem::take(&mut self.deferred.future_type_definitions);