From bbac81a0f11c21eb09344a1d9277393ffdba474e Mon Sep 17 00:00:00 2001 From: varkor Date: Thu, 23 May 2019 22:56:23 +0100 Subject: [PATCH 1/7] Warn for #[must_use] in tuples --- src/librustc_lint/unused.rs | 89 ++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 36 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 036820c6d7fa1..a765c0436a8c0 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -47,42 +47,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { return; } - let t = cx.tables.expr_ty(&expr); - let type_permits_lack_of_use = if t.is_unit() + let ty = cx.tables.expr_ty(&expr); + let type_permits_lack_of_use = if ty.is_unit() || cx.tcx.is_ty_uninhabited_from( - cx.tcx.hir().get_module_parent_by_hir_id(expr.hir_id), t) + cx.tcx.hir().get_module_parent_by_hir_id(expr.hir_id), ty) { true } else { - match t.sty { - ty::Adt(def, _) => check_must_use(cx, def.did, s.span, "", ""), - ty::Opaque(def, _) => { - let mut must_use = false; - for (predicate, _) in &cx.tcx.predicates_of(def).predicates { - if let ty::Predicate::Trait(ref poly_trait_predicate) = predicate { - let trait_ref = poly_trait_predicate.skip_binder().trait_ref; - if check_must_use(cx, trait_ref.def_id, s.span, "implementer of ", "") { - must_use = true; - break; - } - } - } - must_use - } - ty::Dynamic(binder, _) => { - let mut must_use = false; - for predicate in binder.skip_binder().iter() { - if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate { - if check_must_use(cx, trait_ref.def_id, s.span, "", " trait object") { - must_use = true; - break; - } - } - } - must_use - } - _ => false, - } + check_must_use_ty(cx, ty, s.span) }; let mut fn_warned = false; @@ -108,7 +80,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { _ => None }; if let Some(def_id) = maybe_def_id { - fn_warned = check_must_use(cx, def_id, s.span, "return value of ", ""); + fn_warned = check_must_use_def(cx, def_id, s.span, "return value of ", ""); } else if type_permits_lack_of_use { // We don't warn about unused unit or uninhabited types. // (See https://github.com/rust-lang/rust/issues/43806 for details.) @@ -162,10 +134,55 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { cx.span_lint(UNUSED_RESULTS, s.span, "unused result"); } - fn check_must_use( + // Returns whether an error has been emitted (and thus another does not need to be later). + fn check_must_use_ty( + cx: &LateContext<'_, '_>, + ty: ty::Ty<'_>, + span: Span, + ) -> bool { + match ty.sty { + ty::Adt(def, _) => check_must_use_def(cx, def.did, span, "", ""), + ty::Opaque(def, _) => { + let mut has_emitted = false; + for (predicate, _) in &cx.tcx.predicates_of(def).predicates { + if let ty::Predicate::Trait(ref poly_trait_predicate) = predicate { + let trait_ref = poly_trait_predicate.skip_binder().trait_ref; + let def_id = trait_ref.def_id; + if check_must_use_def(cx, def_id, span, "implementer of ", "") { + has_emitted = true; + break; + } + } + } + has_emitted + } + ty::Dynamic(binder, _) => { + let mut has_emitted = false; + for predicate in binder.skip_binder().iter() { + if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate { + let def_id = trait_ref.def_id; + if check_must_use_def(cx, def_id, span, "", " trait object") { + has_emitted = true; + break; + } + } + } + has_emitted + } + ty::Tuple(ref tys) => { + tys.iter().map(|k| k.expect_ty()).any(|ty| { + check_must_use_ty(cx, ty, span) + }) + } + _ => false, + } + } + + // Returns whether an error has been emitted (and thus another does not need to be later). + fn check_must_use_def( cx: &LateContext<'_, '_>, def_id: DefId, - sp: Span, + span: Span, descr_pre_path: &str, descr_post_path: &str, ) -> bool { @@ -173,7 +190,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { if attr.check_name(sym::must_use) { let msg = format!("unused {}`{}`{} that must be used", descr_pre_path, cx.tcx.def_path_str(def_id), descr_post_path); - let mut err = cx.struct_span_lint(UNUSED_MUST_USE, sp, &msg); + let mut err = cx.struct_span_lint(UNUSED_MUST_USE, span, &msg); // check for #[must_use = "..."] if let Some(note) = attr.value_str() { err.note(¬e.as_str()); From fd36b5fd52003d3c3b246c8fb9fb669c5f0f68b0 Mon Sep 17 00:00:00 2001 From: varkor Date: Thu, 23 May 2019 22:56:31 +0100 Subject: [PATCH 2/7] Add test for #[must_use] in tuples --- src/librustc_lint/unused.rs | 4 ++-- src/test/ui/lint/must_use-tuple.rs | 5 +++++ src/test/ui/lint/must_use-tuple.stderr | 15 +++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/lint/must_use-tuple.rs create mode 100644 src/test/ui/lint/must_use-tuple.stderr diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index a765c0436a8c0..67ea41e1bedad 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -1,7 +1,7 @@ use rustc::hir::def::{Res, DefKind}; use rustc::hir::def_id::DefId; use rustc::lint; -use rustc::ty; +use rustc::ty::{self, Ty}; use rustc::ty::adjustment; use rustc_data_structures::fx::FxHashMap; use lint::{LateContext, EarlyContext, LintContext, LintArray}; @@ -137,7 +137,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { // Returns whether an error has been emitted (and thus another does not need to be later). fn check_must_use_ty( cx: &LateContext<'_, '_>, - ty: ty::Ty<'_>, + ty: Ty<'_>, span: Span, ) -> bool { match ty.sty { diff --git a/src/test/ui/lint/must_use-tuple.rs b/src/test/ui/lint/must_use-tuple.rs new file mode 100644 index 0000000000000..bc972d7ffc391 --- /dev/null +++ b/src/test/ui/lint/must_use-tuple.rs @@ -0,0 +1,5 @@ +#![deny(unused_must_use)] + +fn main() { + (Ok::<(), ()>(()),); //~ ERROR unused `std::result::Result` that must be used +} diff --git a/src/test/ui/lint/must_use-tuple.stderr b/src/test/ui/lint/must_use-tuple.stderr new file mode 100644 index 0000000000000..67d1ec01966f9 --- /dev/null +++ b/src/test/ui/lint/must_use-tuple.stderr @@ -0,0 +1,15 @@ +error: unused `std::result::Result` that must be used + --> $DIR/must_use-tuple.rs:4:5 + | +LL | (Ok::<(), ()>(()),); + | ^^^^^^^^^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/must_use-tuple.rs:1:9 + | +LL | #![deny(unused_must_use)] + | ^^^^^^^^^^^^^^^ + = note: this `Result` may be an `Err` variant, which should be handled + +error: aborting due to previous error + From e121d9671afe4eae1f418db14a6fdae07652c51c Mon Sep 17 00:00:00 2001 From: varkor Date: Mon, 27 May 2019 16:48:43 +0100 Subject: [PATCH 3/7] Use precise span for must_use tuple components --- src/librustc_lint/unused.rs | 19 +++++++++++++++---- src/test/ui/lint/must_use-tuple.rs | 4 ++++ src/test/ui/lint/must_use-tuple.stderr | 22 +++++++++++++++++++--- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 67ea41e1bedad..a2bf0b894f6c1 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -54,7 +54,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { { true } else { - check_must_use_ty(cx, ty, s.span) + check_must_use_ty(cx, ty, &expr, s.span) }; let mut fn_warned = false; @@ -138,6 +138,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { fn check_must_use_ty( cx: &LateContext<'_, '_>, ty: Ty<'_>, + expr: &hir::Expr, span: Span, ) -> bool { match ty.sty { @@ -170,9 +171,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { has_emitted } ty::Tuple(ref tys) => { - tys.iter().map(|k| k.expect_ty()).any(|ty| { - check_must_use_ty(cx, ty, span) - }) + let mut has_emitted = false; + let spans = if let hir::ExprKind::Tup(comps) = &expr.node { + debug_assert_eq!(comps.len(), tys.len()); + comps.iter().map(|e| e.span).collect() + } else { + vec![] + }; + for (i, ty) in tys.iter().map(|k| k.expect_ty()).enumerate() { + if check_must_use_ty(cx, ty, expr, *spans.get(i).unwrap_or(&span)) { + has_emitted = true; + } + } + has_emitted } _ => false, } diff --git a/src/test/ui/lint/must_use-tuple.rs b/src/test/ui/lint/must_use-tuple.rs index bc972d7ffc391..2cdcfef35f7a6 100644 --- a/src/test/ui/lint/must_use-tuple.rs +++ b/src/test/ui/lint/must_use-tuple.rs @@ -2,4 +2,8 @@ fn main() { (Ok::<(), ()>(()),); //~ ERROR unused `std::result::Result` that must be used + + (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); + //~^ ERROR unused `std::result::Result` that must be used + //~^^ ERROR unused `std::result::Result` that must be used } diff --git a/src/test/ui/lint/must_use-tuple.stderr b/src/test/ui/lint/must_use-tuple.stderr index 67d1ec01966f9..59cdf5849f757 100644 --- a/src/test/ui/lint/must_use-tuple.stderr +++ b/src/test/ui/lint/must_use-tuple.stderr @@ -1,8 +1,8 @@ error: unused `std::result::Result` that must be used - --> $DIR/must_use-tuple.rs:4:5 + --> $DIR/must_use-tuple.rs:4:6 | LL | (Ok::<(), ()>(()),); - | ^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^ | note: lint level defined here --> $DIR/must_use-tuple.rs:1:9 @@ -11,5 +11,21 @@ LL | #![deny(unused_must_use)] | ^^^^^^^^^^^^^^^ = note: this `Result` may be an `Err` variant, which should be handled -error: aborting due to previous error +error: unused `std::result::Result` that must be used + --> $DIR/must_use-tuple.rs:6:6 + | +LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); + | ^^^^^^^^^^^^^^^^ + | + = note: this `Result` may be an `Err` variant, which should be handled + +error: unused `std::result::Result` that must be used + --> $DIR/must_use-tuple.rs:6:27 + | +LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); + | ^^^^^^^^^^^^^^^^ + | + = note: this `Result` may be an `Err` variant, which should be handled + +error: aborting due to 3 previous errors From 3c768ade4d7c18db873c201a8aebda0f9c243a30 Mon Sep 17 00:00:00 2001 From: varkor Date: Mon, 27 May 2019 20:11:15 +0100 Subject: [PATCH 4/7] Fix issue with recursively encountering uninhabited type --- src/librustc_lint/unused.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index a2bf0b894f6c1..bbec42b238f46 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -48,14 +48,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { } let ty = cx.tables.expr_ty(&expr); - let type_permits_lack_of_use = if ty.is_unit() - || cx.tcx.is_ty_uninhabited_from( - cx.tcx.hir().get_module_parent_by_hir_id(expr.hir_id), ty) - { - true - } else { - check_must_use_ty(cx, ty, &expr, s.span) - }; + let type_permits_lack_of_use = check_must_use_ty(cx, ty, &expr, s.span); let mut fn_warned = false; let mut op_warned = false; @@ -135,12 +128,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { } // Returns whether an error has been emitted (and thus another does not need to be later). - fn check_must_use_ty( - cx: &LateContext<'_, '_>, - ty: Ty<'_>, + fn check_must_use_ty<'tcx>( + cx: &LateContext<'_, 'tcx>, + ty: Ty<'tcx>, expr: &hir::Expr, span: Span, ) -> bool { + if ty.is_unit() || cx.tcx.is_ty_uninhabited_from( + cx.tcx.hir().get_module_parent_by_hir_id(expr.hir_id), ty) + { + return true; + } + match ty.sty { ty::Adt(def, _) => check_must_use_def(cx, def.did, span, "", ""), ty::Opaque(def, _) => { From 058551c4fdb067a642e096e7c50ea82e3fd09e7c Mon Sep 17 00:00:00 2001 From: varkor Date: Mon, 27 May 2019 20:38:13 +0100 Subject: [PATCH 5/7] Add function call to test --- src/test/ui/lint/must_use-tuple.rs | 6 ++++++ src/test/ui/lint/must_use-tuple.stderr | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/test/ui/lint/must_use-tuple.rs b/src/test/ui/lint/must_use-tuple.rs index 2cdcfef35f7a6..3091dbbdf9860 100644 --- a/src/test/ui/lint/must_use-tuple.rs +++ b/src/test/ui/lint/must_use-tuple.rs @@ -1,9 +1,15 @@ #![deny(unused_must_use)] +fn foo() -> Result<(), ()> { + Ok::<(), ()>(()) +} + fn main() { (Ok::<(), ()>(()),); //~ ERROR unused `std::result::Result` that must be used (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); //~^ ERROR unused `std::result::Result` that must be used //~^^ ERROR unused `std::result::Result` that must be used + + foo(); //~ ERROR unused `std::result::Result` that must be used } diff --git a/src/test/ui/lint/must_use-tuple.stderr b/src/test/ui/lint/must_use-tuple.stderr index 59cdf5849f757..dee53e70edd21 100644 --- a/src/test/ui/lint/must_use-tuple.stderr +++ b/src/test/ui/lint/must_use-tuple.stderr @@ -1,5 +1,5 @@ error: unused `std::result::Result` that must be used - --> $DIR/must_use-tuple.rs:4:6 + --> $DIR/must_use-tuple.rs:8:6 | LL | (Ok::<(), ()>(()),); | ^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | #![deny(unused_must_use)] = note: this `Result` may be an `Err` variant, which should be handled error: unused `std::result::Result` that must be used - --> $DIR/must_use-tuple.rs:6:6 + --> $DIR/must_use-tuple.rs:10:6 | LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); | ^^^^^^^^^^^^^^^^ @@ -20,12 +20,20 @@ LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); = note: this `Result` may be an `Err` variant, which should be handled error: unused `std::result::Result` that must be used - --> $DIR/must_use-tuple.rs:6:27 + --> $DIR/must_use-tuple.rs:10:27 | LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); | ^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled -error: aborting due to 3 previous errors +error: unused `std::result::Result` that must be used + --> $DIR/must_use-tuple.rs:14:5 + | +LL | foo(); + | ^^^^^^ + | + = note: this `Result` may be an `Err` variant, which should be handled + +error: aborting due to 4 previous errors From 81fa794af9c515eb07a1b6956dc048473770b8ae Mon Sep 17 00:00:00 2001 From: varkor Date: Tue, 28 May 2019 20:12:48 +0100 Subject: [PATCH 6/7] Specify tuple element in lint message --- src/librustc_lint/unused.rs | 9 ++++++--- src/test/ui/lint/must_use-tuple.rs | 12 ++++++------ src/test/ui/lint/must_use-tuple.stderr | 8 ++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index bbec42b238f46..d540b3f7e40a3 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -48,7 +48,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { } let ty = cx.tables.expr_ty(&expr); - let type_permits_lack_of_use = check_must_use_ty(cx, ty, &expr, s.span); + let type_permits_lack_of_use = check_must_use_ty(cx, ty, &expr, s.span, ""); let mut fn_warned = false; let mut op_warned = false; @@ -133,6 +133,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { ty: Ty<'tcx>, expr: &hir::Expr, span: Span, + descr_post_path: &str, ) -> bool { if ty.is_unit() || cx.tcx.is_ty_uninhabited_from( cx.tcx.hir().get_module_parent_by_hir_id(expr.hir_id), ty) @@ -141,7 +142,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { } match ty.sty { - ty::Adt(def, _) => check_must_use_def(cx, def.did, span, "", ""), + ty::Adt(def, _) => check_must_use_def(cx, def.did, span, "", descr_post_path), ty::Opaque(def, _) => { let mut has_emitted = false; for (predicate, _) in &cx.tcx.predicates_of(def).predicates { @@ -178,7 +179,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { vec![] }; for (i, ty) in tys.iter().map(|k| k.expect_ty()).enumerate() { - if check_must_use_ty(cx, ty, expr, *spans.get(i).unwrap_or(&span)) { + let descr_post_path = &format!(" in tuple element {}", i); + let span = *spans.get(i).unwrap_or(&span); + if check_must_use_ty(cx, ty, expr, span, descr_post_path) { has_emitted = true; } } diff --git a/src/test/ui/lint/must_use-tuple.rs b/src/test/ui/lint/must_use-tuple.rs index 3091dbbdf9860..efbe0ff9c6c9d 100644 --- a/src/test/ui/lint/must_use-tuple.rs +++ b/src/test/ui/lint/must_use-tuple.rs @@ -1,15 +1,15 @@ #![deny(unused_must_use)] -fn foo() -> Result<(), ()> { - Ok::<(), ()>(()) +fn foo() -> (Result<(), ()>, ()) { + (Ok::<(), ()>(()), ()) } fn main() { - (Ok::<(), ()>(()),); //~ ERROR unused `std::result::Result` that must be used + (Ok::<(), ()>(()),); //~ ERROR unused `std::result::Result` (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); - //~^ ERROR unused `std::result::Result` that must be used - //~^^ ERROR unused `std::result::Result` that must be used + //~^ ERROR unused `std::result::Result` + //~^^ ERROR unused `std::result::Result` - foo(); //~ ERROR unused `std::result::Result` that must be used + foo(); //~ ERROR unused `std::result::Result` } diff --git a/src/test/ui/lint/must_use-tuple.stderr b/src/test/ui/lint/must_use-tuple.stderr index dee53e70edd21..4efcb8703aaa6 100644 --- a/src/test/ui/lint/must_use-tuple.stderr +++ b/src/test/ui/lint/must_use-tuple.stderr @@ -1,4 +1,4 @@ -error: unused `std::result::Result` that must be used +error: unused `std::result::Result` in tuple element 0 that must be used --> $DIR/must_use-tuple.rs:8:6 | LL | (Ok::<(), ()>(()),); @@ -11,7 +11,7 @@ LL | #![deny(unused_must_use)] | ^^^^^^^^^^^^^^^ = note: this `Result` may be an `Err` variant, which should be handled -error: unused `std::result::Result` that must be used +error: unused `std::result::Result` in tuple element 0 that must be used --> $DIR/must_use-tuple.rs:10:6 | LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); @@ -19,7 +19,7 @@ LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); | = note: this `Result` may be an `Err` variant, which should be handled -error: unused `std::result::Result` that must be used +error: unused `std::result::Result` in tuple element 2 that must be used --> $DIR/must_use-tuple.rs:10:27 | LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); @@ -27,7 +27,7 @@ LL | (Ok::<(), ()>(()), 0, Ok::<(), ()>(()), 5); | = note: this `Result` may be an `Err` variant, which should be handled -error: unused `std::result::Result` that must be used +error: unused `std::result::Result` in tuple element 0 that must be used --> $DIR/must_use-tuple.rs:14:5 | LL | foo(); From de2bf3a761db4510c7aae109ada58acbbc450fa7 Mon Sep 17 00:00:00 2001 From: varkor Date: Mon, 3 Jun 2019 18:50:32 +0100 Subject: [PATCH 7/7] Add nested must_use variant --- src/test/ui/lint/must_use-tuple.rs | 2 ++ src/test/ui/lint/must_use-tuple.stderr | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/test/ui/lint/must_use-tuple.rs b/src/test/ui/lint/must_use-tuple.rs index efbe0ff9c6c9d..f6b579a7f35cf 100644 --- a/src/test/ui/lint/must_use-tuple.rs +++ b/src/test/ui/lint/must_use-tuple.rs @@ -12,4 +12,6 @@ fn main() { //~^^ ERROR unused `std::result::Result` foo(); //~ ERROR unused `std::result::Result` + + ((Err::<(), ()>(()), ()), ()); //~ ERROR unused `std::result::Result` } diff --git a/src/test/ui/lint/must_use-tuple.stderr b/src/test/ui/lint/must_use-tuple.stderr index 4efcb8703aaa6..45d2a439e52b0 100644 --- a/src/test/ui/lint/must_use-tuple.stderr +++ b/src/test/ui/lint/must_use-tuple.stderr @@ -35,5 +35,13 @@ LL | foo(); | = note: this `Result` may be an `Err` variant, which should be handled -error: aborting due to 4 previous errors +error: unused `std::result::Result` in tuple element 0 that must be used + --> $DIR/must_use-tuple.rs:16:6 + | +LL | ((Err::<(), ()>(()), ()), ()); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this `Result` may be an `Err` variant, which should be handled + +error: aborting due to 5 previous errors