Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Consider the new f-string tokens for `flake8-commas` (#8582) 

## Summary

This fixes the bug where the `flake8-commas` rules weren't taking the
new f-string tokens into account.

## Test Plan

Add new test cases around f-strings for all of `flake8-commas`'s rules.

fixes: #8556
This commit is contained in:
Dhruv Manilawala 2023-11-10 09:49:14 +05:30 committed by GitHub
parent 7968e190dd
commit d5606b7705
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 137 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py vendored
View File

@ -639,3 +639,18 @@ foo = namedtuple(
:20
],
)
# F-strings
kwargs.pop("remove", f"this {trailing_comma}",)
raise Exception(
"first", extra=f"Add trailing comma here ->"
)
assert False, f"<- This is not a trailing comma"
f"""This is a test. {
"Another sentence."
if True else
"Don't add a trailing comma here ->"
}"""

2
crates/ruff_linter/src/checkers/tokens.rs
View File

@ -141,7 +141,7 @@ pub(crate) fn check_tokens(
Rule::TrailingCommaOnBareTuple,
Rule::ProhibitedTrailingComma,
]) {
flake8_commas::rules::trailing_commas(&mut diagnostics, tokens, locator);
flake8_commas::rules::trailing_commas(&mut diagnostics, tokens, locator, indexer);
}
if settings.rules.enabled(Rule::ExtraneousParentheses) {

125
crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs
View File

@ -3,6 +3,7 @@ use itertools::Itertools;
use ruff_diagnostics::{AlwaysFixableViolation, Violation};
use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_index::Indexer;
use ruff_python_parser::lexer::{LexResult, Spanned};
use ruff_python_parser::Tok;
use ruff_source_file::Locator;
@ -29,22 +30,33 @@ enum TokenType {
/// Simplified token specialized for the task.
#[derive(Copy, Clone)]
struct Token<'tok> {
type_: TokenType,
// Underlying token.
spanned: Option<&'tok Spanned>,
struct Token {
r#type: TokenType,
range: TextRange,
}
impl<'tok> Token<'tok> {
const fn irrelevant() -> Token<'static> {
Token {
type_: TokenType::Irrelevant,
spanned: None,
impl Ranged for Token {
fn range(&self) -> TextRange {
self.range
}
}
impl Token {
fn new(r#type: TokenType, range: TextRange) -> Self {
Self { r#type, range }
}
const fn from_spanned(spanned: &'tok Spanned) -> Token<'tok> {
let type_ = match &spanned.0 {
fn irrelevant() -> Token {
Token {
r#type: TokenType::Irrelevant,
range: TextRange::default(),
}
}
}
impl From<&Spanned> for Token {
fn from(spanned: &Spanned) -> Self {
let r#type = match &spanned.0 {
Tok::NonLogicalNewline => TokenType::NonLogicalNewline,
Tok::Newline => TokenType::Newline,
Tok::For => TokenType::For,
@ -63,8 +75,8 @@ impl<'tok> Token<'tok> {
_ => TokenType::Irrelevant,
};
Self {
spanned: Some(spanned),
type_,
range: spanned.1,
r#type,
}
}
}
@ -92,14 +104,14 @@ enum ContextType {
/// Comma context - described a comma-delimited "situation".
#[derive(Copy, Clone)]
struct Context {
type_: ContextType,
r#type: ContextType,
num_commas: u32,
}
impl Context {
const fn new(type_: ContextType) -> Self {
const fn new(r#type: ContextType) -> Self {
Self {
type_,
r#type,
num_commas: 0,
}
}
@ -222,21 +234,49 @@ pub(crate) fn trailing_commas(
diagnostics: &mut Vec<Diagnostic>,
tokens: &[LexResult],
locator: &Locator,
indexer: &Indexer,
) {
let mut fstrings = 0u32;
let tokens = tokens
.iter()
.flatten()
.filter_map(|spanned @ (tok, tok_range)| match tok {
// Completely ignore comments -- they just interfere with the logic.
.filter(|&r| !matches!(r, (Tok::Comment(_), _)))
.map(Token::from_spanned);
Tok::Comment(_) => None,
// F-strings are handled as `String` token type with the complete range
// of the outermost f-string. This means that the expression inside the
// f-string is not checked for trailing commas.
Tok::FStringStart => {
fstrings = fstrings.saturating_add(1);
None
}
Tok::FStringEnd => {
fstrings = fstrings.saturating_sub(1);
if fstrings == 0 {
indexer
.fstring_ranges()
.outermost(tok_range.start())
.map(|range| Token::new(TokenType::String, range))
} else {
None
}
}
_ => {
if fstrings == 0 {
Some(Token::from(spanned))
} else {
None
}
}
});
let tokens = [Token::irrelevant(), Token::irrelevant()]
.into_iter()
.chain(tokens);
// Collapse consecutive newlines to the first one -- trailing commas are
// added before the first newline.
let tokens = tokens.coalesce(|previous, current| {
if previous.type_ == TokenType::NonLogicalNewline
&& current.type_ == TokenType::NonLogicalNewline
if previous.r#type == TokenType::NonLogicalNewline
&& current.r#type == TokenType::NonLogicalNewline
{
Ok(previous)
} else {
@ -249,8 +289,8 @@ pub(crate) fn trailing_commas(
for (prev_prev, prev, token) in tokens.tuple_windows() {
// Update the comma context stack.
match token.type_ {
TokenType::OpeningBracket => match (prev.type_, prev_prev.type_) {
match token.r#type {
TokenType::OpeningBracket => match (prev.r#type, prev_prev.r#type) {
(TokenType::Named, TokenType::Def) => {
stack.push(Context::new(ContextType::FunctionParameters));
}
@ -261,7 +301,7 @@ pub(crate) fn trailing_commas(
stack.push(Context::new(ContextType::Tuple));
}
},
TokenType::OpeningSquareBracket => match prev.type_ {
TokenType::OpeningSquareBracket => match prev.r#type {
TokenType::ClosingBracket | TokenType::Named | TokenType::String => {
stack.push(Context::new(ContextType::Subscript));
}
@ -288,8 +328,8 @@ pub(crate) fn trailing_commas(
let context = &stack[stack.len() - 1];
// Is it allowed to have a trailing comma before this token?
let comma_allowed = token.type_ == TokenType::ClosingBracket
&& match context.type_ {
let comma_allowed = token.r#type == TokenType::ClosingBracket
&& match context.r#type {
ContextType::No => false,
ContextType::FunctionParameters => true,
ContextType::CallArguments => true,
@ -304,22 +344,21 @@ pub(crate) fn trailing_commas(
};
// Is prev a prohibited trailing comma?
let comma_prohibited = prev.type_ == TokenType::Comma && {
let comma_prohibited = prev.r#type == TokenType::Comma && {
// Is `(1,)` or `x[1,]`?
let is_singleton_tuplish =
matches!(context.type_, ContextType::Subscript | ContextType::Tuple)
matches!(context.r#type, ContextType::Subscript | ContextType::Tuple)
&& context.num_commas <= 1;
// There was no non-logical newline, so prohibit (except in `(1,)` or `x[1,]`).
if comma_allowed && !is_singleton_tuplish {
true
// Lambdas not handled by comma_allowed so handle it specially.
} else {
context.type_ == ContextType::LambdaParameters && token.type_ == TokenType::Colon
context.r#type == ContextType::LambdaParameters && token.r#type == TokenType::Colon
}
};
if comma_prohibited {
let comma = prev.spanned.unwrap();
let mut diagnostic = Diagnostic::new(ProhibitedTrailingComma, comma.1);
let mut diagnostic = Diagnostic::new(ProhibitedTrailingComma, prev.range());
diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(diagnostic.range())));
diagnostics.push(diagnostic);
}
@ -327,10 +366,9 @@ pub(crate) fn trailing_commas(
// Is prev a prohibited trailing comma on a bare tuple?
// Approximation: any comma followed by a statement-ending newline.
let bare_comma_prohibited =
prev.type_ == TokenType::Comma && token.type_ == TokenType::Newline;
prev.r#type == TokenType::Comma && token.r#type == TokenType::Newline;
if bare_comma_prohibited {
let comma = prev.spanned.unwrap();
diagnostics.push(Diagnostic::new(TrailingCommaOnBareTuple, comma.1));
diagnostics.push(Diagnostic::new(TrailingCommaOnBareTuple, prev.range()));
}
// Comma is required if:
@ -339,40 +377,37 @@ pub(crate) fn trailing_commas(
// - Not already present,
// - Not on an empty (), {}, [].
let comma_required = comma_allowed
&& prev.type_ == TokenType::NonLogicalNewline
&& prev.r#type == TokenType::NonLogicalNewline
&& !matches!(
prev_prev.type_,
prev_prev.r#type,
TokenType::Comma
| TokenType::OpeningBracket
| TokenType::OpeningSquareBracket
| TokenType::OpeningCurlyBracket
);
if comma_required {
let missing_comma = prev_prev.spanned.unwrap();
let mut diagnostic = Diagnostic::new(
MissingTrailingComma,
TextRange::empty(missing_comma.1.end()),
);
let mut diagnostic =
Diagnostic::new(MissingTrailingComma, TextRange::empty(prev_prev.end()));
// Create a replacement that includes the final bracket (or other token),
// rather than just inserting a comma at the end. This prevents the UP034 fix
// removing any brackets in the same linter pass - doing both at the same time could
// lead to a syntax error.
let contents = locator.slice(missing_comma.1);
let contents = locator.slice(prev_prev.range());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
format!("{contents},"),
missing_comma.1,
prev_prev.range(),
)));
diagnostics.push(diagnostic);
}
// Pop the current context if the current token ended it.
// The top context is never popped (if unbalanced closing brackets).
let pop_context = match context.type_ {
let pop_context = match context.r#type {
// Lambda terminated by `:`.
ContextType::LambdaParameters => token.type_ == TokenType::Colon,
ContextType::LambdaParameters => token.r#type == TokenType::Colon,
// All others terminated by a closing bracket.
// flake8-commas doesn't verify that it matches the opening...
_ => token.type_ == TokenType::ClosingBracket,
_ => token.r#type == TokenType::ClosingBracket,
};
if pop_context && stack.len() > 1 {
stack.pop();

39
crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap
View File

@ -939,4 +939,43 @@ COM81.py:632:42: COM812 [*] Trailing comma missing
634 634 |
635 635 | foo = namedtuple(
COM81.py:644:46: COM819 [*] Trailing comma prohibited
|
643 | # F-strings
644 | kwargs.pop("remove", f"this {trailing_comma}",)
| ^ COM819
645 |
646 | raise Exception(
|
= help: Remove trailing comma
ℹ Safe fix
641 641 | )
642 642 |
643 643 | # F-strings
644 |-kwargs.pop("remove", f"this {trailing_comma}",)
644 |+kwargs.pop("remove", f"this {trailing_comma}")
645 645 |
646 646 | raise Exception(
647 647 | "first", extra=f"Add trailing comma here ->"
COM81.py:647:49: COM812 [*] Trailing comma missing
|
646 | raise Exception(
647 | "first", extra=f"Add trailing comma here ->"
| COM812
648 | )
|
= help: Add trailing comma
ℹ Safe fix
644 644 | kwargs.pop("remove", f"this {trailing_comma}",)
645 645 |
646 646 | raise Exception(
647 |- "first", extra=f"Add trailing comma here ->"
647 |+ "first", extra=f"Add trailing comma here ->",
648 648 | )
649 649 |
650 650 | assert False, f"<- This is not a trailing comma"