Auto merge of #65077 - estebank:mut-trait-expected, r=nikomatsakis

Note when a mutable trait object is needed

Fix #63619, fix #37914. CC #64068.

Author: bors

src/librustc/hir/mod.rs

@@ -1053,6 +1053,13 @@ impl Mutability {
             MutImmutable => MutImmutable,
         }
     }
+
+    pub fn invert(self) -> Self {
+        match self {
+            MutMutable => MutImmutable,
+            MutImmutable => MutMutable,
+        }
+    }
 }
 
 #[derive(Copy, Clone, PartialEq, RustcEncodable, RustcDecodable, Debug, Hash, HashStable)]

src/librustc/traits/error_reporting.rs

@@ -453,21 +453,17 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         }
     }
 
-    fn find_similar_impl_candidates(&self,
-                                    trait_ref: ty::PolyTraitRef<'tcx>)
-                                    -> Vec<ty::TraitRef<'tcx>>
-    {
-        let simp = fast_reject::simplify_type(self.tcx,
-                                              trait_ref.skip_binder().self_ty(),
-                                              true);
+    fn find_similar_impl_candidates(
+        &self,
+        trait_ref: ty::PolyTraitRef<'tcx>,
+    ) -> Vec<ty::TraitRef<'tcx>> {
+        let simp = fast_reject::simplify_type(self.tcx, trait_ref.skip_binder().self_ty(), true);
         let all_impls = self.tcx.all_impls(trait_ref.def_id());
 
         match simp {
             Some(simp) => all_impls.iter().filter_map(|&def_id| {
                 let imp = self.tcx.impl_trait_ref(def_id).unwrap();
-                let imp_simp = fast_reject::simplify_type(self.tcx,
-                                                          imp.self_ty(),
-                                                          true);
+                let imp_simp = fast_reject::simplify_type(self.tcx, imp.self_ty(), true);
                 if let Some(imp_simp) = imp_simp {
                     if simp != imp_simp {
                         return None
@@ -482,10 +478,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         }
     }
 
-    fn report_similar_impl_candidates(&self,
-                                      impl_candidates: Vec<ty::TraitRef<'tcx>>,
-                                      err: &mut DiagnosticBuilder<'_>)
-    {
+    fn report_similar_impl_candidates(
+        &self,
+        impl_candidates: Vec<ty::TraitRef<'tcx>>,
+        err: &mut DiagnosticBuilder<'_>,
+    ) {
         if impl_candidates.is_empty() {
             return;
         }
@@ -720,10 +717,18 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                             // which is somewhat confusing.
                             err.help(&format!("consider adding a `where {}` bound",
                                               trait_ref.to_predicate()));
-                        } else if !have_alt_message {
-                            // Can't show anything else useful, try to find similar impls.
-                            let impl_candidates = self.find_similar_impl_candidates(trait_ref);
-                            self.report_similar_impl_candidates(impl_candidates, &mut err);
+                        } else {
+                            if !have_alt_message {
+                                // Can't show anything else useful, try to find similar impls.
+                                let impl_candidates = self.find_similar_impl_candidates(trait_ref);
+                                self.report_similar_impl_candidates(impl_candidates, &mut err);
+                            }
+                            self.suggest_change_mut(
+                                &obligation,
+                                &mut err,
+                                &trait_ref,
+                                points_at_arg,
+                            );
                         }
 
                         // If this error is due to `!: Trait` not implemented but `(): Trait` is
@@ -1081,9 +1086,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
 
                     let substs = self.tcx.mk_substs_trait(trait_type, &[]);
                     let new_trait_ref = ty::TraitRef::new(trait_ref.def_id, substs);
-                    let new_obligation = Obligation::new(ObligationCause::dummy(),
-                                                         obligation.param_env,
-                                                         new_trait_ref.to_predicate());
+                    let new_obligation = Obligation::new(
+                        ObligationCause::dummy(),
+                        obligation.param_env,
+                        new_trait_ref.to_predicate(),
+                    );
 
                     if self.predicate_may_hold(&new_obligation) {
                         let sp = self.tcx.sess.source_map()
@@ -1105,6 +1112,77 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         }
     }
 
+    /// Check if the trait bound is implemented for a different mutability and note it in the
+    /// final error.
+    fn suggest_change_mut(
+        &self,
+        obligation: &PredicateObligation<'tcx>,
+        err: &mut DiagnosticBuilder<'tcx>,
+        trait_ref: &ty::Binder<ty::TraitRef<'tcx>>,
+        points_at_arg: bool,
+    ) {
+        let span = obligation.cause.span;
+        if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
+            let refs_number = snippet.chars()
+                .filter(|c| !c.is_whitespace())
+                .take_while(|c| *c == '&')
+                .count();
+            if let Some('\'') = snippet.chars()
+                .filter(|c| !c.is_whitespace())
+                .skip(refs_number)
+                .next()
+            { // Do not suggest removal of borrow from type arguments.
+                return;
+            }
+            let trait_ref = self.resolve_vars_if_possible(trait_ref);
+            if trait_ref.has_infer_types() {
+                // Do not ICE while trying to find if a reborrow would succeed on a trait with
+                // unresolved bindings.
+                return;
+            }
+
+            if let ty::Ref(region, t_type, mutability) = trait_ref.skip_binder().self_ty().kind {
+                let trait_type = match mutability {
+                    hir::Mutability::MutMutable => self.tcx.mk_imm_ref(region, t_type),
+                    hir::Mutability::MutImmutable => self.tcx.mk_mut_ref(region, t_type),
+                };
+
+                let substs = self.tcx.mk_substs_trait(&trait_type, &[]);
+                let new_trait_ref = ty::TraitRef::new(trait_ref.skip_binder().def_id, substs);
+                let new_obligation = Obligation::new(
+                    ObligationCause::dummy(),
+                    obligation.param_env,
+                    new_trait_ref.to_predicate(),
+                );
+
+                if self.evaluate_obligation_no_overflow(
+                    &new_obligation,
+                ).must_apply_modulo_regions() {
+                    let sp = self.tcx.sess.source_map()
+                        .span_take_while(span, |c| c.is_whitespace() || *c == '&');
+                    if points_at_arg &&
+                        mutability == hir::Mutability::MutImmutable &&
+                        refs_number > 0
+                    {
+                        err.span_suggestion(
+                            sp,
+                            "consider changing this borrow's mutability",
+                            "&mut ".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    } else {
+                        err.note(&format!(
+                            "`{}` is implemented for `{:?}`, but not for `{:?}`",
+                            trait_ref,
+                            trait_type,
+                            trait_ref.skip_binder().self_ty(),
+                        ));
+                    }
+                }
+            }
+        }
+    }
+
     fn suggest_semicolon_removal(
         &self,
         obligation: &PredicateObligation<'tcx>,

src/librustc/traits/query/evaluate_obligation.rs

@@ -56,7 +56,7 @@ impl<'cx, 'tcx> InferCtxt<'cx, 'tcx> {
     // Helper function that canonicalizes and runs the query. If an
     // overflow results, we re-run it in the local context so we can
     // report a nice error.
-    fn evaluate_obligation_no_overflow(
+    crate fn evaluate_obligation_no_overflow(
         &self,
         obligation: &PredicateObligation<'tcx>,
     ) -> EvaluationResult {

src/librustc_typeck/check/method/mod.rs

@@ -58,7 +58,7 @@ pub enum MethodError<'tcx> {
 
     // Found a `Self: Sized` bound where `Self` is a trait object, also the caller may have
     // forgotten to import a trait.
-    IllegalSizedBound(Vec<DefId>),
+    IllegalSizedBound(Vec<DefId>, bool),
 
     // Found a match, but the return type is wrong
     BadReturnType,
@@ -213,33 +213,49 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             segment,
         );
 
+        let mut needs_mut = false;
+        if let ty::Ref(region, t_type, mutability) = self_ty.kind {
+            let trait_type = self.tcx.mk_ref(region, ty::TypeAndMut {
+                ty: t_type,
+                mutbl: mutability.invert(),
+            });
+            match self.lookup_probe(
+                span,
+                segment.ident,
+                trait_type,
+                call_expr,
+                ProbeScope::TraitsInScope
+            ) {
+                Ok(ref new_pick) if *new_pick != pick => {
+                    needs_mut = true;
+                }
+                _ => {}
+            }
+        }
+
         if result.illegal_sized_bound {
             // We probe again, taking all traits into account (not only those in scope).
-            let candidates =
-                match self.lookup_probe(span,
-                                        segment.ident,
-                                        self_ty,
-                                        call_expr,
-                                        ProbeScope::AllTraits) {
-
-                    // If we find a different result the caller probably forgot to import a trait.
-                    Ok(ref new_pick) if *new_pick != pick => vec![new_pick.item.container.id()],
-                    Err(Ambiguity(ref sources)) => {
-                        sources.iter()
-                               .filter_map(|source| {
-                                   match *source {
-                                       // Note: this cannot come from an inherent impl,
-                                       // because the first probing succeeded.
-                                       ImplSource(def) => self.tcx.trait_id_of_impl(def),
-                                       TraitSource(_) => None,
-                                   }
-                               })
-                               .collect()
+            let candidates = match self.lookup_probe(
+                span,
+                segment.ident,
+                self_ty,
+                call_expr,
+                ProbeScope::AllTraits,
+            ) {
+                // If we find a different result the caller probably forgot to import a trait.
+                Ok(ref new_pick) if *new_pick != pick => vec![new_pick.item.container.id()],
+                Err(Ambiguity(ref sources)) => sources.iter().filter_map(|source| {
+                    match *source {
+                        // Note: this cannot come from an inherent impl,
+                        // because the first probing succeeded.
+                        ImplSource(def) => self.tcx.trait_id_of_impl(def),
+                        TraitSource(_) => None,
                     }
-                    _ => Vec::new(),
-                };
+                }).collect(),
+                _ => Vec::new(),
+            };
 
-            return Err(IllegalSizedBound(candidates));
+            return Err(IllegalSizedBound(candidates, needs_mut));
         }
 
         Ok(result.callee)

src/librustc_typeck/check/method/suggest.rs

@@ -593,22 +593,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 err.emit();
             }
 
-            MethodError::IllegalSizedBound(candidates) => {
+            MethodError::IllegalSizedBound(candidates, needs_mut) => {
                 let msg = format!("the `{}` method cannot be invoked on a trait object", item_name);
                 let mut err = self.sess().struct_span_err(span, &msg);
                 if !candidates.is_empty() {
-                    let help = format!("{an}other candidate{s} {were} found in the following \
-                                        trait{s}, perhaps add a `use` for {one_of_them}:",
-                                    an = if candidates.len() == 1 {"an" } else { "" },
-                                    s = pluralise!(candidates.len()),
-                                    were = if candidates.len() == 1 { "was" } else { "were" },
-                                    one_of_them = if candidates.len() == 1 {
-                                        "it"
-                                    } else {
-                                        "one_of_them"
-                                    });
+                    let help = format!(
+                        "{an}other candidate{s} {were} found in the following trait{s}, perhaps \
+                         add a `use` for {one_of_them}:",
+                        an = if candidates.len() == 1 {"an" } else { "" },
+                        s = pluralise!(candidates.len()),
+                        were = if candidates.len() == 1 { "was" } else { "were" },
+                        one_of_them = if candidates.len() == 1 {
+                            "it"
+                        } else {
+                            "one_of_them"
+                        },
+                    );
                     self.suggest_use_candidates(&mut err, help, candidates);
                 }
+                if let ty::Ref(region, t_type, mutability) = rcvr_ty.kind {
+                    if needs_mut {
+                        let trait_type = self.tcx.mk_ref(region, ty::TypeAndMut {
+                            ty: t_type,
+                            mutbl: mutability.invert(),
+                        });
+                        err.note(&format!("you need `{}` instead of `{}`", trait_type, rcvr_ty));
+                    }
+                }
                 err.emit();
             }
 

src/test/ui/not-panic/not-panic-safe.stderr

@@ -8,6 +8,7 @@ LL |     assert::<&mut i32>();
    |     ^^^^^^^^^^^^^^^^^^ `&mut i32` may not be safely transferred across an unwind boundary
    |
    = help: the trait `std::panic::UnwindSafe` is not implemented for `&mut i32`
+   = note: `std::panic::UnwindSafe` is implemented for `&i32`, but not for `&mut i32`
 
 error: aborting due to previous error
 

src/test/ui/suggestions/imm-ref-trait-object-literal.rs

@@ -0,0 +1,14 @@
+trait Trait {}
+
+struct S;
+
+impl<'a> Trait for &'a mut S {}
+
+fn foo<X: Trait>(_: X) {}
+
+
+fn main() {
+  let s = S;
+  foo(&s); //~ ERROR the trait bound `&S: Trait` is not satisfied
+  foo(s); //~ ERROR the trait bound `S: Trait` is not satisfied
+}

src/test/ui/suggestions/imm-ref-trait-object-literal.stderr

@@ -0,0 +1,30 @@
+error[E0277]: the trait bound `&S: Trait` is not satisfied
+  --> $DIR/imm-ref-trait-object-literal.rs:12:7
+   |
+LL | fn foo<X: Trait>(_: X) {}
+   |    ---    ----- required by this bound in `foo`
+...
+LL |   foo(&s);
+   |       -^
+   |       |
+   |       the trait `Trait` is not implemented for `&S`
+   |       help: consider changing this borrow's mutability: `&mut`
+   |
+   = help: the following implementations were found:
+             <&'a mut S as Trait>
+
+error[E0277]: the trait bound `S: Trait` is not satisfied
+  --> $DIR/imm-ref-trait-object-literal.rs:13:7
+   |
+LL | fn foo<X: Trait>(_: X) {}
+   |    ---    ----- required by this bound in `foo`
+...
+LL |   foo(s);
+   |       ^ the trait `Trait` is not implemented for `S`
+   |
+   = help: the following implementations were found:
+             <&'a mut S as Trait>
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0277`.

src/test/ui/suggestions/imm-ref-trait-object.rs

@@ -0,0 +1,8 @@
+fn test(t: &dyn Iterator<Item=&u64>) -> u64 {
+     t.min().unwrap() //~ ERROR the `min` method cannot be invoked on a trait object
+}
+
+fn main() {
+     let array = [0u64];
+     test(&mut array.iter());
+}

src/test/ui/suggestions/imm-ref-trait-object.stderr

@@ -0,0 +1,10 @@
+error: the `min` method cannot be invoked on a trait object
+  --> $DIR/imm-ref-trait-object.rs:2:8
+   |
+LL |      t.min().unwrap()
+   |        ^^^
+   |
+   = note: you need `&mut dyn std::iter::Iterator<Item = &u64>` instead of `&dyn std::iter::Iterator<Item = &u64>`
+
+error: aborting due to previous error
+

src/test/ui/suggestions/into-str.stderr

@@ -8,6 +8,7 @@ LL |     foo(String::new());
    |     ^^^ the trait `std::convert::From<std::string::String>` is not implemented for `&str`
    |
    = note: to coerce a `std::string::String` into a `&str`, use `&*` as a prefix
+   = note: `std::convert::From<std::string::String>` is implemented for `&mut str`, but not for `&str`
    = note: required because of the requirements on the impl of `std::convert::Into<&str>` for `std::string::String`
 
 error: aborting due to previous error