Enhance suggestions of `dropping_*` lints #126275

tbu- · 2024-06-11T15:11:03Z

Distinguish dropping mutable references and add more intelligent suggestions if a reference or copyable type is dropped: E.g. delete the expression if it is trivially side-effect free.

Fixes #125972.

Dropping mutable references is not a no-op, it makes the mutable reference inaccessible (as it is not a `Copy` type).

Previously, the lint suggested to change code like `drop(&variable);` into `let _ = &variable;`. Now it suggests to remove the whole expression.

The lint will now suggest to simply remove the `drop` call instead of changing it to `let _ = …`.

rustbot · 2024-06-11T15:11:11Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Urgau · 2024-06-11T15:27:52Z

delete the expression if it is trivially side-effect free.

T-lang was the one to suggest that we should explicitly suggest let _ = ... instead of completely removing the expression, cf. #109732 (comment), in big part to avoid the unused variable lint.

nnethercote · 2024-06-12T23:48:02Z

I have read through #125972, and now this comment:

T-lang was the one to suggest that we should explicitly suggest let _ = ... instead of completely removing the expression, cf. #109732 (comment), in big part to avoid the unused variable lint.

I am now quite confused! There has been a lot of back and forth, it's not clear to me what the conclusion was and if there is consensus about that conclusion. Can someone summarize for me the individual changes being proposed here, each with a simple example? Thanks.

tbu- · 2024-06-13T08:06:35Z

I am now quite confused! There has been a lot of back and forth, it's not clear to me what the conclusion was and if there is consensus about that conclusion. Can someone summarize for me the individual changes being proposed here, each with a simple example? Thanks.

The first commit tweaks the wording of the lint if a mutable reference is mem::dropped.

The second commit suggests to remove the entire mem::drop expression including its argument when it's trivially a no-op. This includes cases like mem::drop(&local_variable). It also changes the suggestion for ()/! drops to simply remove the mem::drop leaving its argument, e.g. mem::drop(vec.push(1)) to vec.push(1).

The third commit adds some tests for the latter thing.

nnethercote · 2024-06-14T03:53:15Z

tests/ui/lint/dropping_references-can-fixed.stderr

@@ -45,7 +45,7 @@ LL -     drop(&&owned1);
 LL +     let _ = &&owned1;
   |

-error: calls to `std::mem::drop` with a reference instead of an owned value does nothing


So it's the "does nothing" part that is the problem here? I.e. it's not true for mutable references?

How about just changing the message to this: "a call to std::mem::drop that is passed a reference drops the reference, not the underlying value". It is true for both kinds of reference, and avoids the need for two different error messages?

So it's the "does nothing" part that is the problem here? I.e. it's not true for mutable references?

Yes.

How about just changing the message to this: "a call to std::mem::drop that is passed a reference drops the reference, not the underlying value". It is true for both kinds of reference, and avoids the need for two different error messages?

Works for me, I'm not attached to the original message.

So it's the "does nothing" part that is the problem here? I.e. it's not true for mutable references?

Yes.

IMO, this isn't true, the call to std::mem::drop with a reference (mutable or not) does nothing to the reference or the underline value. The fact that is does something to the binding/variable it-self is unfortunate but IMO irrelevant here.

How about just changing the message to this: "a call to std::mem::drop that is passed a reference drops the reference, not the underlying value".

I would personally prefer: "calls to std::mem::drop with a reference does not drop the underline value"

nnethercote · 2024-06-14T03:54:48Z

tests/ui/lint/dropping_references-can-fixed.stderr

   |
 LL -     drop(&owned1);
-LL +     let _ = &owned1;


Is this the best suggestion? Perhaps changing it to drop(owned1) is what the author really intended?

I agree that suggesting let _ = ... is a bit silly when the drop argument has an explicit &. It's more reasonable for something like drop(ref);.

I agree that suggesting let _ = ... is a bit silly when the drop argument has an explicit &. It's more reasonable for something like drop(ref);.

let _ = … makes sense as a replacement when it actually drops something, e.g. for let _ = func_call(). let _ = variable_containing_mut_ref; is not useful as a replacement because it does exactly nothing, it simply matches the always matching pattern against the variable. The variable is not made inaccessible.

The variable is not made inaccessible.

That was never the point. The point of the lint is to signal users that the drop call does not drop the underline value.

Whenever or not the variable after the suggestion is made inaccessible is IMO completely irrelevant to the issue that the lint is trying to point at.

let _ = … makes sense as a replacement when it actually drops something

Or as a way to suppress to the "unused variable" lint, as was called out by T-lang in #109732 (comment).

let _ = … makes sense as a replacement when it actually drops something

Or as a way to suppress to the "unused variable" lint, as was called out by T-lang in #109732 (comment).

Yes. I think this should be a conscious decision though. let _ = variable; signals to the reader that the programmer wanted to explicitly silence the unused variable lint. Perhaps we could mention that in the replacement hint? "If you just want to mark the variable as used, use let _ = variable?

Wasn't this already done through the help text: "help: use let _ = ... to ignore the expression or result" ?

I didn't understand the wording that way.

I'd go for something like "help: user let _ = … to mark the variables as used". "to ignore the expression or result [or variable]" doesn't tell me (personally) that this is used to suppress the unused variable lint.

nnethercote · 2024-06-14T04:05:06Z

T-lang was the one to suggest that we should explicitly suggest let _ = ... instead of completely removing the expression, cf. #109732 (comment), in big part to avoid the unused variable lint.

@Urgau, did you mention this to argue against the changes in the second commit?

As per my comments above, I have doubts about both sets of changes here.

Urgau · 2024-06-14T08:28:14Z

did you mention this to argue against the changes in the second commit?

More as a way to signal that the use of let _ = ... was intentional by T-lang and that we should have a strong motivation to override that suggestion.

I'm also very sceptical of calling out (explicitly) the "inaccessibility" issue, I don't think it's relevant to most users. In fact the lint as been stable for many releases without people complaining (or even noticing) about this and I don't think we should complicate something that is (currently) relatively simple to understand with a more advanced notion of "moved variable for non-Copy types".

nnethercote · 2024-06-17T04:31:55Z

@tbu- I think the best path forward here is:

Modify the first commit to just say "a call to std::mem::drop with a reference does not drop the underlying value"
Remove the second and third commits, due to the lack of agreement about their usefulness.

bors · 2024-07-29T05:21:27Z

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2024-08-20T04:48:43Z

@tbu- any updates on this? thanks

alex-semenyuk · 2024-10-07T09:07:08Z

@tbu-
Form wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.

tbu- added 3 commits June 11, 2024 17:07

Distinguish dropping mutable references in lint

15bd318

Dropping mutable references is not a no-op, it makes the mutable reference inaccessible (as it is not a `Copy` type).

Suggest to delete the whole drop expression if it doesn't do anything

5f06edd

Previously, the lint suggested to change code like `drop(&variable);` into `let _ = &variable;`. Now it suggests to remove the whole expression.

Test dropping_copy_types lint when the type is ()

aa06ced

The lint will now suggest to simply remove the `drop` call instead of changing it to `let _ = …`.

rustbot assigned nnethercote Jun 11, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2024

nnethercote reviewed Jun 14, 2024

View reviewed changes

nnethercote added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2024

alex-semenyuk closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance suggestions of `dropping_*` lints #126275

Enhance suggestions of `dropping_*` lints #126275

tbu- commented Jun 11, 2024

rustbot commented Jun 11, 2024

Urgau commented Jun 11, 2024 •

edited

Loading

nnethercote commented Jun 12, 2024

tbu- commented Jun 13, 2024

nnethercote Jun 14, 2024

tbu- Jun 14, 2024

Urgau Jun 14, 2024

nnethercote Jun 14, 2024

nnethercote Jun 14, 2024

tbu- Jun 14, 2024

Urgau Jun 14, 2024

tbu- Jun 14, 2024

Urgau Jun 14, 2024

tbu- Jun 17, 2024

nnethercote commented Jun 14, 2024

Urgau commented Jun 14, 2024

nnethercote commented Jun 17, 2024

bors commented Jul 29, 2024

Dylan-DPC commented Aug 20, 2024

alex-semenyuk commented Oct 7, 2024

Enhance suggestions of dropping_* lints #126275

Enhance suggestions of dropping_* lints #126275

Conversation

tbu- commented Jun 11, 2024

rustbot commented Jun 11, 2024

Urgau commented Jun 11, 2024 • edited Loading

nnethercote commented Jun 12, 2024

tbu- commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Jun 14, 2024

Urgau commented Jun 14, 2024

nnethercote commented Jun 17, 2024

bors commented Jul 29, 2024

Dylan-DPC commented Aug 20, 2024

alex-semenyuk commented Oct 7, 2024

Enhance suggestions of `dropping_*` lints #126275

Enhance suggestions of `dropping_*` lints #126275

Urgau commented Jun 11, 2024 •

edited

Loading