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.

Sign up for GitHub

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

Jump to bottom

Enhance suggestions of dropping_* lints #126275

Closed
wants to merge 3 commits into from
Closed

Enhance suggestions of dropping_* lints #126275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15bd318
Distinguish dropping mutable references in lint
tbu- Jun 11, 2024
5f06edd
Suggest to delete the whole `drop` expression if it doesn't do anything
tbu- Jun 11, 2024
aa06ced
Test `dropping_copy_types` lint when the type is `()`
tbu- Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler/rustc_lint/messages.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ lint_drop_trait_constraints =
lint_dropping_copy_types = calls to `std::mem::drop` with a value that implements `Copy` does nothing
.label = argument has type `{$arg_ty}`

lint_dropping_mutable_references = calls to `std::mem::drop` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
.label = argument has type `{$arg_ty}`

lint_dropping_references = calls to `std::mem::drop` with a reference instead of an owned value does nothing
.label = argument has type `{$arg_ty}`

Expand Down Expand Up @@ -270,6 +273,9 @@ lint_for_loops_over_fallibles =
lint_forgetting_copy_types = calls to `std::mem::forget` with a value that implements `Copy` does nothing
.label = argument has type `{$arg_ty}`

lint_forgetting_mutable_references = calls to `std::mem::forget` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
.label = argument has type `{$arg_ty}`

lint_forgetting_references = calls to `std::mem::forget` with a reference instead of an owned value does nothing
.label = argument has type `{$arg_ty}`

Expand Down
41 changes: 32 additions & 9 deletions compiler/rustc_lint/src/drop_forget_useless.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use rustc_hir::{Arm, Expr, ExprKind, Node, StmtKind};
use rustc_hir::{Arm, Expr, ExprKind, Mutability, Node, StmtKind};
use rustc_middle::ty;
use rustc_session::{declare_lint, declare_lint_pass};
use rustc_span::sym;

use crate::{
lints::{
DropCopyDiag, DropRefDiag, ForgetCopyDiag, ForgetRefDiag, UndroppedManuallyDropsDiag,
UndroppedManuallyDropsSuggestion, UseLetUnderscoreIgnoreSuggestion,
DropCopyDiag, DropMutRefDiag, DropRefDiag, ForgetCopyDiag, ForgetMutRefDiag, ForgetRefDiag,
UndroppedManuallyDropsDiag, UndroppedManuallyDropsSuggestion,
UseLetUnderscoreIgnoreSuggestion,
},
LateContext, LateLintPass, LintContext,
};
Expand Down Expand Up @@ -162,15 +163,26 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetUseless {
UseLetUnderscoreIgnoreSuggestion::Note
}
};
match fn_name {
sym::mem_drop if arg_ty.is_ref() && !drop_is_single_call_in_arm => {
match (fn_name, arg_ty.ref_mutability()) {
(sym::mem_drop, Some(Mutability::Not)) if !drop_is_single_call_in_arm => {
cx.emit_span_lint(
DROPPING_REFERENCES,
expr.span,
DropRefDiag { arg_ty, label: arg.span, sugg: let_underscore_ignore_sugg() },
);
}
sym::mem_forget if arg_ty.is_ref() => {
(sym::mem_drop, Some(Mutability::Mut)) if !drop_is_single_call_in_arm => {
cx.emit_span_lint(
DROPPING_REFERENCES,
expr.span,
DropMutRefDiag {
arg_ty,
label: arg.span,
sugg: let_underscore_ignore_sugg(),
},
);
}
(sym::mem_forget, Some(Mutability::Not)) => {
cx.emit_span_lint(
FORGETTING_REFERENCES,
expr.span,
Expand All @@ -181,7 +193,18 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetUseless {
},
);
}
sym::mem_drop if is_copy && !drop_is_single_call_in_arm => {
(sym::mem_forget, Some(Mutability::Mut)) => {
cx.emit_span_lint(
FORGETTING_REFERENCES,
expr.span,
ForgetMutRefDiag {
arg_ty,
label: arg.span,
sugg: let_underscore_ignore_sugg(),
},
);
}
(sym::mem_drop, _) if is_copy && !drop_is_single_call_in_arm => {
cx.emit_span_lint(
DROPPING_COPY_TYPES,
expr.span,
Expand All @@ -192,7 +215,7 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetUseless {
},
);
}
sym::mem_forget if is_copy => {
(sym::mem_forget, _) if is_copy => {
cx.emit_span_lint(
FORGETTING_COPY_TYPES,
expr.span,
Expand All @@ -203,7 +226,7 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetUseless {
},
);
}
sym::mem_drop
(sym::mem_drop, _)
if let ty::Adt(adt, _) = arg_ty.kind()
&& adt.is_manually_drop() =>
{
Expand Down
20 changes: 20 additions & 0 deletions compiler/rustc_lint/src/lints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,16 @@ pub struct DropRefDiag<'a> {
pub sugg: UseLetUnderscoreIgnoreSuggestion,
}

#[derive(LintDiagnostic)]
#[diag(lint_dropping_mutable_references)]
pub struct DropMutRefDiag<'a> {
pub arg_ty: Ty<'a>,
#[label]
pub label: Span,
#[subdiagnostic]
pub sugg: UseLetUnderscoreIgnoreSuggestion,
}

#[derive(LintDiagnostic)]
#[diag(lint_dropping_copy_types)]
pub struct DropCopyDiag<'a> {
Expand All @@ -704,6 +714,16 @@ pub struct ForgetRefDiag<'a> {
pub sugg: UseLetUnderscoreIgnoreSuggestion,
}

#[derive(LintDiagnostic)]
#[diag(lint_forgetting_mutable_references)]
pub struct ForgetMutRefDiag<'a> {
pub arg_ty: Ty<'a>,
#[label]
pub label: Span,
#[subdiagnostic]
pub sugg: UseLetUnderscoreIgnoreSuggestion,
}

#[derive(LintDiagnostic)]
#[diag(lint_forgetting_copy_types)]
pub struct ForgetCopyDiag<'a> {
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lint/dropping_references-can-fixed.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

error: calls to `std::mem::drop` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/dropping_references-can-fixed.rs:14:5
|
LL | drop(&mut owned1);
Expand Down Expand Up @@ -73,7 +73,7 @@ LL - drop(reference1);
LL + let _ = reference1;
|

error: calls to `std::mem::drop` with a reference instead of an owned value does nothing
error: calls to `std::mem::drop` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/dropping_references-can-fixed.rs:21:5
|
LL | drop(reference2);
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lint/dropping_references.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ LL - drop(&&owned1);
LL + let _ = &&owned1;
|

warning: calls to `std::mem::drop` with a reference instead of an owned value does nothing
warning: calls to `std::mem::drop` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/dropping_references.rs:13:5
|
LL | drop(&mut owned1);
Expand Down Expand Up @@ -73,7 +73,7 @@ LL - drop(reference1);
LL + let _ = reference1;
|

warning: calls to `std::mem::drop` with a reference instead of an owned value does nothing
warning: calls to `std::mem::drop` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/dropping_references.rs:20:5
|
LL | drop(reference2);
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lint/forgetting_references-can-fixed.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ LL - forget(&&owned);
LL + let _ = &&owned;
|

error: calls to `std::mem::forget` with a reference instead of an owned value does nothing
error: calls to `std::mem::forget` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/forgetting_references-can-fixed.rs:16:5
|
LL | forget(&mut owned);
Expand Down Expand Up @@ -73,7 +73,7 @@ LL - forget(&*reference1);
LL + let _ = &*reference1;
|

error: calls to `std::mem::forget` with a reference instead of an owned value does nothing
error: calls to `std::mem::forget` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/forgetting_references-can-fixed.rs:23:5
|
LL | forget(reference2);
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lint/forgetting_references.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ LL - forget(&&owned);
LL + let _ = &&owned;
|

warning: calls to `std::mem::forget` with a reference instead of an owned value does nothing
warning: calls to `std::mem::forget` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/forgetting_references.rs:15:5
|
LL | forget(&mut owned);
Expand Down Expand Up @@ -73,7 +73,7 @@ LL - forget(&*reference1);
LL + let _ = &*reference1;
|

warning: calls to `std::mem::forget` with a reference instead of an owned value does nothing
warning: calls to `std::mem::forget` with a mutable reference instead of an owned value only makes the reference inaccessible, it does not drop the underlying value
--> $DIR/forgetting_references.rs:22:5
|
LL | forget(reference2);
Expand Down