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

A new lint to remove unneeded arg referencing in a format! #10851

Open
nyurik opened this issue May 31, 2023 · 15 comments
Open

A new lint to remove unneeded arg referencing in a format! #10851

nyurik opened this issue May 31, 2023 · 15 comments
Labels
A-lint Area: New lints

Comments

@nyurik
Copy link
Contributor

nyurik commented May 31, 2023

What it does

Passing a reference to a value instead of the value itself to the format! macro causes unneeded double referencing that is not compiled away, as described in my stackoverflow question, and demonstrated in the 1.69 assembly output. The issue seem to be that reference format dispatch is not inlined by llvm. While this might be some issue with the Rust compiler itself, or an issue that is hard/impossible to solve in a general case, I think we could introduce a lint in the mean time to suggest it to improve readability and performance.

Lint Name

unneeded_format_arg_ref

Category

No response

Advantage

  • Improve runtime performance
  • Remove unneeded value referencing
  • Allow variable inlining, e.g. format!("{}", &var) --> format!("{var}")

Drawbacks

Uncertain if there could ever be an edge case where formatting &var and var would produce different result, e.g. if format wants to print the address of a variable?

Example

format("{}", &foo.bar)

Could be written as:

format("{}", foo.bar)
@nyurik nyurik added the A-lint Area: New lints label May 31, 2023
@nyurik
Copy link
Contributor Author

nyurik commented May 31, 2023

Performance update: I consistently see a ~5% performance difference in a small benchmark.

image

@disco07
Copy link
Contributor

disco07 commented May 31, 2023

Hello for your problem, I wish to work on this subject. Is it possible? If yes, have you some suggestions to give me?

@Alexendoo
Copy link
Member

For a plain binding yeah you wouldn't be able to do {:p} if you remove the & for many types. Technically it could be the case for the other formatting traits too that T has a different impl compared to &T, or no impl at all on T

Another thing to watch out for would be !Sized types e.g.

  • &v[..]
  • &*x where x derefs to something like str

@nyurik
Copy link
Contributor Author

nyurik commented May 31, 2023

@Alexendoo thanks, good points. I am unclear about the last one -- all of these seems to work identically:

fn main() {
    let s = "hello world";
    println!("{}", s);
    println!("{}", &s);
    println!("{}", &*s);
}

The rest of it could be added as unit tests -- these should not trigger the lint:

fn main() {
    let vec = vec![1, 2, 3];
    let int = 42_i32;

    println!("{:?}", &vec[0..2]);
    println!("{:p}", &int);
}

@nyurik
Copy link
Contributor Author

nyurik commented May 31, 2023

@disco07 go for it! See https://doc.rust-lang.org/nightly/clippy/development/adding_lints.html as a good starting point

@Alexendoo
Copy link
Member

e.g. only the last println here will compile https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ad20abfd2b21dbd4b77845b28f029d9a

struct S;

impl std::ops::Deref for S {
    type Target = str;
    fn deref(&self) -> &str {
        ""
    }
}

fn main() {
    let s = S;

    println!("{}", s);
    println!("{}", &s);
    println!("{}", *s);
    println!("{}", &*s);
}

@maxammann
Copy link

Actually only the 3rd one will not compile. But same point :)

#[derive(Debug)]
struct S;

impl std::ops::Deref for S {
    type Target = str;
    fn deref(&self) -> &str {
        ""
    }
}

fn main() {
    let s = S;

    println!("{:?}", s);
    println!("{:?}", &s);
    println!("{:?}", *s);
    println!("{:?}", &*s);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2b3c6557c6d6f24c93fae05110331a7f

@Alexendoo
Copy link
Member

Alexendoo commented May 31, 2023

If you change the code to something else... yes

The other two cases still change the behaviour though in that example and so shouldn't be suggested

@maxammann
Copy link

Ah oke sorry, now I got your point :)

@nyurik
Copy link
Contributor Author

nyurik commented Jul 19, 2024

Is there ever a case format!("{var:p}") has any meaning? Or it must always use &var? If so, it may mean that there is no code out there that compiles it, and therefor the format! macro could be improved to auto-ref on :p?

@Alexendoo
Copy link
Member

It makes sense when var is already a reference or pointer, it would be the difference of printing the address stored in var and printing the address of var

@nyurik
Copy link
Contributor Author

nyurik commented Jul 19, 2024

I somehow feel this is almost never the case... to the point of perhaps creating a lint that flags format!("{var:p}") as likely being incorrect

@Alexendoo
Copy link
Member

When would you want to print the stack address of a pointer rather than the pointer itself?

@nyurik
Copy link
Contributor Author

nyurik commented Jul 19, 2024

I don't think you ever would... thus my point that {var:p} is almost always pointless... Maybe we should submit this as a 2024 edition change - to always treat "{var:p}" as "{:p}", &var, and if someone wants to print the address of the address of the var, they will just have to write it explicitly with &&var?

@Alexendoo
Copy link
Member

It's the other way around, &var would print the stack address

tgross35 added a commit to tgross35/rust that referenced this issue Jul 19, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 20, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 20, 2024
Avoid ref when using format! in src

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
bors added a commit to rust-lang/rust-analyzer that referenced this issue Jul 20, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing). Inlining format args prevents accidental `&` misuse.

See rust-lang/rust-clippy#10851
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 20, 2024
Rollup merge of rust-lang#127984 - nyurik:src-refs, r=onur-ozkan

Avoid ref when using format! in src

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 20, 2024
Rollup merge of rust-lang#127980 - nyurik:compiler-refs, r=oli-obk

Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).  Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust-clippy#10851
lnicola pushed a commit to lnicola/rust that referenced this issue Jul 28, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing). Inlining format args prevents accidental `&` misuse.

See rust-lang/rust-clippy#10851
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 1, 2024
Avoid ref when using format! in compiler

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing). Inlining format args prevents accidental `&` misuse.

See rust-lang/rust-clippy#10851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

4 participants