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

error-msg: expand suggestion for unused_def lint #109158

Merged
merged 1 commit into from
Mar 16, 2023
Merged

error-msg: expand suggestion for unused_def lint #109158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 10 additions & 12 deletions compiler/rustc_lint/src/lints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1390,7 +1390,7 @@ pub struct UnusedOp<'a> {
pub op: &'a str,
#[label]
pub label: Span,
#[suggestion(style = "verbose", code = "let _ = ", applicability = "machine-applicable")]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could suggest using

_ = ...;

Instead? It silences the warning and looks a bit nicer & shorter IMO.

Copy link
Member

@Noratrieb Noratrieb Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, let _ is more widely used and understood and also nicer imo.

Copy link
Member

Choose a reason for hiding this comment

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

Why is it more idiomatic? I think the only reason let _ is used more, is that it was available since forever, while _ was added after that.

Copy link
Contributor Author

@Ezrashaw Ezrashaw Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, keep in mind that it is about more than just ignoring a value; you might want to bind it to a variable. Removing let preemptively makes using the value as opposed to ignoring it harder.

Also I'd like to keep the let anyway because it's much more common and IMHO conveys intent better.

#[suggestion(style = "verbose", code = "let _ = ", applicability = "maybe-incorrect")]
pub suggestion: Span,
}

Expand Down Expand Up @@ -1434,17 +1434,15 @@ pub struct UnusedDef<'a, 'b> {
}

#[derive(Subdiagnostic)]
pub enum UnusedDefSuggestion {
#[suggestion(
lint_suggestion,
style = "verbose",
code = "let _ = ",
applicability = "machine-applicable"
)]
Default {
#[primary_span]
span: Span,
},
#[suggestion(
lint_suggestion,
style = "verbose",
code = "let _ = ",
applicability = "maybe-incorrect"
)]
pub struct UnusedDefSuggestion {
#[primary_span]
pub span: Span,
}

// Needed because of def_path_str
Expand Down
31 changes: 12 additions & 19 deletions compiler/rustc_lint/src/unused.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
let must_use_result = is_ty_must_use(cx, ty, &expr, expr.span);
let type_lint_emitted_or_suppressed = match must_use_result {
Some(path) => {
emit_must_use_untranslated(cx, &path, "", "", 1);
emit_must_use_untranslated(cx, &path, "", "", 1, false);
true
}
None => false,
Expand Down Expand Up @@ -358,6 +358,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
descr_pre_path,
descr_post_path,
1,
false,
)
})
.is_some()
Expand All @@ -370,27 +371,30 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
descr_pre: &str,
descr_post: &str,
plural_len: usize,
is_inner: bool,
) {
let plural_suffix = pluralize!(plural_len);

match path {
MustUsePath::Suppressed => {}
MustUsePath::Boxed(path) => {
let descr_pre = &format!("{}boxed ", descr_pre);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len, true);
}
MustUsePath::Opaque(path) => {
let descr_pre = &format!("{}implementer{} of ", descr_pre, plural_suffix);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len, true);
}
MustUsePath::TraitObject(path) => {
let descr_post = &format!(" trait object{}{}", plural_suffix, descr_post);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len, true);
}
MustUsePath::TupleElement(elems) => {
for (index, path) in elems {
let descr_post = &format!(" in tuple element {}", index);
emit_must_use_untranslated(cx, path, descr_pre, descr_post, plural_len);
emit_must_use_untranslated(
cx, path, descr_pre, descr_post, plural_len, true,
);
}
}
MustUsePath::Array(path, len) => {
Expand All @@ -401,6 +405,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
descr_pre,
descr_post,
plural_len.saturating_add(usize::try_from(*len).unwrap_or(usize::MAX)),
true,
);
}
MustUsePath::Closure(span) => {
Expand All @@ -418,19 +423,6 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
);
}
MustUsePath::Def(span, def_id, reason) => {
let suggestion = if matches!(
cx.tcx.get_diagnostic_name(*def_id),
Some(sym::add)
| Some(sym::sub)
| Some(sym::mul)
| Some(sym::div)
| Some(sym::rem)
| Some(sym::neg),
) {
Some(UnusedDefSuggestion::Default { span: span.shrink_to_lo() })
} else {
None
};
cx.emit_spanned_lint(
UNUSED_MUST_USE,
*span,
Expand All @@ -440,7 +432,8 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
cx,
def_id: *def_id,
note: *reason,
suggestion,
suggestion: (!is_inner)
Copy link
Member

Choose a reason for hiding this comment

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

Why only for the outermost one and not nested ones as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let-statements can't exist in the expression position. We don't want (get_result(), other_fn(), ...) to become (let _ = get_result(), other_fn(), ...)

Copy link
Member

Choose a reason for hiding this comment

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

ah right, the spans

.then_some(UnusedDefSuggestion { span: span.shrink_to_lo() }),
},
);
}
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/conditional-compilation/cfg-attr-multi-true.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ note: the lint level is defined here
|
LL | #![warn(unused_must_use)]
| ^^^^^^^^^^^^^^^
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = MustUseDeprecated::new();
| +++++++

warning: 5 warnings emitted

28 changes: 28 additions & 0 deletions tests/ui/lint/fn_must_use.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,21 @@ note: the lint level is defined here
|
LL | #![warn(unused_must_use)]
| ^^^^^^^^^^^^^^^
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = need_to_use_this_value();
| +++++++

warning: unused return value of `MyStruct::need_to_use_this_method_value` that must be used
--> $DIR/fn_must_use.rs:60:5
|
LL | m.need_to_use_this_method_value();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = m.need_to_use_this_method_value();
| +++++++

warning: unused return value of `EvenNature::is_even` that must be used
--> $DIR/fn_must_use.rs:61:5
Expand All @@ -24,24 +33,43 @@ LL | m.is_even(); // trait method!
| ^^^^^^^^^^^
|
= note: no side effects
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = m.is_even(); // trait method!
| +++++++

warning: unused return value of `MyStruct::need_to_use_this_associated_function_value` that must be used
--> $DIR/fn_must_use.rs:64:5
|
LL | MyStruct::need_to_use_this_associated_function_value();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = MyStruct::need_to_use_this_associated_function_value();
| +++++++

warning: unused return value of `std::cmp::PartialEq::eq` that must be used
--> $DIR/fn_must_use.rs:70:5
|
LL | 2.eq(&3);
| ^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = 2.eq(&3);
| +++++++

warning: unused return value of `std::cmp::PartialEq::eq` that must be used
--> $DIR/fn_must_use.rs:71:5
|
LL | m.eq(&n);
| ^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = m.eq(&n);
| +++++++

warning: unused comparison that must be used
--> $DIR/fn_must_use.rs:74:5
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/lint/unused/must-use-box-from-raw.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ note: the lint level is defined here
|
LL | #![warn(unused_must_use)]
| ^^^^^^^^^^^^^^^
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = Box::from_raw(ptr);
| +++++++

warning: 1 warning emitted

9 changes: 9 additions & 0 deletions tests/ui/lint/unused/must_use-unit.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,21 @@ note: the lint level is defined here
|
LL | #![deny(unused_must_use)]
| ^^^^^^^^^^^^^^^
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo();
| +++++++

error: unused return value of `bar` that must be used
--> $DIR/must_use-unit.rs:15:5
|
LL | bar();
| ^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = bar();
| +++++++

error: aborting due to 2 previous errors

20 changes: 20 additions & 0 deletions tests/ui/lint/unused/unused-async.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,22 @@ error: unused return value of `foo` that must be used
|
LL | foo();
| ^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo();
| +++++++

error: unused output of future returned by `foo` that must be used
--> $DIR/unused-async.rs:33:5
|
LL | foo().await;
| ^^^^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo().await;
| +++++++

error: unused implementer of `Future` that must be used
--> $DIR/unused-async.rs:34:5
Expand All @@ -36,12 +46,22 @@ error: unused return value of `bar` that must be used
|
LL | bar();
| ^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = bar();
| +++++++

error: unused output of future returned by `bar` that must be used
--> $DIR/unused-async.rs:36:5
|
LL | bar().await;
| ^^^^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = bar().await;
| +++++++

error: unused implementer of `Future` that must be used
--> $DIR/unused-async.rs:37:5
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/lint/unused/unused-result.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ note: the lint level is defined here
|
LL | #![deny(unused_results, unused_must_use)]
| ^^^^^^^^^^^^^^^
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo::<MustUse>();
| +++++++

error: unused `MustUseMsg` that must be used
--> $DIR/unused-result.rs:22:5
Expand All @@ -17,6 +21,10 @@ LL | foo::<MustUseMsg>();
| ^^^^^^^^^^^^^^^^^^^
|
= note: some message
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo::<MustUseMsg>();
| +++++++

error: unused result of type `isize`
--> $DIR/unused-result.rs:34:5
Expand All @@ -35,6 +43,11 @@ error: unused `MustUse` that must be used
|
LL | foo::<MustUse>();
| ^^^^^^^^^^^^^^^^
|
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo::<MustUse>();
| +++++++

error: unused `MustUseMsg` that must be used
--> $DIR/unused-result.rs:36:5
Expand All @@ -43,6 +56,10 @@ LL | foo::<MustUseMsg>();
| ^^^^^^^^^^^^^^^^^^^
|
= note: some message
help: use `let _ = ...` to ignore the resulting value
|
LL | let _ = foo::<MustUseMsg>();
| +++++++

error: aborting due to 5 previous errors

Loading