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

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 ->"
+}"""

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) {

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,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()
-        // 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,
+            // 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,33 +344,31 @@ 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);
         }
 
         // 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();

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"
+