Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix literal_string_with_formatting_args lint emitted when it should not #13953

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions clippy_lints/src/literal_string_with_formatting_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use rustc_session::declare_lint_pass;
use rustc_span::{BytePos, Span};

use clippy_utils::diagnostics::span_lint;
use clippy_utils::is_from_proc_macro;
use clippy_utils::mir::enclosing_mir;

declare_clippy_lint! {
Expand Down Expand Up @@ -79,9 +80,9 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option<Strin
}
}

impl LateLintPass<'_> for LiteralStringWithFormattingArg {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
if expr.span.from_expansion() {
impl<'tcx> LateLintPass<'tcx> for LiteralStringWithFormattingArg {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if expr.span.from_expansion() || expr.span.is_dummy() {
return;
}
if let ExprKind::Lit(lit) = expr.kind {
Expand All @@ -95,6 +96,9 @@ impl LateLintPass<'_> for LiteralStringWithFormattingArg {
},
_ => return,
};
if is_from_proc_macro(cx, expr) {
return;
}
let fmt_str = symbol.as_str();
let lo = expr.span.lo();
let mut current = fmt_str;
Expand Down Expand Up @@ -124,7 +128,11 @@ impl LateLintPass<'_> for LiteralStringWithFormattingArg {
pos.start += diff_len;
pos.end += diff_len;

let start = fmt_str[..pos.start].rfind('{').unwrap_or(pos.start);
let mut start = pos.start;
while start < fmt_str.len() && !fmt_str.is_char_boundary(start) {
start += 1;
}
let start = fmt_str[..start].rfind('{').unwrap_or(start);
Comment on lines +131 to +135
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same as fmt_str[..fmt_str.ceil_char_boundary(pos.start)].rfind('{')?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, it looks like the format parser offsets all spans by 1 (presumably because it's relative to the " quote), so that's what causes the index to be in the middle of a character sometimes. Just doing pos.start - 1 instead of pos.start on the old deleted line (to undo the parser's offset) should fix this too as far as I can tell, but this over-conservative approach should be fine too I guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep it over-conservative if you don't mind. 😉

// If this is a unicode character escape, we don't want to lint.
if start > 1 && fmt_str[..start].ends_with("\\u") {
continue;
Expand Down
26 changes: 26 additions & 0 deletions tests/ui/literal_string_with_formatting_arg.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,31 @@
#![warn(clippy::literal_string_with_formatting_args)]
#![allow(clippy::unnecessary_literal_unwrap)]

// Regression test for <https://github.com/rust-lang/rust-clippy/issues/13885>.
// It's not supposed to emit the lint in this case (in `assert!` expansion).
fn compiler_macro() {
fn parse(_: &str) -> Result<(), i32> {
unimplemented!()
}

assert!(
parse(
#[allow(clippy::literal_string_with_formatting_args)]
"foo {:}"
)
.is_err()
);
let value = 0;
assert!(format!("{value}").is_ascii());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add another true positive test such as

Suggested change
}
assert!("{value}".is_ascii());
}

so that we can check that the lint properly triggers in non-formatting-macro arguments?


// Regression test for <https://github.com/rust-lang/rust-clippy/issues/14007>.
fn regression_14007() {
let s = "{и}";
let ш = 12;
let s = "{ш}"; //~ literal_string_with_formatting_args
}

fn main() {
let x: Option<usize> = None;
let y = "hello";
Expand All @@ -13,6 +38,7 @@ fn main() {
x.expect(r"{y:?} {y:?} "); //~ literal_string_with_formatting_args
x.expect(r"{y:?} y:?}"); //~ literal_string_with_formatting_args
x.expect(r##" {y:?} {y:?} "##); //~ literal_string_with_formatting_args
assert!("{y}".is_ascii()); //~ literal_string_with_formatting_args
// Ensure that it doesn't try to go in the middle of a unicode character.
x.expect("———{:?}"); //~ literal_string_with_formatting_args

Expand Down
40 changes: 26 additions & 14 deletions tests/ui/literal_string_with_formatting_arg.stderr
Original file line number Diff line number Diff line change
@@ -1,71 +1,83 @@
error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:7:15
--> tests/ui/literal_string_with_formatting_arg.rs:26:14
|
LL | x.expect("{y} {}");
| ^^^
LL | let s = "{ш}";
| ^^^
|
= note: `-D clippy::literal-string-with-formatting-args` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::literal_string_with_formatting_args)]`

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:8:16
--> tests/ui/literal_string_with_formatting_arg.rs:32:15
|
LL | x.expect("{y} {}");
| ^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:33:16
|
LL | x.expect(" {y} bla");
| ^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:9:15
--> tests/ui/literal_string_with_formatting_arg.rs:34:15
|
LL | x.expect("{:?}");
| ^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:10:15
--> tests/ui/literal_string_with_formatting_arg.rs:35:15
|
LL | x.expect("{y:?}");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:11:16
--> tests/ui/literal_string_with_formatting_arg.rs:36:16
|
LL | x.expect(" {y:?} {y:?} ");
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:12:23
--> tests/ui/literal_string_with_formatting_arg.rs:37:23
|
LL | x.expect(" {y:..} {y:?} ");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:13:16
--> tests/ui/literal_string_with_formatting_arg.rs:38:16
|
LL | x.expect(r"{y:?} {y:?} ");
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:14:16
--> tests/ui/literal_string_with_formatting_arg.rs:39:16
|
LL | x.expect(r"{y:?} y:?}");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:15:19
--> tests/ui/literal_string_with_formatting_arg.rs:40:19
|
LL | x.expect(r##" {y:?} {y:?} "##);
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:17:18
--> tests/ui/literal_string_with_formatting_arg.rs:41:14
|
LL | assert!("{y}".is_ascii());
| ^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:43:18
|
LL | x.expect("———{:?}");
| ^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:27:19
--> tests/ui/literal_string_with_formatting_arg.rs:53:19
|
LL | x.expect(r##" {x:?} "##); // `x` doesn't exist so we shoud not lint
| ^^^^^

error: aborting due to 11 previous errors
error: aborting due to 13 previous errors

46 changes: 46 additions & 0 deletions tests/ui/{literal_string_with_formatting_args}.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Regression test for <https://github.com/rust-lang/rust-clippy/issues/13885>.
// The `dbg` macro generates a literal with the name of the current file, so
// we need to ensure the lint is not emitted in this case.

// Clippy sets `-Zflatten_format_args=no`, which changes the default behavior of how format args
// are lowered and only that one has this non-macro span. Adding the flag makes it repro on
// godbolt and shows a root context span for the file name string.
//
// So instead of having:
//
// ```
// Lit(
// Spanned {
// node: Str(
// "[/app/example.rs:2:5] \"something\" = ",
// Cooked,
// ),
// span: /rustc/eb54a50837ad4bcc9842924f27e7287ca66e294c/library/std/src/macros.rs:365:35: 365:58 (#4),
// },
// ),
// ```
//
// We get:
//
// ```
// Lit(
// Spanned {
// node: Str(
// "/app/example.rs",
// Cooked,
// ),
// span: /app/example.rs:2:5: 2:22 (#0),
// },
// )
// ```

#![crate_name = "foo"]
#![allow(unused)]
#![warn(clippy::literal_string_with_formatting_args)]

fn another_bad() {
let literal_string_with_formatting_args = 0;
dbg!("something");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that the expr.span.from_expansion() check doesn't already prevent this. Looking at -Zunpretty=hir-tree output it does seem like the span of the string with the file name is properly marked as being from the non-root context

Lit(
    Spanned {
        node: Str(
            "[/app/example.rs:2:5] \"something\" = ",
            Cooked,
        ),
        span: /rustc/eb54a50837ad4bcc9842924f27e7287ca66e294c/library/std/src/macros.rs:365:35: 365:58 (#4),
    },
),

Odd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what's happening. Clippy sets -Zflatten_format_args=no, which changes the default behavior of how format args are lowered and only that one has this non-macro span. Adding the flag makes it repro on godbolt and shows a root context span for the file name string.

Lit(
    Spanned {
        node: Str(
            "/app/example.rs",
            Cooked,
        ),
        span: /app/example.rs:2:5: 2:22 (#0),
    },
)

Maybe add to the comment that clippy sets this flag and only with that flag does it have a non-macro span which gets around the from_expansion check, in case anyone wants to look into this in the future and doesn't have to go down this rabbit hole?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it but couldn't figure out why it didn't work. Thanks a lot for the detailed explanations! Adding it as a comment.

}

fn main() {}
Loading