Auto merge of #62262 - varkor:must_use-adt-components-ii, r=<try>

bors · bors · commit acb4fefbf40e · 2019-07-22T21:11:19.000Z
Extend `#[must_use]` to nested structures Extends the `#[must_use]` lint to apply when `#[must_use]` types are nested within `struct`s (or one-variant `enum`s), making the lint much more generally useful. This is in line with #61100 extending the lint to tuples. Fixes #39524. cc @rust-lang/lang and @rust-lang/compiler for discussion in case this is a controversial change. In particular, we might want to consider allowing annotations on fields containing `#[must_use]` types in user-defined types (e.g. `#[allow(unused_must_use)]`) to opt out of this behaviour, if there are cases where we this this is likely to have frequent false positives. (This is based on top of #62235.)
diff --git a/src/liballoc/string.rs b/src/liballoc/string.rs
@@ -1571,7 +1571,7 @@ impl String {
              Unbounded => {},
         };
 
-        unsafe {
+        let _ = unsafe {
             self.as_mut_vec()
         }.splice(range, replace_with.bytes());
     }
diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs
@@ -1948,7 +1948,7 @@ pub struct FieldDef {
 pub struct AdtDef {
     /// `DefId` of the struct, enum or union item.
     pub did: DefId,
-    /// Variants of the ADT. If this is a struct or enum, then there will be a single variant.
+    /// Variants of the ADT. If this is a struct or union, then there will be a single variant.
     pub variants: IndexVec<self::layout::VariantIdx, VariantDef>,
     /// Flags of the ADT (e.g. is this a struct? is this non-exhaustive?)
     flags: AdtFlags,
diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs
@@ -1,8 +1,11 @@
 use rustc::hir::def::{Res, DefKind};
 use rustc::hir::def_id::DefId;
+use rustc::hir::HirVec;
 use rustc::lint;
 use rustc::ty::{self, Ty};
+use rustc::ty::subst::Subst;
 use rustc::ty::adjustment;
+use rustc::mir::interpret::{GlobalId, ConstValue};
 use rustc_data_structures::fx::FxHashMap;
 use lint::{LateContext, EarlyContext, LintContext, LintArray};
 use lint::{LintPass, EarlyLintPass, LateLintPass};
@@ -23,7 +26,7 @@ use log::debug;
 
 declare_lint! {
     pub UNUSED_MUST_USE,
-    Warn,
+    Deny,
     "unused result of a type flagged as `#[must_use]`",
     report_in_external_macro: true
 }
@@ -151,8 +154,40 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
                     let descr_pre = &format!("{}boxed ", descr_pre);
                     check_must_use_ty(cx, boxed_ty, expr, span, descr_pre, descr_post, plural)
                 }
-                ty::Adt(def, _) => {
-                    check_must_use_def(cx, def.did, span, descr_pre, descr_post)
+                ty::Adt(def, subst) => {
+                    // Check the type itself for `#[must_use]` annotations.
+                    let mut has_emitted = check_must_use_def(
+                        cx, def.did, span, descr_pre, descr_post);
+                    // Check any fields of the type for `#[must_use]` annotations.
+                    // We ignore ADTs with more than one variant for simplicity and to avoid
+                    // false positives.
+                    // Unions are also ignored (though in theory, we could lint if every field of
+                    // a union was `#[must_use]`).
+                    if def.variants.len() == 1 && !def.is_union() {
+                        let fields = match &expr.node {
+                            hir::ExprKind::Struct(_, fields, _) => {
+                                fields.iter().map(|f| &*f.expr).collect()
+                            }
+                            hir::ExprKind::Call(_, args) => args.iter().collect(),
+                            _ => HirVec::new(),
+                        };
+
+                        for variant in &def.variants {
+                            for (i, field) in variant.fields.iter().enumerate() {
+                                let descr_post
+                                    = &format!(" in field `{}`", field.ident.as_str());
+                                let ty = cx.tcx.type_of(field.did).subst(cx.tcx, subst);
+                                let (expr, span) = if let Some(&field) = fields.get(i) {
+                                    (field, field.span)
+                                } else {
+                                    (expr, span)
+                                };
+                                has_emitted |= check_must_use_ty(
+                                    cx, ty, expr, span, descr_pre, descr_post, plural);
+                            }
+                        }
+                    }
+                    has_emitted
                 }
                 ty::Opaque(def, _) => {
                     let mut has_emitted = false;
@@ -202,24 +237,43 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
                     for (i, ty) in tys.iter().map(|k| k.expect_ty()).enumerate() {
                         let descr_post = &format!(" in tuple element {}", i);
                         let span = *spans.get(i).unwrap_or(&span);
-                        if check_must_use_ty(cx, ty, expr, span, descr_pre, descr_post, plural) {
-                            has_emitted = true;
-                        }
+                        has_emitted |= check_must_use_ty(
+                            cx, ty, expr, span, descr_pre, descr_post, plural);
                     }
                     has_emitted
                 }
-                ty::Array(ty, len) => match len.assert_usize(cx.tcx) {
-                    // If the array is definitely non-empty, we can do `#[must_use]` checking.
-                    Some(n) if n != 0 => {
-                        let descr_pre = &format!(
-                            "{}array{} of ",
-                            descr_pre,
-                            plural_suffix,
-                        );
-                        check_must_use_ty(cx, ty, expr, span, descr_pre, descr_post, true)
+                ty::Array(ty, mut len) => {
+                    // Try to evaluate the length if it's unevaluated.
+                    // FIXME(59369): we should be able to remove this once we merge
+                    // https://github.com/rust-lang/rust/pull/59369.
+                    if let ConstValue::Unevaluated(def_id, substs) = len.val {
+                        let instance = ty::Instance::resolve(
+                            cx.tcx.global_tcx(),
+                            cx.param_env,
+                            def_id,
+                            substs,
+                        ).unwrap();
+                        let global_id = GlobalId {
+                            instance,
+                            promoted: None
+                        };
+                        if let Ok(ct) = cx.tcx.const_eval(cx.param_env.and(global_id)) {
+                            len = ct;
+                        }
+                    }
+
+                    match len.assert_usize(cx.tcx) {
+                        Some(0) => false, // Empty arrays won't contain any `#[must_use]` types.
+                        // If the array may be non-empty, we do `#[must_use]` checking.
+                        _ => {
+                            let descr_pre = &format!(
+                                "{}array{} of ",
+                                descr_pre,
+                                plural_suffix,
+                            );
+                            check_must_use_ty(cx, ty, expr, span, descr_pre, descr_post, true)
+                        }
                     }
-                    // Otherwise, we don't lint, to avoid false positives.
-                    _ => false,
                 }
                 _ => false,
             }
diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs
@@ -103,9 +103,7 @@ pub fn set_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>) {
         HOOK_LOCK.write_unlock();
 
         if let Hook::Custom(ptr) = old_hook {
-            #[allow(unused_must_use)] {
-                Box::from_raw(ptr);
-            }
+            mem::drop(Box::from_raw(ptr));
         }
     }
 }
diff --git a/src/test/ui/lint/must_use-adt.rs b/src/test/ui/lint/must_use-adt.rs
@@ -0,0 +1,77 @@
+#![deny(unused_must_use)]
+
+#[must_use]
+struct S;
+
+#[must_use]
+trait A {}
+
+struct B;
+
+impl A for B {}
+
+struct T(S);
+
+struct U {
+    x: (),
+    y: T,
+}
+
+struct V {
+    a: S,
+}
+
+struct W {
+    w: [(u8, Box<dyn A>); 2],
+    x: u32,
+    y: (B, B),
+    z: (S, S),
+    e: [(u8, Box<dyn A>); 2],
+    f: S,
+}
+
+fn get_v() -> V {
+    V { a: S }
+}
+
+struct Z([(u8, Box<dyn A>); 2]);
+
+fn get_wrapped_arr() -> Z {
+    Z([(0, Box::new(B)), (0, Box::new(B))])
+}
+
+fn get_tuple_arr() -> ([(u8, Box<dyn A>); 2],) {
+    ([(0, Box::new(B)), (0, Box::new(B))],)
+}
+
+struct R<T> {
+    r: T
+}
+
+struct List<T>(T, Option<Box<Self>>);
+
+fn main() {
+    S; //~ ERROR unused `S` that must be used
+    T(S); //~ ERROR unused `S` in field `0` that must be used
+    U { x: (), y: T(S) }; //~ ERROR unused `S` in field `0` that must be used
+    get_v(); //~ ERROR unused `S` in field `a` that must be used
+    V { a: S }; //~ ERROR unused `S` in field `a` that must be used
+    W {
+        w: [(0, Box::new(B)), (0, Box::new(B))],
+        //~^ ERROR unused array of boxed `A` trait objects in tuple element 1 that must be used
+        x: 0,
+        y: (B, B),
+        z: (S, S),
+        //~^ unused `S` in tuple element 0 that must be used
+        //~^^ unused `S` in tuple element 1 that must be used
+        e: [(0, Box::new(B)), (0, Box::new(B))],
+        //~^ unused array of boxed `A` trait objects in tuple element 1 that must be used
+        f: S, //~ ERROR unused `S` in field `f` that must be used
+    };
+    get_wrapped_arr();
+    //~^ ERROR unused array of boxed `A` trait objects in tuple element 1 that must be use
+    get_tuple_arr();
+    //~^ ERROR unused array of boxed `A` trait objects in tuple element 1 that must be used
+    R { r: S }; //~ ERROR unused `S` in field `r` that must be used
+    List(S, Some(Box::new(List(S, None)))); //~ ERROR unused `S` in field `0` that must be used
+}
diff --git a/src/test/ui/lint/must_use-adt.stderr b/src/test/ui/lint/must_use-adt.stderr
@@ -0,0 +1,92 @@
+error: unused `S` that must be used
+  --> $DIR/must_use-adt.rs:54:5
+   |
+LL |     S;
+   |     ^^
+   |
+note: lint level defined here
+  --> $DIR/must_use-adt.rs:1:9
+   |
+LL | #![deny(unused_must_use)]
+   |         ^^^^^^^^^^^^^^^
+
+error: unused `S` in field `0` that must be used
+  --> $DIR/must_use-adt.rs:55:7
+   |
+LL |     T(S);
+   |       ^
+
+error: unused `S` in field `0` that must be used
+  --> $DIR/must_use-adt.rs:56:21
+   |
+LL |     U { x: (), y: T(S) };
+   |                     ^
+
+error: unused `S` in field `a` that must be used
+  --> $DIR/must_use-adt.rs:57:5
+   |
+LL |     get_v();
+   |     ^^^^^^^^
+
+error: unused `S` in field `a` that must be used
+  --> $DIR/must_use-adt.rs:58:12
+   |
+LL |     V { a: S };
+   |            ^
+
+error: unused array of boxed `A` trait objects in tuple element 1 that must be used
+  --> $DIR/must_use-adt.rs:60:12
+   |
+LL |         w: [(0, Box::new(B)), (0, Box::new(B))],
+   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: unused `S` in tuple element 0 that must be used
+  --> $DIR/must_use-adt.rs:64:13
+   |
+LL |         z: (S, S),
+   |             ^
+
+error: unused `S` in tuple element 1 that must be used
+  --> $DIR/must_use-adt.rs:64:16
+   |
+LL |         z: (S, S),
+   |                ^
+
+error: unused array of boxed `A` trait objects in tuple element 1 that must be used
+  --> $DIR/must_use-adt.rs:67:12
+   |
+LL |         e: [(0, Box::new(B)), (0, Box::new(B))],
+   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: unused `S` in field `f` that must be used
+  --> $DIR/must_use-adt.rs:69:12
+   |
+LL |         f: S,
+   |            ^
+
+error: unused array of boxed `A` trait objects in tuple element 1 that must be used
+  --> $DIR/must_use-adt.rs:71:5
+   |
+LL |     get_wrapped_arr();
+   |     ^^^^^^^^^^^^^^^^^^
+
+error: unused array of boxed `A` trait objects in tuple element 1 that must be used
+  --> $DIR/must_use-adt.rs:73:5
+   |
+LL |     get_tuple_arr();
+   |     ^^^^^^^^^^^^^^^^
+
+error: unused `S` in field `r` that must be used
+  --> $DIR/must_use-adt.rs:75:12
+   |
+LL |     R { r: S };
+   |            ^
+
+error: unused `S` in field `0` that must be used
+  --> $DIR/must_use-adt.rs:76:10
+   |
+LL |     List(S, Some(Box::new(List(S, None))));
+   |          ^
+
+error: aborting due to 14 previous errors
+

Original file line number	Diff line number	Diff line change
`@@ -1571,7 +1571,7 @@ impl String {`
`1571`	`1571`	`Unbounded => {},`
`1572`	`1572`	`};`
`1573`	`1573`
`1574`		`- unsafe {`
	`1574`	`+ let _ = unsafe {`
`1575`	`1575`	`self.as_mut_vec()`
`1576`	`1576`	`}.splice(range, replace_with.bytes());`
`1577`	`1577`	`}`
Original file line number	Diff line number	Diff line change
`@@ -103,9 +103,7 @@ pub fn set_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>) {`
`103`	`103`	`HOOK_LOCK.write_unlock();`
`104`	`104`
`105`	`105`	`if let Hook::Custom(ptr) = old_hook {`
`106`		`- #[allow(unused_must_use)] {`
`107`		`- Box::from_raw(ptr);`
`108`		`- }`
	`106`	`+ mem::drop(Box::from_raw(ptr));`
`109`	`107`	`}`
`110`	`108`	`}`
`111`	`109`	`}`