From 9e1039f823243dd790cf35cd7afd53762dc00a25 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 5 Jul 2023 15:19:14 -0400 Subject: [PATCH] Enable attribute lookups via semantic model (#5536) ## Summary This PR enables us to resolve attribute accesses within files, at least for static and class methods. For example, we can now detect that this is a function access (and avoid a false-positive): ```python class Class: @staticmethod def error(): return ValueError("Something") # OK raise Class.error() ``` Closes #5487. Closes #5416. --- .../test/fixtures/flake8_raise/RSE102.py | 20 +++++++++++++ crates/ruff/src/checkers/ast/mod.rs | 28 +++++++++---------- crates/ruff/src/renamer.rs | 4 +-- .../unnecessary_paren_on_raise_exception.rs | 12 ++++++++ ...ry-paren-on-raise-exception_RSE102.py.snap | 6 ++-- crates/ruff_python_semantic/src/binding.rs | 14 +++++----- crates/ruff_python_semantic/src/model.rs | 28 ++++++++++++++++++- 7 files changed, 85 insertions(+), 27 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_raise/RSE102.py b/crates/ruff/resources/test/fixtures/flake8_raise/RSE102.py index aa80fa51452ac..ce75d7ab7c179 100644 --- a/crates/ruff/resources/test/fixtures/flake8_raise/RSE102.py +++ b/crates/ruff/resources/test/fixtures/flake8_raise/RSE102.py @@ -29,6 +29,26 @@ # Hello, world! ) +# OK raise AssertionError +# OK raise AttributeError("test message") + + +def return_error(): + return ValueError("Something") + + +# OK +raise return_error() + + +class Class: + @staticmethod + def error(): + return ValueError("Something") + + +# OK +raise Class.error() diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index a321aebbc63b1..7767129c5f5bd 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1781,7 +1781,6 @@ where match stmt { Stmt::FunctionDef(ast::StmtFunctionDef { body, - name, args, decorator_list, returns, @@ -1789,7 +1788,6 @@ where }) | Stmt::AsyncFunctionDef(ast::StmtAsyncFunctionDef { body, - name, args, decorator_list, returns, @@ -1848,13 +1846,6 @@ where }; } - self.add_binding( - name, - stmt.identifier(), - BindingKind::FunctionDefinition, - BindingFlags::empty(), - ); - let definition = docstrings::extraction::extract_definition( ExtractionTarget::Function, stmt, @@ -2064,19 +2055,28 @@ where // Post-visit. match stmt { - Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) => { - self.deferred.scopes.push(self.semantic.scope_id); + Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) + | Stmt::AsyncFunctionDef(ast::StmtAsyncFunctionDef { name, .. }) => { + let scope_id = self.semantic.scope_id; + self.deferred.scopes.push(scope_id); self.semantic.pop_scope(); self.semantic.pop_definition(); + self.add_binding( + name, + stmt.identifier(), + BindingKind::FunctionDefinition(scope_id), + BindingFlags::empty(), + ); } Stmt::ClassDef(ast::StmtClassDef { name, .. }) => { - self.deferred.scopes.push(self.semantic.scope_id); + let scope_id = self.semantic.scope_id; + self.deferred.scopes.push(scope_id); self.semantic.pop_scope(); self.semantic.pop_definition(); self.add_binding( name, stmt.identifier(), - BindingKind::ClassDefinition, + BindingKind::ClassDefinition(scope_id), BindingFlags::empty(), ); } @@ -3887,7 +3887,7 @@ where } // Store the existing binding, if any. - let existing_id = self.semantic.lookup(name); + let existing_id = self.semantic.lookup_symbol(name); // Add the bound exception name to the scope. let binding_id = self.add_binding( diff --git a/crates/ruff/src/renamer.rs b/crates/ruff/src/renamer.rs index a7beaafc5e56c..14aec66ef7ad9 100644 --- a/crates/ruff/src/renamer.rs +++ b/crates/ruff/src/renamer.rs @@ -248,8 +248,8 @@ impl Renamer { | BindingKind::LoopVar | BindingKind::Global | BindingKind::Nonlocal(_) - | BindingKind::ClassDefinition - | BindingKind::FunctionDefinition + | BindingKind::ClassDefinition(_) + | BindingKind::FunctionDefinition(_) | BindingKind::Deletion | BindingKind::UnboundException(_) => { Some(Edit::range_replacement(target.to_string(), binding.range)) diff --git a/crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs b/crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs index ee3b0d7e6960b..b9fbec46971e5 100644 --- a/crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs +++ b/crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs @@ -2,6 +2,7 @@ use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; + use ruff_python_ast::helpers::match_parens; use crate::checkers::ast::Checker; @@ -53,6 +54,17 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr: }) = expr { if args.is_empty() && keywords.is_empty() { + // `raise func()` still requires parentheses; only `raise Class()` does not. + if checker + .semantic() + .lookup_attribute(func) + .map_or(false, |id| { + checker.semantic().binding(id).kind.is_function_definition() + }) + { + return; + } + let range = match_parens(func.end(), checker.locator) .expect("Expected call to include parentheses"); let mut diagnostic = Diagnostic::new(UnnecessaryParenOnRaiseException, range); diff --git a/crates/ruff/src/rules/flake8_raise/snapshots/ruff__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap b/crates/ruff/src/rules/flake8_raise/snapshots/ruff__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap index 9d84196033a8f..4f905dd654c0a 100644 --- a/crates/ruff/src/rules/flake8_raise/snapshots/ruff__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap +++ b/crates/ruff/src/rules/flake8_raise/snapshots/ruff__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap @@ -118,7 +118,7 @@ RSE102.py:28:16: RSE102 [*] Unnecessary parentheses on raised exception 30 | | ) | |_^ RSE102 31 | -32 | raise AssertionError +32 | # OK | = help: Remove unnecessary parentheses @@ -131,7 +131,7 @@ RSE102.py:28:16: RSE102 [*] Unnecessary parentheses on raised exception 30 |-) 28 |+raise TypeError 31 29 | -32 30 | raise AssertionError -33 31 | +32 30 | # OK +33 31 | raise AssertionError diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 00d138494f213..862be2b617021 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -126,11 +126,11 @@ impl<'a> Binding<'a> { } matches!( existing.kind, - BindingKind::ClassDefinition - | BindingKind::FunctionDefinition - | BindingKind::Import(..) - | BindingKind::FromImport(..) - | BindingKind::SubmoduleImport(..) + BindingKind::ClassDefinition(_) + | BindingKind::FunctionDefinition(_) + | BindingKind::Import(_) + | BindingKind::FromImport(_) + | BindingKind::SubmoduleImport(_) ) } @@ -372,14 +372,14 @@ pub enum BindingKind<'a> { /// class Foo: /// ... /// ``` - ClassDefinition, + ClassDefinition(ScopeId), /// A binding for a function, like `foo` in: /// ```python /// def foo(): /// ... /// ``` - FunctionDefinition, + FunctionDefinition(ScopeId), /// A binding for an `__all__` export, like `__all__` in: /// ```python diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 619020ac12bbb..6773c973f202d 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -414,7 +414,7 @@ impl<'a> SemanticModel<'a> { /// Lookup a symbol in the current scope. This is a carbon copy of [`Self::resolve_read`], but /// doesn't add any read references to the resolved symbol. - pub fn lookup(&mut self, symbol: &str) -> Option { + pub fn lookup_symbol(&self, symbol: &str) -> Option { if self.in_forward_reference() { if let Some(binding_id) = self.scopes.global().get(symbol) { if !self.bindings[binding_id].is_unbound() { @@ -456,6 +456,32 @@ impl<'a> SemanticModel<'a> { None } + /// Lookup a qualified attribute in the current scope. + /// + /// For example, given `["Class", "method"`], resolve the `BindingKind::ClassDefinition` + /// associated with `Class`, then the `BindingKind::FunctionDefinition` associated with + /// `Class#method`. + pub fn lookup_attribute(&'a self, value: &'a Expr) -> Option { + let call_path = collect_call_path(value)?; + + // Find the symbol in the current scope. + let (symbol, attribute) = call_path.split_first()?; + let mut binding_id = self.lookup_symbol(symbol)?; + + // Recursively resolve class attributes, e.g., `foo.bar.baz` in. + let mut tail = attribute; + while let Some((symbol, rest)) = tail.split_first() { + // Find the next symbol in the class scope. + let BindingKind::ClassDefinition(scope_id) = self.binding(binding_id).kind else { + return None; + }; + binding_id = self.scopes[scope_id].get(symbol)?; + tail = rest; + } + + Some(binding_id) + } + /// Given a `BindingId`, return the `BindingId` of the submodule import that it aliases. fn resolve_submodule( &self,