From 064a3ca81d3aed0fb2ccce1b2be4bea2360177a4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 25 Dec 2023 08:47:20 -0500 Subject: [PATCH] Avoid adding return types to stub methods --- .../flake8_annotations/auto_return_type.py | 17 +++ .../flake8_annotations/rules/definition.rs | 123 ++++++++++-------- 2 files changed, 85 insertions(+), 55 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py index a1df0985b0acd..267a4ac373f39 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py @@ -212,3 +212,20 @@ def func(x: int): raise ValueError else: return 1 + + +from typing import overload + + +@overload +def overloaded(i: int) -> "int": + ... + + +@overload +def overloaded(i: "str") -> "str": + ... + + +def overloaded(i): + return i 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 dee00d6667831..607d07cd5f82f 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -482,7 +482,6 @@ impl Violation for AnyType { format!("Dynamically typed expressions (typing.Any) are disallowed in `{name}`") } } - fn is_none_returning(body: &[Stmt]) -> bool { let mut visitor = ReturnStatementVisitor::default(); visitor.visit_body(body); @@ -537,17 +536,41 @@ fn check_dynamically_typed( } } -fn is_empty_body(body: &[Stmt]) -> bool { - body.iter().all(|stmt| match stmt { - Stmt::Pass(_) => true, - Stmt::Expr(ast::StmtExpr { value, range: _ }) => { - matches!( - value.as_ref(), - Expr::StringLiteral(_) | Expr::EllipsisLiteral(_) - ) +/// Return `true` if a function appears to be a stub. +fn is_stub_function(function_def: &ast::StmtFunctionDef, checker: &Checker) -> bool { + /// Returns `true` if a function has an empty body. + fn is_empty_body(function_def: &ast::StmtFunctionDef) -> bool { + function_def.body.iter().all(|stmt| match stmt { + Stmt::Pass(_) => true, + Stmt::Expr(ast::StmtExpr { value, range: _ }) => { + matches!( + value.as_ref(), + Expr::StringLiteral(_) | Expr::EllipsisLiteral(_) + ) + } + _ => false, + }) + } + + // Ignore functions with empty bodies in... + if is_empty_body(function_def) { + // Stub definitions (.pyi files)... + if checker.source_type.is_stub() { + return true; + } + + // Abstract methods... + if visibility::is_abstract(&function_def.decorator_list, checker.semantic()) { + return true; + } + + // Overload definitions... + if visibility::is_overload(&function_def.decorator_list, checker.semantic()) { + return true; } - _ => false, - }) + } + + false } /// Generate flake8-annotation checks for a given `Definition`. @@ -738,9 +761,7 @@ pub(crate) fn definition( ) { if is_method && visibility::is_classmethod(decorator_list, checker.semantic()) { if checker.enabled(Rule::MissingReturnTypeClassMethod) { - let return_type = if visibility::is_abstract(decorator_list, checker.semantic()) - && is_empty_body(body) - { + let return_type = if is_stub_function(function, checker) { None } else { auto_return_type(function) @@ -771,9 +792,7 @@ pub(crate) fn definition( } } else if is_method && visibility::is_staticmethod(decorator_list, checker.semantic()) { if checker.enabled(Rule::MissingReturnTypeStaticMethod) { - let return_type = if visibility::is_abstract(decorator_list, checker.semantic()) - && is_empty_body(body) - { + let return_type = if is_stub_function(function, checker) { None } else { auto_return_type(function) @@ -843,25 +862,22 @@ pub(crate) fn definition( match visibility { visibility::Visibility::Public => { if checker.enabled(Rule::MissingReturnTypeUndocumentedPublicFunction) { - let return_type = - if visibility::is_abstract(decorator_list, checker.semantic()) - && is_empty_body(body) - { - None - } else { - auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| { - (checker.generator().expr(&return_type), edits) - }) - }; + let return_type = if is_stub_function(function, checker) { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| { + (checker.generator().expr(&return_type), edits) + }) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypeUndocumentedPublicFunction { name: name.to_string(), @@ -885,25 +901,22 @@ pub(crate) fn definition( } visibility::Visibility::Private => { if checker.enabled(Rule::MissingReturnTypePrivateFunction) { - let return_type = - if visibility::is_abstract(decorator_list, checker.semantic()) - && is_empty_body(body) - { - None - } else { - auto_return_type(function) - .and_then(|return_type| { - return_type.into_expression( - checker.importer(), - function.parameters.start(), - checker.semantic(), - checker.settings.target_version, - ) - }) - .map(|(return_type, edits)| { - (checker.generator().expr(&return_type), edits) - }) - }; + let return_type = if is_stub_function(function, checker) { + None + } else { + auto_return_type(function) + .and_then(|return_type| { + return_type.into_expression( + checker.importer(), + function.parameters.start(), + checker.semantic(), + checker.settings.target_version, + ) + }) + .map(|(return_type, edits)| { + (checker.generator().expr(&return_type), edits) + }) + }; let mut diagnostic = Diagnostic::new( MissingReturnTypePrivateFunction { name: name.to_string(),