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

Confusing suggestion for dropping_copy_types #125189

Closed
thanatos opened this issue May 16, 2024 · 1 comment · Fixed by #125433
Closed

Confusing suggestion for dropping_copy_types #125189

thanatos opened this issue May 16, 2024 · 1 comment · Fixed by #125433
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-confusing Diagnostics: Confusing error or lint that should be reworked. L-dropping_copy_types Lint: dropping_copy_types T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@thanatos
Copy link
Contributor

thanatos commented May 16, 2024

Code

I had the following code:

    let w_fd = w.as_raw_fd();
    let cmd_stdout = unsafe { Stdio::from_raw_fd(w_fd) };
    let cmd_stderr = unsafe { Stdio::from_raw_fd(w_fd) };
    // Not required, but prevents us from accidentally using it later.
    drop(w_fd);

if we compile this, rustc rightly warns us:

warning: calls to `std::mem::drop` with a value that implements `Copy` does nothing
   --> src/main.rs:158:5
    |
158 |     drop(w_fd);
    |     ^^^^^----^
    |          |
    |          argument has type `i32`
    |
    = note: use `let _ = ...` to ignore the expression or result
    = note: `#[warn(dropping_copy_types)]` on by default

This bit, however:

    = note: use `let _ = ...` to ignore the expression or result

… I'm not clear on. Let's take the compiler's suggestion and modify the code:

    // Not required, but prevents us from accidentally using it later.
    let _ = drop(w_fd);

Now we'll get an even more confusing diagnostic:

warning: calls to `std::mem::drop` with a value that implements `Copy` does nothing
   --> src/main.rs:158:13
    |
158 |     let _ = drop(w_fd);
    |             ^^^^^----^
    |                  |
    |                  argument has type `i32`
    |
    = note: use `let _ = ...` to ignore the expression or result
    = note: `#[warn(dropping_copy_types)]` on by default

…we're now note'd to do something we're already doing.

A competing interpretation of this diagnostic, in my mind, is that it means to do let _ = <the original expression where we compute w_fd>. But we cannot do that, as we use w_fd. I suppose we could inline all the uses, but I wonder if rustc shouldn't realize that no, the variable must be named / that suggestion breaks the compile. Or, should rustc understand that we cannot let _ = this variable, and that we'd need to inline it? But here there are two inlines, and in the general case, side-effects in the expression could make this tricky.

The higher level thing here is that I was attempting to drop a variable from scope, essentially, by moving it with drop. Obviously, that doesn't work for Copy types. I guess I could use {} to scope w_fd … but unfortunately I need both cmd_stdout and cmd_stderr later.

Current output

warning: calls to `std::mem::drop` with a value that implements `Copy` does nothing
   --> src/main.rs:158:5
    |
158 |     drop(w_fd);
    |     ^^^^^----^
    |          |
    |          argument has type `i32`
    |
    = note: use `let _ = ...` to ignore the expression or result
    = note: `#[warn(dropping_copy_types)]` on by default

Desired output

No response

Rationale and extra context

No response

Other cases

No response

Rust Version

rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-unknown-linux-gnu
release: 1.76.0
LLVM version: 17.0.6

Anything else?

No response

@thanatos thanatos added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 16, 2024
@jieyouxu jieyouxu added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-confusing Diagnostics: Confusing error or lint that should be reworked. L-dropping_copy_types Lint: dropping_copy_types labels May 18, 2024
@surechen
Copy link
Contributor

@rustbot claim

surechen added a commit to surechen/rust that referenced this issue May 23, 2024
surechen added a commit to surechen/rust that referenced this issue May 24, 2024
surechen added a commit to surechen/rust that referenced this issue May 24, 2024
surechen added a commit to surechen/rust that referenced this issue May 24, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
fmease added a commit to fmease/rust that referenced this issue May 26, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
jhpratt added a commit to jhpratt/rust that referenced this issue May 26, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 26, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 26, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
bors added a commit to rust-lang-ci/rust that referenced this issue May 26, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
bors added a commit to rust-lang-ci/rust that referenced this issue May 27, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
bors added a commit to rust-lang-ci/rust that referenced this issue May 29, 2024
A small diagnostic improvement for dropping_copy_types

For a value `m`  which implements `Copy` trait, `drop(m);` does nothing.
We now suggest user to ignore it by a abstract and general note: `let _ = ...`.
I think we can give a clearer note here: `let _ = m;`

fixes rust-lang#125189

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
@bors bors closed this as completed in 09c8e39 May 29, 2024
MabezDev pushed a commit to SergioGasquez/rust that referenced this issue May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-confusing Diagnostics: Confusing error or lint that should be reworked. L-dropping_copy_types Lint: dropping_copy_types 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.

3 participants