Autofix for none-not-at-end-of-union (RUF036)
#15139
Open
+331
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF036 | 472 | 0 | 82 | 390 | 0 |
harupy
commented
Dec 25, 2024
| applicability, | ||
| ) | ||
| }; | ||
| diagnostic.set_fix(fix); |
There was a problem hiding this comment.
Should we set the fix to all the diagnostics?
There was a problem hiding this comment.
Can you tell me a bit more why you think we should (or shouldn't) set the fix for all violations?
There was a problem hiding this comment.
@MichaReiser For example, this part looks like the same fix is set to all the diagnostics:
ruff/crates/ruff_linter/src/rules/flake8_pyi/rules/duplicate_union_member.rs
Lines 126 to 129 in 8b9c843
There was a problem hiding this comment.
I'm actually leaning towards only emitting a single diagnostic rather than emitting multiple diagnostics. This should also simplify the code a bit because we can change none_exprs to an Option only tracking the last_none rather than all None values
There was a problem hiding this comment.
A single diagnostic makes sense.
|
Thanks for working on this! Can you add some tests with nested and mixed uses of unions? Like these: def f(x: None | Union[None ,int]): ...
def g(x: int | (str | None) | list): ...
def h(x: Union[int, Union[None, list | set]]: ...also the examples from def _detect_active_python(io: None | IO = None) -> Path | None: ... |
|
It looks like in the ecosystem check a bunch of violations were removed with no fix, so I'm wondering if a syntax error was introduced by the fix. Maybe when there is a default argument something goes wrong? |
I think this happens for projects where the |
MichaReiser
reviewed
Dec 26, 2024
crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs
Outdated
Show resolved
Hide resolved
MichaReiser
requested changes
Dec 30, 2024
There was a problem hiding this comment.
Thanks for working on this.
The fix now seems to remove duplicate None elements and I think it also isn't handling nesting correctly - see the inline comments in the tests.
I tried to implement a fix that does more narrow edits than regenerating the entire union / subscript but the whitespace handling isn't perfect yet and I haven't looked into some edge cases like quoted annotations or parentheses. But maybe it's useful for you
Index: crates/ruff_python_parser/src/lib.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_python_parser/src/lib.rs b/crates/ruff_python_parser/src/lib.rs
--- a/crates/ruff_python_parser/src/lib.rs (revision aa1a5cb2c7ef0dc3b38595a92285066721e626c0)
+++ b/crates/ruff_python_parser/src/lib.rs (date 1735565508471)
@@ -478,6 +478,35 @@
}
}
}
+
+ /// Returns a slice of tokens before the given [`TextSize`] offset.
+ ///
+ /// If the given offset is between two tokens, the returned slice will start from the following
+ /// token. In other words, if the offset is between the end of previous token and start of next
+ /// token, the returned slice will start from the next token.
+ ///
+ /// # Panics
+ ///
+ /// If the given offset is inside a token range.
+ pub fn before(&self, offset: TextSize) -> &[Token] {
+ match self.binary_search_by(|token| token.start().cmp(&offset)) {
+ Ok(idx) => &self[..idx],
+ Err(idx) => {
+ if let Some(prev) = self.get(idx.saturating_sub(1)) {
+ // If it's equal to the end offset, then it's at a token boundary which is
+ // valid. If it's greater than the end offset, then it's in the gap between
+ // the tokens which is valid as well.
+ assert!(
+ offset >= prev.end(),
+ "Offset {:?} is inside a token range {:?}",
+ offset,
+ prev.range()
+ );
+ }
+ &self[..idx]
+ }
+ }
+ }
}
impl<'a> IntoIterator for &'a Tokens {
Index: crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs b/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs
--- a/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs (revision aa1a5cb2c7ef0dc3b38595a92285066721e626c0)
+++ b/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs (date 1735567068721)
@@ -1,13 +1,14 @@
+use crate::checkers::ast::Checker;
use itertools::{Itertools, Position};
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{self as ast, Expr};
+use ruff_python_parser::TokenKind;
use ruff_python_semantic::analyze::typing::traverse_union;
-use ruff_text_size::Ranged;
+use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
+use ruff_text_size::{Ranged, TextRange, TextSlice};
use smallvec::SmallVec;
-use crate::checkers::ast::Checker;
-
/// ## What it does
/// Checks for type annotations where `None` is not at the end of an union.
///
@@ -49,15 +50,13 @@
/// RUF036
pub(crate) fn none_not_at_end_of_union<'a>(checker: &mut Checker, union: &'a Expr) {
let semantic = checker.semantic();
- let mut none_exprs: SmallVec<[&Expr; 1]> = SmallVec::new();
- let mut non_none_exprs: SmallVec<[&Expr; 1]> = SmallVec::new();
- let mut last_expr: Option<&Expr> = None;
+ let mut last_none = None;
+ let mut last_expr: Option<&'a Expr> = None;
let mut find_none = |expr: &'a Expr, _parent: &Expr| {
- if matches!(expr, Expr::NoneLiteral(_)) {
- none_exprs.push(expr);
- } else {
- non_none_exprs.push(expr);
+ if expr.is_none_literal_expr() {
+ last_none = Some(expr);
}
+
last_expr = Some(expr);
};
@@ -69,40 +68,72 @@
};
// There must be at least one `None` expression.
- let Some(last_none) = none_exprs.last() else {
+ let Some(last_none) = last_none else {
return;
};
// If any of the `None` literals is last we do not emit.
- if *last_none == last_expr {
+ if last_none == last_expr {
return;
}
- for (pos, none_expr) in none_exprs.iter().with_position() {
- let mut diagnostic = Diagnostic::new(NoneNotAtEndOfUnion, none_expr.range());
- if matches!(pos, Position::Last | Position::Only) {
- let mut elements = non_none_exprs
- .iter()
- .map(|expr| checker.locator().slice(expr.range()).to_string())
- .chain(std::iter::once("None".to_string()));
- let (range, separator) =
- if let Expr::Subscript(ast::ExprSubscript { slice, .. }) = union {
- (slice.range(), ", ")
- } else {
- (union.range(), " | ")
- };
- let applicability = if checker.comment_ranges().intersects(range) {
- Applicability::Unsafe
- } else {
- Applicability::Safe
- };
- let fix = Fix::applicable_edit(
- Edit::range_replacement(elements.join(separator), range),
- applicability,
- );
- diagnostic.set_fix(fix);
- }
+ let range = if let Expr::Subscript(ast::ExprSubscript { slice, .. }) = union {
+ slice.range()
+ } else {
+ union.range()
+ };
+
+ let preceding_separator = checker.tokens().before(last_none.start());
+
+ let applicability = if checker.comment_ranges().intersects(range) {
+ Applicability::Unsafe
+ } else {
+ Applicability::Safe
+ };
+
+ // ```python
+ // int | None | str
+ // ```
+ let (insertion, range) = if let Some(last) = preceding_separator
+ .last()
+ .filter(|last| matches!(last.kind(), TokenKind::Comma | TokenKind::Vbar))
+ {
+ let range = TextRange::new(last.start(), last_none.end());
+ (
+ Edit::insertion(checker.source().slice(range).to_string(), last_expr.end()),
+ range,
+ )
+ } else {
+ // ```python
+ // None | int
+ // int | (None | str) | Literal[4]
+ // ```
+ let separator_after = checker
+ .tokens()
+ .after(last_none.end())
+ .first()
+ .filter(|token| matches!(token.kind(), TokenKind::Comma | TokenKind::Vbar))
+ .unwrap();
+ (
+ Edit::insertion(
+ format!(
+ "{separator} None",
+ separator = if separator_after.kind() == TokenKind::Comma {
+ ","
+ } else {
+ "|"
+ }
+ ),
+ last_expr.end(),
+ ),
+ TextRange::new(last_none.start(), separator_after.end()),
+ )
+ };
+
+ let fix = Fix::applicable_edits(insertion, [Edit::range_deletion(range)], applicability);
+
+ let mut diagnostic = Diagnostic::new(NoneNotAtEndOfUnion, last_none.range());
+ diagnostic.set_fix(fix);
- checker.diagnostics.push(diagnostic);
- }
+ checker.diagnostics.push(diagnostic);
}| applicability, | ||
| ) | ||
| }; | ||
| diagnostic.set_fix(fix); |
There was a problem hiding this comment.
I'm actually leaning towards only emitting a single diagnostic rather than emitting multiple diagnostics. This should also simplify the code a bit because we can change none_exprs to an Option only tracking the last_none rather than all None values
| 10 10 | | ||
| 11 11 | | ||
| 12 |-def func3(arg: None | None | int): | ||
| 12 |+def func3(arg: int | None): |
There was a problem hiding this comment.
We should avoid removing duplicate None values. While the second None is useless, it's not what the rule is about
Comment on lines
+207
to
+208
| 44 |-def func10(x: U[int, U[None, list | set]]): | ||
| 44 |+def func10(x: U[int, list, set, None]): |
There was a problem hiding this comment.
The fix here looks incorrect. It flattens the nested U types.
There was a problem hiding this comment.
@MichaReiser Do we need to fix this case? Can we only generate a fix when the type annotation has a single None and no nested unions?
There was a problem hiding this comment.
I don't think we necessarily have to but we at least shouldn't provide a fix for those cases.
dhruvmanila
added
rule
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Close #15136. Implement autofix for
none-not-at-end-of-union (RUF036)Test Plan
Existing and new tests