Skip to content

Commit 2ba91dd

Browse files
JarchoJarcho
committed
Actually check for a problematic line comment in with_leading_whitespace 
1 parent 0b01931 commit 2ba91dd

File tree

9 files changed

+78
-39
lines changed

9 files changed

+78
-39
lines changed

‎clippy_lints/src/cognitive_complexity.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ impl CognitiveComplexity {
104104
FnKind::Closure => {
105105
let header_span = body_span.with_hi(decl.output.span().lo());
106106
#[expect(clippy::range_plus_one)]
107-
if let Some(range) = header_span.map_range(cx, |src, range| {
107+
if let Some(range) = header_span.map_range(cx, |_, src, range| {
108108
let mut idxs = src.get(range.clone())?.match_indices('|');
109109
Some(range.start + idxs.next()?.0..range.start + idxs.next()?.0 + 1)
110110
}) {

‎clippy_lints/src/copies.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ fn lint_branches_sharing_code<'tcx>(
258258
let span = span.with_hi(last_block.span.hi());
259259
// Improve formatting if the inner block has indentation (i.e. normal Rust formatting)
260260
let span = span
261-
.map_range(cx, |src, range| {
261+
.map_range(cx, |_, src, range| {
262262
(range.start > 4 && src.get(range.start - 4..range.start)? == " ")
263263
.then_some(range.start - 4..range.end)
264264
})

‎clippy_lints/src/implicit_hasher.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
119119
}
120120

121121
let generics_suggestion_span = impl_.generics.span.substitute_dummy({
122-
let range = (item.span.lo()..target.span().lo()).map_range(cx, |src, range| {
122+
let range = (item.span.lo()..target.span().lo()).map_range(cx, |_, src, range| {
123123
Some(src.get(range.clone())?.find("impl")? + 4..range.end)
124124
});
125125
if let Some(range) = range {
@@ -165,11 +165,12 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
165165
continue;
166166
}
167167
let generics_suggestion_span = generics.span.substitute_dummy({
168-
let range = (item.span.lo()..body.params[0].pat.span.lo()).map_range(cx, |src, range| {
169-
let (pre, post) = src.get(range.clone())?.split_once("fn")?;
170-
let pos = post.find('(')? + pre.len() + 2;
171-
Some(pos..pos)
172-
});
168+
let range =
169+
(item.span.lo()..body.params[0].pat.span.lo()).map_range(cx, |_, src, range| {
170+
let (pre, post) = src.get(range.clone())?.split_once("fn")?;
171+
let pos = post.find('(')? + pre.len() + 2;
172+
Some(pos..pos)
173+
});
173174
if let Some(range) = range {
174175
range.with_ctxt(item.span.ctxt())
175176
} else {

‎clippy_lints/src/methods/manual_inspect.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
101101
UseKind::Return(s) => edits.push((s.with_leading_whitespace(cx).with_ctxt(s.ctxt()), String::new())),
102102
UseKind::Borrowed(s) => {
103103
#[expect(clippy::range_plus_one)]
104-
let range = s.map_range(cx, |src, range| {
104+
let range = s.map_range(cx, |_, src, range| {
105105
let src = src.get(range.clone())?;
106106
let trimmed = src.trim_start_matches([' ', '\t', '\n', '\r', '(']);
107107
trimmed.starts_with('&').then(|| {

‎clippy_utils/src/source.rs‎

Lines changed: 62 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@ use std::sync::Arc;
77
use rustc_ast::{LitKind, StrStyle};
88
use rustc_errors::Applicability;
99
use rustc_hir::{BlockCheckMode, Expr, ExprKind, UnsafeSource};
10+
use rustc_lexer::{LiteralKind, TokenKind, tokenize};
1011
use rustc_lint::{EarlyContext, LateContext};
1112
use rustc_middle::ty::TyCtxt;
1213
use rustc_session::Session;
1314
use rustc_span::source_map::{SourceMap, original_sp};
1415
use rustc_span::{
15-
BytePos, DUMMY_SP, FileNameDisplayPreference, Pos, SourceFile, SourceFileAndLine, Span, SpanData, SyntaxContext,
16-
hygiene,
16+
BytePos, DUMMY_SP, FileNameDisplayPreference, Pos, RelativeBytePos, SourceFile, SourceFileAndLine, Span, SpanData,
17+
SyntaxContext, hygiene,
1718
};
1819
use std::borrow::Cow;
1920
use std::fmt;
@@ -137,24 +138,24 @@ pub trait SpanRangeExt: SpanRange {
137138
fn map_range(
138139
self,
139140
cx: &impl HasSession,
140-
f: impl for<'a> FnOnce(&'a str, Range<usize>) -> Option<Range<usize>>,
141+
f: impl for<'a> FnOnce(&'a SourceFile, &'a str, Range<usize>) -> Option<Range<usize>>,
141142
) -> Option<Range<BytePos>> {
142143
map_range(cx.sess().source_map(), self.into_range(), f)
143144
}
144145

145-
/// Extends the range to include all preceding whitespace characters, unless there
146-
/// are non-whitespace characters left on the same line after `self`.
146+
/// Extends the range to include all preceding whitespace characters.
147+
///
148+
/// The range will not be expanded if it would cross a line boundary, the line the range would
149+
/// be extended to ends with a line comment and the text after the range contains a
150+
/// non-whitespace character on the same line. e.g.
147151
///
148-
/// This extra condition prevents a problem when removing the '}' in:
149152
/// ```ignore
150-
/// ( // There was an opening bracket after the parenthesis, which has been removed
151-
/// // This is a comment
152-
/// })
153+
/// ( // Some comment
154+
/// foo)
153155
/// ```
154-
/// Removing the whitespaces, including the linefeed, before the '}', would put the
155-
/// closing parenthesis at the end of the `// This is a comment` line, which would
156-
/// make it part of the comment as well. In this case, it is best to keep the span
157-
/// on the '}' alone.
156+
///
157+
/// When the range points to `foo`, suggesting to remove the range after it's been extended will
158+
/// cause the `)` to be placed inside the line comment as `( // Some comment)`.
158159
fn with_leading_whitespace(self, cx: &impl HasSession) -> Range<BytePos> {
159160
with_leading_whitespace(cx.sess().source_map(), self.into_range())
160161
}
@@ -253,11 +254,11 @@ fn with_source_text_and_range<T>(
253254
fn map_range(
254255
sm: &SourceMap,
255256
sp: Range<BytePos>,
256-
f: impl for<'a> FnOnce(&'a str, Range<usize>) -> Option<Range<usize>>,
257+
f: impl for<'a> FnOnce(&'a SourceFile, &'a str, Range<usize>) -> Option<Range<usize>>,
257258
) -> Option<Range<BytePos>> {
258259
if let Some(src) = get_source_range(sm, sp.clone())
259260
&& let Some(text) = &src.sf.src
260-
&& let Some(range) = f(text, src.range.clone())
261+
&& let Some(range) = f(&*src.sf, text, src.range.clone())
261262
{
262263
debug_assert!(
263264
range.start <= text.len() && range.end <= text.len(),
@@ -274,20 +275,57 @@ fn map_range(
274275
}
275276
}
276277

278+
fn ends_with_line_comment_or_broken(text: &str) -> bool {
279+
let Some(last) = tokenize(text).last() else {
280+
return false;
281+
};
282+
match last.kind {
283+
// Will give the wrong result on text like `" // "` where the first quote ends a string
284+
// started earlier. The only workaround is to lex the whole file which we don't really want
285+
// to do.
286+
TokenKind::LineComment { .. } | TokenKind::BlockComment { terminated: false, .. } => true,
287+
TokenKind::Literal { kind, .. } => matches!(
288+
kind,
289+
LiteralKind::Byte { terminated: false }
290+
| LiteralKind::ByteStr { terminated: false }
291+
| LiteralKind::CStr { terminated: false }
292+
| LiteralKind::Char { terminated: false }
293+
| LiteralKind::RawByteStr { n_hashes: None }
294+
| LiteralKind::RawCStr { n_hashes: None }
295+
| LiteralKind::RawStr { n_hashes: None }
296+
),
297+
_ => false,
298+
}
299+
}
300+
301+
fn with_leading_whitespace_inner(lines: &[RelativeBytePos], src: &str, range: Range<usize>) -> Option<usize> {
302+
debug_assert!(lines.is_empty() || lines[0].to_u32() == 0);
303+
304+
let start = src.get(..range.start)?.trim_end();
305+
let next_line = lines.partition_point(|&pos| pos.to_usize() <= start.len());
306+
if let Some(line_end) = lines.get(next_line)
307+
&& line_end.to_usize() <= range.start
308+
&& let prev_start = lines.get(next_line - 1).map_or(0, |&x| x.to_usize())
309+
&& ends_with_line_comment_or_broken(&start[prev_start..])
310+
&& let next_line = lines.partition_point(|&pos| pos.to_usize() < range.end)
311+
&& let next_start = lines.get(next_line).map_or(src.len(), |&x| x.to_usize())
312+
&& tokenize(src.get(range.end..next_start)?).any(|t| !matches!(t.kind, TokenKind::Whitespace))
313+
{
314+
Some(range.start)
315+
} else {
316+
Some(start.len())
317+
}
318+
}
319+
277320
fn with_leading_whitespace(sm: &SourceMap, sp: Range<BytePos>) -> Range<BytePos> {
278-
map_range(sm, sp, |src, range| {
279-
let non_blank_after = src.len() - src.get(range.end..)?.trim_start().len();
280-
if src.get(range.end..non_blank_after)?.contains(['\r', '\n']) {
281-
Some(src.get(..range.start)?.trim_end().len()..range.end)
282-
} else {
283-
Some(range)
284-
}
321+
map_range(sm, sp.clone(), |sf, src, range| {
322+
Some(with_leading_whitespace_inner(sf.lines(), src, range.clone())?..range.end)
285323
})
286-
.unwrap()
324+
.unwrap_or(sp)
287325
}
288326

289327
fn trim_start(sm: &SourceMap, sp: Range<BytePos>) -> Range<BytePos> {
290-
map_range(sm, sp.clone(), |src, range| {
328+
map_range(sm, sp.clone(), |_, src, range| {
291329
let src = src.get(range.clone())?;
292330
Some(range.start + (src.len() - src.trim_start().len())..range.end)
293331
})

‎tests/ui-toml/collapsible_if/collapsible_if.fixed‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ fn main() {
1313
//~^^^^^^ collapsible_if
1414

1515
// 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
1717
&& y == "world" {
1818
println!("Hello world!");
1919
}
@@ -26,7 +26,7 @@ fn main() {
2626
}
2727
//~^^^^^^ collapsible_if
2828

29-
if x == "hello" /* Inner comment */
29+
if x == "hello" /* Inner comment */
3030
&& y == "world" {
3131
println!("Hello world!");
3232
}

‎tests/ui-toml/collapsible_if/collapsible_if.stderr‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ LL | | }
3232
|
3333
help: collapse nested if block
3434
|
35-
LL ~ if x == "hello" // Inner comment
35+
LL ~ if x == "hello" // Inner comment
3636
LL ~ && y == "world" {
3737
LL | println!("Hello world!");
3838
LL ~ }
@@ -70,7 +70,7 @@ LL | | }
7070
|
7171
help: collapse nested if block
7272
|
73-
LL ~ if x == "hello" /* Inner comment */
73+
LL ~ if x == "hello" /* Inner comment */
7474
LL ~ && y == "world" {
7575
LL | println!("Hello world!");
7676
LL ~ }

‎tests/ui/manual_inspect.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ fn main() {
107107
let _ = || {
108108
let _x = x;
109109
};
110-
return ;
110+
return;
111111
}
112112
println!("test");
113113
});

‎tests/ui/manual_inspect.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ LL | if x.is_empty() {
9898
LL | let _ = || {
9999
LL ~ let _x = x;
100100
LL | };
101-
LL ~ return ;
101+
LL ~ return;
102102
LL | }
103103
LL ~ println!("test");
104104
|

0 commit comments

Comments
 (0)