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

Missing warning for migration of let _ = x between Rust 2018 and 2021 edition #90465

Closed
sdroege opened this issue Nov 1, 2021 · 11 comments · Fixed by #90597
Closed

Missing warning for migration of let _ = x between Rust 2018 and 2021 edition #90465

sdroege opened this issue Nov 1, 2021 · 11 comments · Fixed by #90597
Labels
A-closures Area: closures (`|args| { .. }`) A-edition-2021 Area: The 2021 edition C-bug Category: This is a bug.

Comments

@sdroege
Copy link
Contributor

sdroege commented Nov 1, 2021

The following example does not get a migration warning in Rust 2018 (or Rust 2021) -- (playground):

#![warn(rust_2021_incompatible_closure_captures)]

fn main() {
    struct Foo(u32);
    impl Drop for Foo {
        fn drop(&mut self) {
            println!("dropped {}", self.0);
        }
    }
    
    let f0 = Foo(0);
    let f1 = Foo(1);
    
    let c0 = move || {
        let _ = f0;
    };
    
    let c1 = move || {
        let _ = &f1;
    };
    
    println!("dropping 0");
    drop(c0);
    println!("dropping 1");
    drop(c1);
    println!("dropped all");
}

The behavior of let _ = x changed in the new edition, but there ought to have been a warning.

Original issue

See the below code

fn main() {
    struct Foo(u32);
    impl Drop for Foo {
        fn drop(&mut self) {
            println!("dropped {}", self.0);
        }
    }
    
    let f0 = Foo(0);
    let f1 = Foo(1);
    
    let c0 = move || {
        let _ = f0;
    };
    
    let c1 = move || {
        let _ = &f1;
    };
    
    println!("dropping 0");
    drop(c0);
    println!("dropping 1");
    drop(c1);
    println!("dropped all");
}

With 2018 edition (playground link) this prints

dropping 0
dropped 0
dropping 1
dropped 1
dropped all

With 2021 edition (playground link) this prints

dropping 0
dropping 1
dropped 1
dropped all
dropped 0

That is, with the 2021 edition the first closure is apparently not capturing f0 at all (and drops it at the end of main) while with the 2018 edition it did.

This might be intentional and expected or not, which is why I'm creating this issue. It might be worth mentioning it as a potential porting trap in the edition guide. Note that cargo fix --edition did not catch this one (but a couple of others where the behaviour difference was irrelevant).

This subtle behaviour change caused a test in gtk-rs to timeout and fail.

@ehuss
Copy link
Contributor

ehuss commented Nov 1, 2021

Thanks for the report! I believe this is already mentioned in the edition guide here. Values that aren't read are not captured, and I believe that is the intended behavior.

However, I am a little surprised that rust_2021_incompatible_closure_captures does not add a suggestion to add let _ = &f0; to it. @rust-lang/wg-rfc-2229 was that intended or a known hole in the migration?

@ehuss ehuss added A-closures Area: closures (`|args| { .. }`) A-edition-2021 Area: The 2021 edition C-bug Category: This is a bug. labels Nov 1, 2021
@sdroege
Copy link
Contributor Author

sdroege commented Nov 2, 2021

The confusing issue here is that the value was actually "used" (well, it was written in the closure body) and only the easy to miss & was missing.

Maybe there should be generally a lint for the let _ = x case in closure bodies as that's probably never what one intended?

@nikomatsakis
Copy link
Contributor

This is indeed expected behavior, but a lint for this case is an interesting idea. I guess that the let _ = x was there previously in order to force capture, @sdroege?

@nikomatsakis
Copy link
Contributor

Hmm, I too am a bit surprised that you don't get a migration lint already. Are you sure that you don't?

@nikomatsakis
Copy link
Contributor

OK, I can attest that you don't get a warning (playground). Curious.

@nikomatsakis nikomatsakis changed the title Subtle unexpected closure variable capture behaviour difference between 2018 and 2021 edition Missing warning for migration of let _ = x between Rust 2018 and 2021 edition Nov 2, 2021
@sdroege
Copy link
Contributor Author

sdroege commented Nov 2, 2021

I guess that the let _ = x was there previously in order to force capture, @sdroege?

Yes, that was the idea.

Hmm, I too am a bit surprised that you don't get a migration lint already. Are you sure that you don't?

Yes, definitely :) I just retried again.

But this should probably not just cause a migration lint but generally cause a warning with the 2021 edition. let _ = x has no effect whatsoever and that's probably not intended by whoever wrote the code.

@nikomatsakis
Copy link
Contributor

I agree that let _ = x should potentially issue a warning, but I don't think it's connected to closures per se. It also has no effect in regular functions. For example, this compiles, even though x is not initialized: (playground)

fn foo() {
    let x: u32;
    let _ = x; 
}

@sdroege
Copy link
Contributor Author

sdroege commented Nov 2, 2021

Indeed, I wasn't aware of this :) Should I create a separate issue for having a general, non-migration warning for this?

@nikomatsakis
Copy link
Contributor

@sdroege +1 from me, I think @Mark-Simulacrum has often discussed their desire for such a lint as well

@sdroege
Copy link
Contributor Author

sdroege commented Nov 3, 2021

Sure, see #90524 . Thanks

@nikomatsakis
Copy link
Contributor

Fix in #90597

@bors bors closed this as completed in 4b1cb73 Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-edition-2021 Area: The 2021 edition C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants