Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule: Auto-add -> None typehint to functions #8212

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF300.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
from typing import NoReturn


def valid1(): # OK
return 123


def valid2() -> int: # OK
return 123


def valid3() -> None: # OK
return


def valid4(): # OK
yield 123
return


def valid5(): # OK
raise NotImplementedError()


def valid5() -> NoReturn: # OK
raise ValueError()


def invalid1(): # RUF300
return None


def invalid2(): # RUF300
print()


def wrong_annotation() -> int: # RUF300
print()


def invalid3(): # RUF300
return (None)


async def invalid4(): # RUF300
return # TODO is this correct?
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
]) {
flake8_return::rules::function(checker, body, returns.as_ref().map(AsRef::as_ref));
}
if checker.enabled(Rule::FunctionReturnHintNone) {
ruff::rules::function_return_hint_none(checker, function_def, body, returns.as_ref().map(AsRef::as_ref));
}
if checker.enabled(Rule::UselessReturn) {
pylint::rules::useless_return(
checker,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "019") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryKeyCheck),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
(Ruff, "300") => (RuleGroup::Preview, rules::ruff::rules::FunctionReturnHintNone),

// flake8-django
(Flake8Django, "001") => (RuleGroup::Stable, rules::flake8_django::rules::DjangoNullableModelStringField),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_return/rules/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use super::super::branch::Branch;
use super::super::helpers::result_exists;
use super::super::visitor::{ReturnVisitor, Stack};

// CHECK HERE!

/// ## What it does
/// Checks for the presence of a `return None` statement when `None` is the only
/// possible return value.
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ mod tests {
#[test_case(Rule::QuadraticListSummation, Path::new("RUF017_0.py"))]
#[test_case(Rule::AssignmentInAssert, Path::new("RUF018.py"))]
#[test_case(Rule::UnnecessaryKeyCheck, Path::new("RUF019.py"))]
#[test_case(Rule::FunctionReturnHintNone, Path::new("RUF300.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
106 changes: 106 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/function_return_hint_none.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::helpers::{is_const_none, ReturnStatementVisitor};
use ruff_python_ast::statement_visitor::StatementVisitor;
use ruff_python_ast::{Expr, Stmt};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
///
/// ## Why is this bad?
///
/// ## Examples
/// ```python
/// ```
///
/// Use instead:
/// ```python
/// ```
#[violation]
pub struct FunctionReturnHintNone;
// TODO naming convention

impl AlwaysFixableViolation for FunctionReturnHintNone {
#[derive_message_formats]
fn message(&self) -> String {
format!("Function should use `-> None` return type hint")
}

fn fix_title(&self) -> String {
format!("Replace with `-> None`")
}
}

/// Return `true` if a function's return statement include at least one
/// non-`None` value.
// FIXME copy-paste
pub(super) fn result_exists(returns: &[&ast::StmtReturn]) -> bool {
returns.iter().any(|stmt| {
stmt.value.as_deref().is_some_and(|value| {
!matches!(
value,
Expr::Constant(constant) if constant.value.is_none()
)
})
})
}

/// RUF300
pub(crate) fn function_return_hint_none(
checker: &mut Checker,
function_def: &ast::StmtFunctionDef,
body: &[Stmt],
returns: Option<&Expr>,
) {
// Already hinted `-> None`, ignore
if returns.map_or(false, is_const_none) {
return;
}

// Find the last statement in the function.
// let Some(last_stmt) = body.last() else {
// Skip empty functions.
// return;
// };

// Traverse the function body, to collect the stack.
let returns = {
let mut visitor = ReturnStatementVisitor::default();
visitor.visit_body(body);
visitor.returns
};

// TODO: Avoid false positives for generators.
// if stack.is_generator {
// return;
// }

// If we have at least one non-`None` return...
if result_exists(&returns) {
return;
}

let edit = edit_function_return_type(function_def, "None".to_string());
let mut diagnostic = Diagnostic::new(FunctionReturnHintNone, function_def.parameters.range);
// Mark as unsafe if we're *changing* an existing return type
diagnostic.set_fix(if function_def.returns.is_none() {
Fix::safe_edit(edit)
} else {
Fix::unsafe_edit(edit)
});
checker.diagnostics.push(diagnostic);
}

fn edit_function_return_type(function_def: &ast::StmtFunctionDef, new_type: String) -> Edit {
if let Some(returns) = &function_def.returns {
Edit::range_replacement(new_type, returns.range())
} else {
Edit::insertion(
format!(" -> {}", new_type),
function_def.parameters.range.end(),
)
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub(crate) use asyncio_dangling_task::*;
pub(crate) use collection_literal_concatenation::*;
pub(crate) use explicit_f_string_type_conversion::*;
pub(crate) use function_call_in_dataclass_default::*;
pub(crate) use function_return_hint_none::*;
pub(crate) use implicit_optional::*;
pub(crate) use invalid_index_type::*;
pub(crate) use invalid_pyproject_toml::*;
Expand All @@ -24,6 +25,7 @@ mod collection_literal_concatenation;
mod confusables;
mod explicit_f_string_type_conversion;
mod function_call_in_dataclass_default;
mod function_return_hint_none;
mod helpers;
mod implicit_optional;
mod invalid_index_type;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF300.py:16:11: RUF300 [*] Function should use `-> None` return type hint
|
16 | def valid4(): # OK
| ^^ RUF300
17 | yield 123
18 | return
|
= help: Replace with `-> None`

ℹ Fix
13 13 | return
14 14 |
15 15 |
16 |-def valid4(): # OK
16 |+def valid4() -> None: # OK
17 17 | yield 123
18 18 | return
19 19 |

RUF300.py:21:11: RUF300 [*] Function should use `-> None` return type hint
|
21 | def valid5(): # OK
| ^^ RUF300
22 | raise NotImplementedError()
|
= help: Replace with `-> None`

ℹ Fix
18 18 | return
19 19 |
20 20 |
21 |-def valid5(): # OK
21 |+def valid5() -> None: # OK
22 22 | raise NotImplementedError()
23 23 |
24 24 |

RUF300.py:25:11: RUF300 [*] Function should use `-> None` return type hint
|
25 | def valid5() -> NoReturn: # OK
| ^^ RUF300
26 | raise ValueError()
|
= help: Replace with `-> None`

ℹ Suggested fix
22 22 | raise NotImplementedError()
23 23 |
24 24 |
25 |-def valid5() -> NoReturn: # OK
25 |+def valid5() -> None: # OK
26 26 | raise ValueError()
27 27 |
28 28 |

RUF300.py:29:13: RUF300 [*] Function should use `-> None` return type hint
|
29 | def invalid1(): # RUF300
| ^^ RUF300
30 | return None
|
= help: Replace with `-> None`

ℹ Fix
26 26 | raise ValueError()
27 27 |
28 28 |
29 |-def invalid1(): # RUF300
29 |+def invalid1() -> None: # RUF300
30 30 | return None
31 31 |
32 32 |

RUF300.py:33:13: RUF300 [*] Function should use `-> None` return type hint
|
33 | def invalid2(): # RUF300
| ^^ RUF300
34 | print()
|
= help: Replace with `-> None`

ℹ Fix
30 30 | return None
31 31 |
32 32 |
33 |-def invalid2(): # RUF300
33 |+def invalid2() -> None: # RUF300
34 34 | print()
35 35 |
36 36 |

RUF300.py:37:21: RUF300 [*] Function should use `-> None` return type hint
|
37 | def wrong_annotation() -> int: # RUF300
| ^^ RUF300
38 | print()
|
= help: Replace with `-> None`

ℹ Suggested fix
34 34 | print()
35 35 |
36 36 |
37 |-def wrong_annotation() -> int: # RUF300
37 |+def wrong_annotation() -> None: # RUF300
38 38 | print()
39 39 |
40 40 |

RUF300.py:41:13: RUF300 [*] Function should use `-> None` return type hint
|
41 | def invalid3(): # RUF300
| ^^ RUF300
42 | return (None)
|
= help: Replace with `-> None`

ℹ Fix
38 38 | print()
39 39 |
40 40 |
41 |-def invalid3(): # RUF300
41 |+def invalid3() -> None: # RUF300
42 42 | return (None)
43 43 |
44 44 |

RUF300.py:45:19: RUF300 [*] Function should use `-> None` return type hint
|
45 | async def invalid4(): # RUF300
| ^^ RUF300
46 | return # TODO is this correct?
|
= help: Replace with `-> None`

ℹ Fix
42 42 | return (None)
43 43 |
44 44 |
45 |-async def invalid4(): # RUF300
45 |+async def invalid4() -> None: # RUF300
46 46 | return # TODO is this correct?


3 changes: 3 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading