rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.42k stars 1.54k forks source link

warn on strings that look like inline format strings but aren't #10195

Open matthiaskrgr opened 1 year ago

matthiaskrgr commented 1 year ago

What it does

fn main() {
    let number = 123;
    let s = "{number:?}"; // this is likely a bug.
    println!("{s}");
}

Lint Name

No response

Category

No response

Advantage

No response

Drawbacks

No response

Example

fn main() {
    let number = 123;
    let s = "{number:?}"; // this is likely a bug.
    println!("{s}");
}

Could be written as:


fn main() {
    let number = 123;
    let s = format!("{number:?}"); 
    println!("{s}");
}
awused commented 1 year ago

I've noticed myself running into this a fair bit with the new format syntax, especially when refactoring code. Before, "{}", arg would be two arguments and fail type checking if I put it into a function call that was expecting a &str, but now "{arg}" is one &str and type checks without issue.

matthiaskrgr commented 12 months ago

I just found an even more interesting case IRL in kani, where it looks like expect()would take a format string, but it does just print the raw strwithout applying any formatting :smile:

https://github.com/model-checking/kani/pull/2865

pub fn main() {
    let mut x = Some(3);
    let y = "aaa";
    x = None.expect("hello {y}");
}

will print literally hello {y} instead of hello aaa

TTWNO commented 8 months ago

Found some code in my crate that did the same thing:

role.rs:632: let encoded = to_bytes(ctxt, &from_role).expect("Unable to encode {from_role}");
role.rs:635: from_slice(&encoded, ctxt).expect("Unable to convert {encoded} into Role");
interface.rs:390: let serde_val = serde_plain::to_string(&iface).expect("Unable to serialize {iface}");
tests.rs:35: .expect("Could not set sender to {unique_bus_name:?}")
tests.rs:91: .expect("Could not set sender to {unique_bus_name:?}")
tests.rs:169: .expect("Could not set sender to {unique_bus_name:?}")

Could we get a comment on if this would be a reasonable lint or not?

emilk commented 1 month ago

This is an easy mistake when using UI toolkits:

let x = 42;
ui.label("The value is {x}"); // oops!
emilk commented 1 month ago

@GuillaumeGomez wdyt?

It requires some thought on exactly what patterns to look for, and I'm sure there will be false positives (so this should probably be an opt-in lint). But at looking for the regex \{\w*:?\??\} should catch quite a lot of the problems, without too many false positives. Looking for the regex \{.*} would catch all problems, but would perhaps have too many false positives

GuillaumeGomez commented 1 month ago

I think it's common enough to definitely deserve a lint. I'm at rustconf currently but I'm putting it in my todo list for next week when I'm back.

EDIT: as for the "level" of check, I'll give a try at different approaches but I think a global regex that then is checked individually (to see if it's actually valid rust syntax and not {{ escaped too) should allow to rule out most false-positive.

GuillaumeGomez commented 1 month ago

I opened https://github.com/rust-lang/rust-clippy/pull/13410. Don't hesitate to give it a try as I'm interested to find out cases I might have missed. :)