Skip to content

Commit

Permalink
Avoid adding return types to stub methods (#9277)
Browse files Browse the repository at this point in the history 
We should avoid adding `-> None` to stubs in `.pyi` files, along with a
few other cases. (We already ignore abstract methods.)

Closes #9270.
  • Loading branch information
@charliermarsh
charliermarsh authored Dec 25, 2023
1 parent 8b70240 commit fa78d2d
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 55 deletions.
17 changes: 17 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
123 changes: 68 additions & 55 deletions crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -537,17 +536,41 @@ fn check_dynamically_typed<F>(
}
}

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`.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down

0 comments on commit fa78d2d

Please sign in to comment.