Actually check for a problematic line comment in with_leading_whitespace

Jarcho · Jarcho · commit 77157f59887f · 2025-05-06T21:42:32.000-04:00
diff --git a/clippy_lints/src/cognitive_complexity.rs b/clippy_lints/src/cognitive_complexity.rs
@@ -104,7 +104,7 @@ impl CognitiveComplexity {
                 FnKind::Closure => {
                     let header_span = body_span.with_hi(decl.output.span().lo());
                     #[expect(clippy::range_plus_one)]
-                    if let Some(range) = header_span.map_range(cx, |src, range| {
+                    if let Some(range) = header_span.map_range(cx, |_, src, range| {
                         let mut idxs = src.get(range.clone())?.match_indices('|');
                         Some(range.start + idxs.next()?.0..range.start + idxs.next()?.0 + 1)
                     }) {
diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs
@@ -258,7 +258,7 @@ fn lint_branches_sharing_code<'tcx>(
         let span = span.with_hi(last_block.span.hi());
         // Improve formatting if the inner block has indentation (i.e. normal Rust formatting)
         let span = span
-            .map_range(cx, |src, range| {
+            .map_range(cx, |_, src, range| {
                 (range.start > 4 && src.get(range.start - 4..range.start)? == "    ")
                     .then_some(range.start - 4..range.end)
             })
diff --git a/clippy_lints/src/implicit_hasher.rs b/clippy_lints/src/implicit_hasher.rs
@@ -119,7 +119,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
                     }
 
                     let generics_suggestion_span = impl_.generics.span.substitute_dummy({
-                        let range = (item.span.lo()..target.span().lo()).map_range(cx, |src, range| {
+                        let range = (item.span.lo()..target.span().lo()).map_range(cx, |_, src, range| {
                             Some(src.get(range.clone())?.find("impl")? + 4..range.end)
                         });
                         if let Some(range) = range {
@@ -165,11 +165,12 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
                             continue;
                         }
                         let generics_suggestion_span = generics.span.substitute_dummy({
-                            let range = (item.span.lo()..body.params[0].pat.span.lo()).map_range(cx, |src, range| {
-                                let (pre, post) = src.get(range.clone())?.split_once("fn")?;
-                                let pos = post.find('(')? + pre.len() + 2;
-                                Some(pos..pos)
-                            });
+                            let range =
+                                (item.span.lo()..body.params[0].pat.span.lo()).map_range(cx, |_, src, range| {
+                                    let (pre, post) = src.get(range.clone())?.split_once("fn")?;
+                                    let pos = post.find('(')? + pre.len() + 2;
+                                    Some(pos..pos)
+                                });
                             if let Some(range) = range {
                                 range.with_ctxt(item.span.ctxt())
                             } else {
diff --git a/clippy_lints/src/methods/manual_inspect.rs b/clippy_lints/src/methods/manual_inspect.rs
@@ -101,7 +101,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
                 UseKind::Return(s) => edits.push((s.with_leading_whitespace(cx).with_ctxt(s.ctxt()), String::new())),
                 UseKind::Borrowed(s) => {
                     #[expect(clippy::range_plus_one)]
-                    let range = s.map_range(cx, |src, range| {
+                    let range = s.map_range(cx, |_, src, range| {
                         let src = src.get(range.clone())?;
                         let trimmed = src.trim_start_matches([' ', '\t', '\n', '\r', '(']);
                         trimmed.starts_with('&').then(|| {
diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -7,13 +7,14 @@ use std::sync::Arc;
 use rustc_ast::{LitKind, StrStyle};
 use rustc_errors::Applicability;
 use rustc_hir::{BlockCheckMode, Expr, ExprKind, UnsafeSource};
+use rustc_lexer::{LiteralKind, TokenKind, tokenize};
 use rustc_lint::{EarlyContext, LateContext};
 use rustc_middle::ty::TyCtxt;
 use rustc_session::Session;
 use rustc_span::source_map::{SourceMap, original_sp};
 use rustc_span::{
-    BytePos, DUMMY_SP, FileNameDisplayPreference, Pos, SourceFile, SourceFileAndLine, Span, SpanData, SyntaxContext,
-    hygiene,
+    BytePos, DUMMY_SP, FileNameDisplayPreference, Pos, RelativeBytePos, SourceFile, SourceFileAndLine, Span, SpanData,
+    SyntaxContext, hygiene,
 };
 use std::borrow::Cow;
 use std::fmt;
@@ -137,25 +138,25 @@ pub trait SpanRangeExt: SpanRange {
     fn map_range(
         self,
         cx: &impl HasSession,
-        f: impl for<'a> FnOnce(&'a str, Range<usize>) -> Option<Range<usize>>,
+        f: impl for<'a> FnOnce(&'a SourceFile, &'a str, Range<usize>) -> Option<Range<usize>>,
     ) -> Option<Range<BytePos>> {
         map_range(cx.sess().source_map(), self.into_range(), f)
     }
 
     #[allow(rustdoc::invalid_rust_codeblocks, reason = "The codeblock is intentionally broken")]
-    /// Extends the range to include all preceding whitespace characters, unless there
-    /// are non-whitespace characters left on the same line after `self`.
+    /// Extends the range to include all preceding whitespace characters.
+    ///
+    /// The range will not be expanded if it would cross a line boundary, the line the range would
+    /// be extended to ends with a line comment and the text after the range contains a
+    /// non-whitespace character on the same line. e.g.
     ///
-    /// This extra condition prevents a problem when removing the '}' in:
     /// ```ignore
-    ///   ( // There was an opening bracket after the parenthesis, which has been removed
-    ///     // This is a comment
-    ///    })
+    /// ( // Some comment
+    /// foo)
     /// ```
-    /// Removing the whitespaces, including the linefeed, before the '}', would put the
-    /// closing parenthesis at the end of the `// This is a comment` line, which would
-    /// make it part of the comment as well. In this case, it is best to keep the span
-    /// on the '}' alone.
+    ///
+    /// When the range points to `foo`, suggesting to remove the range after it's been extended will
+    /// cause the `)` to be placed inside the line comment as `( // Some comment)`.
     fn with_leading_whitespace(self, cx: &impl HasSession) -> Range<BytePos> {
         with_leading_whitespace(cx.sess().source_map(), self.into_range())
     }
@@ -254,11 +255,11 @@ fn with_source_text_and_range<T>(
 fn map_range(
     sm: &SourceMap,
     sp: Range<BytePos>,
-    f: impl for<'a> FnOnce(&'a str, Range<usize>) -> Option<Range<usize>>,
+    f: impl for<'a> FnOnce(&'a SourceFile, &'a str, Range<usize>) -> Option<Range<usize>>,
 ) -> Option<Range<BytePos>> {
     if let Some(src) = get_source_range(sm, sp.clone())
         && let Some(text) = &src.sf.src
-        && let Some(range) = f(text, src.range.clone())
+        && let Some(range) = f(&src.sf, text, src.range.clone())
     {
         debug_assert!(
             range.start <= text.len() && range.end <= text.len(),
@@ -275,20 +276,57 @@ fn map_range(
     }
 }
 
+fn ends_with_line_comment_or_broken(text: &str) -> bool {
+    let Some(last) = tokenize(text).last() else {
+        return false;
+    };
+    match last.kind {
+        // Will give the wrong result on text like `" // "` where the first quote ends a string
+        // started earlier. The only workaround is to lex the whole file which we don't really want
+        // to do.
+        TokenKind::LineComment { .. } | TokenKind::BlockComment { terminated: false, .. } => true,
+        TokenKind::Literal { kind, .. } => matches!(
+            kind,
+            LiteralKind::Byte { terminated: false }
+                | LiteralKind::ByteStr { terminated: false }
+                | LiteralKind::CStr { terminated: false }
+                | LiteralKind::Char { terminated: false }
+                | LiteralKind::RawByteStr { n_hashes: None }
+                | LiteralKind::RawCStr { n_hashes: None }
+                | LiteralKind::RawStr { n_hashes: None }
+        ),
+        _ => false,
+    }
+}
+
+fn with_leading_whitespace_inner(lines: &[RelativeBytePos], src: &str, range: Range<usize>) -> Option<usize> {
+    debug_assert!(lines.is_empty() || lines[0].to_u32() == 0);
+
+    let start = src.get(..range.start)?.trim_end();
+    let next_line = lines.partition_point(|&pos| pos.to_usize() <= start.len());
+    if let Some(line_end) = lines.get(next_line)
+        && line_end.to_usize() <= range.start
+        && let prev_start = lines.get(next_line - 1).map_or(0, |&x| x.to_usize())
+        && ends_with_line_comment_or_broken(&start[prev_start..])
+        && let next_line = lines.partition_point(|&pos| pos.to_usize() < range.end)
+        && let next_start = lines.get(next_line).map_or(src.len(), |&x| x.to_usize())
+        && tokenize(src.get(range.end..next_start)?).any(|t| !matches!(t.kind, TokenKind::Whitespace))
+    {
+        Some(range.start)
+    } else {
+        Some(start.len())
+    }
+}
+
 fn with_leading_whitespace(sm: &SourceMap, sp: Range<BytePos>) -> Range<BytePos> {
-    map_range(sm, sp, |src, range| {
-        let non_blank_after = src.len() - src.get(range.end..)?.trim_start().len();
-        if src.get(range.end..non_blank_after)?.contains(['\r', '\n']) {
-            Some(src.get(..range.start)?.trim_end().len()..range.end)
-        } else {
-            Some(range)
-        }
+    map_range(sm, sp.clone(), |sf, src, range| {
+        Some(with_leading_whitespace_inner(sf.lines(), src, range.clone())?..range.end)
     })
-    .unwrap()
+    .unwrap_or(sp)
 }
 
 fn trim_start(sm: &SourceMap, sp: Range<BytePos>) -> Range<BytePos> {
-    map_range(sm, sp.clone(), |src, range| {
+    map_range(sm, sp.clone(), |_, src, range| {
         let src = src.get(range.clone())?;
         Some(range.start + (src.len() - src.trim_start().len())..range.end)
     })
diff --git a/tests/ui-toml/collapsible_if/collapsible_if.fixed b/tests/ui-toml/collapsible_if/collapsible_if.fixed
@@ -13,7 +13,7 @@ fn main() {
     //~^^^^^^ collapsible_if
 
     // The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798
-    if x == "hello"  // Inner comment
+    if x == "hello" // Inner comment
         && y == "world" {
             println!("Hello world!");
         }
@@ -26,7 +26,7 @@ fn main() {
         }
     //~^^^^^^ collapsible_if
 
-    if x == "hello"  /* Inner comment */
+    if x == "hello" /* Inner comment */
         && y == "world" {
             println!("Hello world!");
         }
diff --git a/tests/ui-toml/collapsible_if/collapsible_if.stderr b/tests/ui-toml/collapsible_if/collapsible_if.stderr
@@ -32,7 +32,7 @@ LL | |     }
    |
 help: collapse nested if block
    |
-LL ~     if x == "hello"  // Inner comment
+LL ~     if x == "hello" // Inner comment
 LL ~         && y == "world" {
 LL |             println!("Hello world!");
 LL ~         }
@@ -70,7 +70,7 @@ LL | |     }
    |
 help: collapse nested if block
    |
-LL ~     if x == "hello"  /* Inner comment */
+LL ~     if x == "hello" /* Inner comment */
 LL ~         && y == "world" {
 LL |             println!("Hello world!");
 LL ~         }
diff --git a/tests/ui/manual_inspect.fixed b/tests/ui/manual_inspect.fixed
@@ -107,7 +107,7 @@ fn main() {
             let _ = || {
                 let _x = x;
             };
-            return ;
+            return;
         }
         println!("test");
     });
diff --git a/tests/ui/manual_inspect.stderr b/tests/ui/manual_inspect.stderr
@@ -98,7 +98,7 @@ LL |         if x.is_empty() {
 LL |             let _ = || {
 LL ~                 let _x = x;
 LL |             };
-LL ~             return ;
+LL ~             return;
 LL |         }
 LL ~         println!("test");
    |

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ fn main() {`
`13`	`13`	`//~^^^^^^ collapsible_if`
`14`	`14`
`15`	`15`	`// The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798`
`16`		`- if x == "hello" // Inner comment`
	`16`	`+ if x == "hello" // Inner comment`
`17`	`17`	`&& y == "world" {`
`18`	`18`	`println!("Hello world!");`
`19`	`19`	`}`
`@@ -26,7 +26,7 @@ fn main() {`
`26`	`26`	`}`
`27`	`27`	`//~^^^^^^ collapsible_if`
`28`	`28`
`29`		`- if x == "hello" /* Inner comment */`
	`29`	`+ if x == "hello" /* Inner comment */`
`30`	`30`	`&& y == "world" {`
`31`	`31`	`println!("Hello world!");`
`32`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ LL \| \| }`
`32`	`32`	`\|`
`33`	`33`	`help: collapse nested if block`
`34`	`34`	`\|`
`35`		`-LL ~ if x == "hello" // Inner comment`
	`35`	`+LL ~ if x == "hello" // Inner comment`
`36`	`36`	`LL ~ && y == "world" {`
`37`	`37`	`LL \| println!("Hello world!");`
`38`	`38`	`LL ~ }`
`@@ -70,7 +70,7 @@ LL \| \| }`
`70`	`70`	`\|`
`71`	`71`	`help: collapse nested if block`
`72`	`72`	`\|`
`73`		`-LL ~ if x == "hello" /* Inner comment */`
	`73`	`+LL ~ if x == "hello" /* Inner comment */`
`74`	`74`	`LL ~ && y == "world" {`
`75`	`75`	`LL \| println!("Hello world!");`
`76`	`76`	`LL ~ }`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,7 @@ fn main() {`
`107`	`107`	`let _ = \|\| {`
`108`	`108`	`let _x = x;`
`109`	`109`	`};`
`110`		`- return ;`
	`110`	`+ return;`
`111`	`111`	`}`
`112`	`112`	`println!("test");`
`113`	`113`	`});`
Original file line number	Diff line number	Diff line change
`@@ -98,7 +98,7 @@ LL \| if x.is_empty() {`
`98`	`98`	`LL \| let _ = \|\| {`
`99`	`99`	`LL ~ let _x = x;`
`100`	`100`	`LL \| };`
`101`		`-LL ~ return ;`
	`101`	`+LL ~ return;`
`102`	`102`	`LL \| }`
`103`	`103`	`LL ~ println!("test");`
`104`	`104`	`\|`