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

Unused braces lint triggers on partial move into ref pattern. #70717

Closed
rodrimati1992 opened this issue Apr 2, 2020 · 11 comments · Fixed by #70789
Closed

Unused braces lint triggers on partial move into ref pattern. #70717

rodrimati1992 opened this issue Apr 2, 2020 · 11 comments · Fixed by #70789
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Apr 2, 2020

I tried this code:

fn main(){
    let a = (String::new(),0);
    let ref _b = {a.0};
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=4b8d234e04e2d77ab45ecf3c41d59d58

I expected the code to compile without warnings.

Instead, the unused_braces lint was triggered:

warning: unnecessary braces around assigned value
 --> src/main.rs:4:18
  |
4 |     let ref _b = {a.0};
  |                  ^^^^^ help: remove these braces
  |
  = note: `#[warn(unused_braces)]` on by default

Meta

This bug happens in 1.44.0-nightly (2020-04-01 76b1198),not in Rust beta nor stable.

@rodrimati1992 rodrimati1992 added the C-bug Category: This is a bug. label Apr 2, 2020
@jonas-schievink jonas-schievink added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2020
@jumbatm
Copy link
Contributor

jumbatm commented Apr 2, 2020

Hello, thanks for the report!

Are the braces not unused? What you have here is

let ref _b = {
    a.0
};

ie, assigning ref binding _b to a block which does nothing but return a.0 as its final expression, which is the same as just assigning directly without the braces (even with the ref).

This bug happens in 1.44.0-nightly (2020-04-01 76b1198),not in Rust beta nor stable.

The unused braces lint was added in b3d744c, which was after the current beta (4c587bb).

@rodrimati1992
Copy link
Contributor Author

rodrimati1992 commented Apr 3, 2020

The braces cause the .0 field to be moved,meaning that this doesn't compile:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=55ac4e94aa2b812a4dfafa4baedf86d4

fn main(){
    let a = (String::new(),0);
    let ref _b = {a.0};
    // Can't print a partially moved value
    println!("{:?}",a);
}
error[E0382]: borrow of moved value: `a`
 --> src/main.rs:4:21
  |
3 |     let ref _b = {a.0};
  |                   --- value moved here
4 |     println!("{:?}",a);
  |                     ^ value borrowed here after partial move
  |
  = note: move occurs because `a.0` has type `std::string::String`, which does not implement the `Copy` trait

While this does:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d939ba3758c02849cc2cbe928990b73f

fn main(){
    let a = (String::new(),0);
    let ref _b = a.0;
    println!("{:?}",a);
}

@jumbatm
Copy link
Contributor

jumbatm commented Apr 3, 2020

Oh, I see - very true! Thanks for clarifying.

@HeroicKatora
Copy link
Contributor

For Copy types this can be devious as it means semantics change without a compilation error:

let a = 0;
let ref mut b = {a};
*b += 1;
assert_eq!(a, 0);

vs.

// `mut` qualifier is necessary, also not observed by the lint.
let mut a = 0;
let ref mut b = a;
*b += 1;
assert_eq!(a, 1);

@shepmaster
Copy link
Member

This may also be triggered by a (IMO common) usage of the quote crate, but it only appears when running rustdoc:

[package]
name = "repro"
version = "0.1.0"
edition = "2018"

[dependencies]
quote = "=1.0.3"
use quote::quote;

pub fn repro() {
    let many = [1, 2, 3];

    let _together = quote! {
        #(#many),*
    };
}
% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 build
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 doc
 Documenting repro v0.1.0 (/private/tmp/repro)
warning: unnecessary braces around block return value
  |
  = note: `#[warn(unused_braces)]` on by default

    Finished dev [unoptimized + debuginfo] target(s) in 0.82s

@shepmaster
Copy link
Member

Ping @lcnr (author of the lint)

@eddyb
Copy link
Member

eddyb commented Apr 4, 2020

Are borrows and "pattern-matching contexts" the only ones where we need to silence the lint?
Do we lint after type-checking? We kind of need to, to take into account implicit borrows.

@lcnr
Copy link
Contributor

lcnr commented Apr 4, 2020

The issue with quote was actually mentioned in the original PR but somehow forgotten.
#70081 (comment)

@eddyb unused_braces currently only warns on top lvl exprs afaik. So I should probably stop linting for "pattern-matching contexts". I don't think that a lint based on syntax should know about types. i.e. changing types should not suddenly cause a warning because of unused braces (this is a personal preference though).

@eddyb
Copy link
Member

eddyb commented Apr 4, 2020

It's not about types, it's about implicit borrows, e.g. ({x.0}).clone(). But if you don't lint that at all then you should be good.

@lcnr
Copy link
Contributor

lcnr commented Apr 4, 2020

@shepmaster I fixed the issue concerning ref patterns for now, but don't yet have a good idea on how to fix the issue with quote.

Can you please open a new issue for this?

@shepmaster
Copy link
Member

Can you please open a new issue for this?

Opened as #70814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants