From 781aef7748835efd6b37c1e1f132c52ee4c677e8 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 9 Nov 2023 17:09:47 +0530 Subject: [PATCH] Consider the new f-string tokens for `flake8-commas` --- .../test/fixtures/flake8_commas/COM81.py | 15 +++ crates/ruff_linter/src/checkers/tokens.rs | 2 +- .../flake8_commas/rules/trailing_commas.rs | 123 +++++++++++------- ...rules__flake8_commas__tests__COM81.py.snap | 39 ++++++ 4 files changed, 133 insertions(+), 46 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py index 8bc53dcd347607..c8987389d987d5 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_commas/COM81.py @@ -639,3 +639,18 @@ def foo( :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 ->" +}""" diff --git a/crates/ruff_linter/src/checkers/tokens.rs b/crates/ruff_linter/src/checkers/tokens.rs index ad623b9e5ba4cc..900f75eb27601f 100644 --- a/crates/ruff_linter/src/checkers/tokens.rs +++ b/crates/ruff_linter/src/checkers/tokens.rs @@ -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) { diff --git a/crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs b/crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs index e3fdd58a7e256d..3f4ff3877c44ac 100644 --- a/crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs +++ b/crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs @@ -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> { +impl Ranged for Token { + fn range(&self) -> TextRange { + self.range + } +} + +impl Token { + fn new(r#type: TokenType, range: TextRange) -> Self { + Self { r#type, range } + } + + fn irrelevant() -> Token { Token { - type_: TokenType::Irrelevant, - spanned: None, + r#type: TokenType::Irrelevant, + range: TextRange::default(), } } +} - const fn from_spanned(spanned: &'tok Spanned) -> Token<'tok> { - let type_ = match &spanned.0 { +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,47 @@ pub(crate) fn trailing_commas( diagnostics: &mut Vec, tokens: &[LexResult], locator: &Locator, + indexer: &Indexer, ) { + let mut fstrings = 0u32; let tokens = tokens .iter() .flatten() - // Completely ignore comments -- they just interfere with the logic. - .filter(|&r| !matches!(r, (Tok::Comment(_), _))) - .map(Token::from_spanned); + .filter_map(|spanned @ (tok, tok_range)| match tok { + // Completely ignore comments -- they just interfere with the logic. + Tok::Comment(_) => None, + // Ignore content within f-strings. + 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 +287,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 +299,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 +326,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 +342,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 +364,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 +375,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(); diff --git a/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap b/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap index d9e4fe146fa656..5e7e1ef24e2553 100644 --- a/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap +++ b/crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap @@ -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" +