Auto merge of #55663 - varkor:must_use-traits, r=estebank

bors · bors · commit 3991bfbbc212 · 2018-11-20T15:15:53.000Z
Allow #[must_use] on traits Addresses #55506, but we'll probably want to add it to some library traits like `Iterator` before the issue is considered fixed. Fixes #51560. `#[must_use]` is already permitted on traits, with no effect, so this seems like a bug fix, but I might be overlooking something. This currently warns for `impl Trait` or `dyn Trait` when the `Trait` is `#[must_use]` (although I don't think the latter is currently possible, so it's simply future-proofed).
diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs
@@ -60,18 +60,39 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
         }
 
         let t = cx.tables.expr_ty(&expr);
-        // FIXME(varkor): replace with `t.is_unit() || t.conservative_is_uninhabited()`.
-        let type_permits_no_use = match t.sty {
-            ty::Tuple(ref tys) if tys.is_empty() => true,
-            ty::Never => true,
-            ty::Adt(def, _) => {
-                if def.variants.is_empty() {
-                    true
-                } else {
-                    check_must_use(cx, def.did, s.span, "")
+        let type_permits_lack_of_use = if t.is_unit()
+            || cx.tcx.is_ty_uninhabited_from(cx.tcx.hir.get_module_parent(expr.id), t) {
+            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,
             }
-            _ => false,
         };
 
         let mut fn_warned = false;
@@ -98,8 +119,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
         };
         if let Some(def) = maybe_def {
             let def_id = def.def_id();
-            fn_warned = check_must_use(cx, def_id, s.span, "return value of ");
-        } else if type_permits_no_use {
+            fn_warned = check_must_use(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.)
             return;
@@ -148,15 +169,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
             op_warned = true;
         }
 
-        if !(type_permits_no_use || fn_warned || op_warned) {
+        if !(type_permits_lack_of_use || fn_warned || op_warned) {
             cx.span_lint(UNUSED_RESULTS, s.span, "unused result");
         }
 
-        fn check_must_use(cx: &LateContext, def_id: DefId, sp: Span, describe_path: &str) -> bool {
+        fn check_must_use(
+            cx: &LateContext,
+            def_id: DefId,
+            sp: Span,
+            descr_pre_path: &str,
+            descr_post_path: &str,
+        ) -> bool {
             for attr in cx.tcx.get_attrs(def_id).iter() {
                 if attr.check_name("must_use") {
-                    let msg = format!("unused {}`{}` that must be used",
-                                          describe_path, cx.tcx.item_path_str(def_id));
+                    let msg = format!("unused {}`{}`{} that must be used",
+                        descr_pre_path, cx.tcx.item_path_str(def_id), descr_post_path);
                     let mut err = cx.struct_span_lint(UNUSED_MUST_USE, sp, &msg);
                     // check for #[must_use = "..."]
                     if let Some(note) = attr.value_str() {
diff --git a/src/test/ui/lint/must_use-trait.rs b/src/test/ui/lint/must_use-trait.rs
@@ -0,0 +1,22 @@
+#![deny(unused_must_use)]
+
+#[must_use]
+trait Critical {}
+
+trait NotSoCritical {}
+
+trait DecidedlyUnimportant {}
+
+struct Anon;
+
+impl Critical for Anon {}
+impl NotSoCritical for Anon {}
+impl DecidedlyUnimportant for Anon {}
+
+fn get_critical() -> impl NotSoCritical + Critical + DecidedlyUnimportant {
+    Anon {}
+}
+
+fn main() {
+    get_critical(); //~ ERROR unused implementer of `Critical` that must be used
+}
diff --git a/src/test/ui/lint/must_use-trait.stderr b/src/test/ui/lint/must_use-trait.stderr
@@ -0,0 +1,14 @@
+error: unused implementer of `Critical` that must be used
+  --> $DIR/must_use-trait.rs:21:5
+   |
+LL |     get_critical(); //~ ERROR unused implementer of `Critical` that must be used
+   |     ^^^^^^^^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/must_use-trait.rs:1:9
+   |
+LL | #![deny(unused_must_use)]
+   |         ^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+
