Improve E0433, so that it suggests missing imports

Author: Patryk27

src/librustc_resolve/diagnostics.rs

@@ -1475,7 +1475,7 @@ crate fn show_candidates(
     // This is `None` if all placement locations are inside expansions
     use_placement_span: Option<Span>,
     candidates: &[ImportSuggestion],
-    better: bool,
+    instead: bool,
     found_use: bool,
 ) {
     if candidates.is_empty() {
@@ -1486,6 +1486,7 @@ crate fn show_candidates(
     // by iterating through a hash map, so make sure they are ordered:
     let mut path_strings: Vec<_> =
         candidates.iter().map(|c| path_names_to_string(&c.path)).collect();
+
     path_strings.sort();
     path_strings.dedup();
 
@@ -1494,8 +1495,9 @@ crate fn show_candidates(
     } else {
         ("one of these", "items")
     };
-    let instead = if better { " instead" } else { "" };
-    let msg = format!("consider importing {} {}{}", determiner, kind, instead);
+
+    let instead = if instead { " instead" } else { "" };
+    let mut msg = format!("consider importing {} {}{}", determiner, kind, instead);
 
     if let Some(span) = use_placement_span {
         for candidate in &mut path_strings {
@@ -1507,12 +1509,13 @@ crate fn show_candidates(
 
         err.span_suggestions(span, &msg, path_strings.into_iter(), Applicability::Unspecified);
     } else {
-        let mut msg = msg;
         msg.push(':');
+
         for candidate in path_strings {
             msg.push('\n');
             msg.push_str(&candidate);
         }
+
         err.note(&msg);
     }
 }

src/librustc_resolve/late.rs

@@ -29,8 +29,9 @@ use rustc_span::Span;
 use smallvec::{smallvec, SmallVec};
 
 use log::debug;
+use rustc_span::source_map::{respan, Spanned};
 use std::collections::BTreeSet;
-use std::mem::replace;
+use std::mem::{replace, take};
 
 mod diagnostics;
 crate mod lifetimes;
@@ -234,6 +235,13 @@ impl<'a> PathSource<'a> {
         }
     }
 
+    fn is_call(self) -> bool {
+        match self {
+            PathSource::Expr(Some(&Expr { kind: ExprKind::Call(..), .. })) => true,
+            _ => false,
+        }
+    }
+
     crate fn is_expected(self, res: Res) -> bool {
         match self {
             PathSource::Type => match res {
@@ -1620,14 +1628,83 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
 
         let report_errors = |this: &mut Self, res: Option<Res>| {
             let (err, candidates) = this.smart_resolve_report_errors(path, span, source, res);
+
             let def_id = this.parent_scope.module.normal_ancestor_id;
-            let better = res.is_some();
+            let instead = res.is_some();
             let suggestion =
                 if res.is_none() { this.report_missing_type_error(path) } else { None };
-            this.r.use_injections.push(UseError { err, candidates, def_id, better, suggestion });
+
+            this.r.use_injections.push(UseError { err, candidates, def_id, instead, suggestion });
+
             PartialRes::new(Res::Err)
         };
 
+        // For paths originating from calls (like in `HashMap::new()`), tries
+        // to enrich the plain `failed to resolve: ...` message with hints
+        // about possible missing imports.
+        //
+        // Similar thing, for types, happens in `report_errors` above.
+        let report_errors_for_call = |this: &mut Self, parent_err: Spanned<ResolutionError<'a>>| {
+            if !source.is_call() {
+                return Some(parent_err);
+            }
+
+            // Before we start looking for candidates, we have to get our hands
+            // on the type user is trying to perform invocation on; basically:
+            // we're transforming `HashMap::new` into just `HashMap`
+            let path = if let Some((_, path)) = path.split_last() {
+                path
+            } else {
+                return Some(parent_err);
+            };
+
+            let (mut err, candidates) =
+                this.smart_resolve_report_errors(path, span, PathSource::Type, None);
+
+            if candidates.is_empty() {
+                err.cancel();
+                return Some(parent_err);
+            }
+
+            // There are two different error messages user might receive at
+            // this point:
+            // - E0412 cannot find type `{}` in this scope
+            // - E0433 failed to resolve: use of undeclared type or module `{}`
+            //
+            // The first one is emitted for paths in type-position, and the
+            // latter one - for paths in expression-position.
+            //
+            // Thus (since we're in expression-position at this point), not to
+            // confuse the user, we want to keep the *message* from E04322 (so
+            // `parent_err`), but we want *hints* from E0412 (so `err`).
+            //
+            // And that's what happens below - we're just mixing both messages
+            // into a single one.
+            let mut parent_err = this.r.into_struct_error(parent_err.span, parent_err.node);
+
+            parent_err.cancel();
+
+            err.message = take(&mut parent_err.message);
+            err.code = take(&mut parent_err.code);
+            err.children = take(&mut parent_err.children);
+
+            drop(parent_err);
+
+            let def_id = this.parent_scope.module.normal_ancestor_id;
+
+            this.r.use_injections.push(UseError {
+                err,
+                candidates,
+                def_id,
+                instead: false,
+                suggestion: None,
+            });
+
+            // We don't return `Some(parent_err)` here, because the error will
+            // be already printed as part of the `use` injections
+            None
+        };
+
         let partial_res = match self.resolve_qpath_anywhere(
             id,
             qself,
@@ -1637,14 +1714,15 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             source.defer_to_typeck(),
             crate_lint,
         ) {
-            Some(partial_res) if partial_res.unresolved_segments() == 0 => {
+            Ok(Some(partial_res)) if partial_res.unresolved_segments() == 0 => {
                 if is_expected(partial_res.base_res()) || partial_res.base_res() == Res::Err {
                     partial_res
                 } else {
                     report_errors(self, Some(partial_res.base_res()))
                 }
             }
-            Some(partial_res) if source.defer_to_typeck() => {
+
+            Ok(Some(partial_res)) if source.defer_to_typeck() => {
                 // Not fully resolved associated item `T::A::B` or `<T as Tr>::A::B`
                 // or `<T>::A::B`. If `B` should be resolved in value namespace then
                 // it needs to be added to the trait map.
@@ -1655,25 +1733,34 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                 }
 
                 let mut std_path = vec![Segment::from_ident(Ident::with_dummy_span(sym::std))];
+
                 std_path.extend(path);
+
                 if self.r.primitive_type_table.primitive_types.contains_key(&path[0].ident.name) {
-                    let cl = CrateLint::No;
-                    let ns = Some(ns);
                     if let PathResult::Module(_) | PathResult::NonModule(_) =
-                        self.resolve_path(&std_path, ns, false, span, cl)
+                        self.resolve_path(&std_path, Some(ns), false, span, CrateLint::No)
                     {
-                        // check if we wrote `str::from_utf8` instead of `std::str::from_utf8`
+                        // Check if we wrote `str::from_utf8` instead of `std::str::from_utf8`
                         let item_span =
                             path.iter().last().map(|segment| segment.ident.span).unwrap_or(span);
-                        debug!("accessed item from `std` submodule as a bare type {:?}", std_path);
+
                         let mut hm = self.r.session.confused_type_with_std_module.borrow_mut();
                         hm.insert(item_span, span);
-                        // In some places (E0223) we only have access to the full path
                         hm.insert(span, span);
                     }
                 }
+
                 partial_res
             }
+
+            Err(err) => {
+                if let Some(err) = report_errors_for_call(self, err) {
+                    self.r.report_error(err.span, err.node);
+                }
+
+                PartialRes::new(Res::Err)
+            }
+
             _ => report_errors(self, None),
         };
 
@@ -1682,6 +1769,7 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             // Avoid recording definition of `A::B` in `<T as A>::B::C`.
             self.r.record_partial_res(id, partial_res);
         }
+
         partial_res
     }
 
@@ -1711,17 +1799,16 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         span: Span,
         defer_to_typeck: bool,
         crate_lint: CrateLint,
-    ) -> Option<PartialRes> {
+    ) -> Result<Option<PartialRes>, Spanned<ResolutionError<'a>>> {
         let mut fin_res = None;
+
         for (i, ns) in [primary_ns, TypeNS, ValueNS].iter().cloned().enumerate() {
             if i == 0 || ns != primary_ns {
-                match self.resolve_qpath(id, qself, path, ns, span, crate_lint) {
-                    // If defer_to_typeck, then resolution > no resolution,
-                    // otherwise full resolution > partial resolution > no resolution.
+                match self.resolve_qpath(id, qself, path, ns, span, crate_lint)? {
                     Some(partial_res)
                         if partial_res.unresolved_segments() == 0 || defer_to_typeck =>
                     {
-                        return Some(partial_res);
+                        return Ok(Some(partial_res));
                     }
                     partial_res => {
                         if fin_res.is_none() {
@@ -1732,19 +1819,19 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             }
         }
 
-        // `MacroNS`
         assert!(primary_ns != MacroNS);
+
         if qself.is_none() {
             let path_seg = |seg: &Segment| PathSegment::from_ident(seg.ident);
             let path = Path { segments: path.iter().map(path_seg).collect(), span };
             if let Ok((_, res)) =
                 self.r.resolve_macro_path(&path, None, &self.parent_scope, false, false)
             {
-                return Some(PartialRes::new(res));
+                return Ok(Some(PartialRes::new(res)));
             }
         }
 
-        fin_res
+        Ok(fin_res)
     }
 
     /// Handles paths that may refer to associated items.
@@ -1756,7 +1843,7 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         ns: Namespace,
         span: Span,
         crate_lint: CrateLint,
-    ) -> Option<PartialRes> {
+    ) -> Result<Option<PartialRes>, Spanned<ResolutionError<'a>>> {
         debug!(
             "resolve_qpath(id={:?}, qself={:?}, path={:?}, ns={:?}, span={:?})",
             id, qself, path, ns, span,
@@ -1767,10 +1854,10 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                 // This is a case like `<T>::B`, where there is no
                 // trait to resolve.  In that case, we leave the `B`
                 // segment to be resolved by type-check.
-                return Some(PartialRes::with_unresolved_segments(
+                return Ok(Some(PartialRes::with_unresolved_segments(
                     Res::Def(DefKind::Mod, DefId::local(CRATE_DEF_INDEX)),
                     path.len(),
-                ));
+                )));
             }
 
             // Make sure `A::B` in `<T as A::B>::C` is a trait item.
@@ -1800,10 +1887,10 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             // The remaining segments (the `C` in our example) will
             // have to be resolved by type-check, since that requires doing
             // trait resolution.
-            return Some(PartialRes::with_unresolved_segments(
+            return Ok(Some(PartialRes::with_unresolved_segments(
                 partial_res.base_res(),
                 partial_res.unresolved_segments() + path.len() - qself.position - 1,
-            ));
+            )));
         }
 
         let result = match self.resolve_path(&path, Some(ns), true, span, crate_lint) {
@@ -1838,11 +1925,10 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                 PartialRes::new(module.res().unwrap())
             }
             PathResult::Failed { is_error_from_last_segment: false, span, label, suggestion } => {
-                self.r.report_error(span, ResolutionError::FailedToResolve { label, suggestion });
-                PartialRes::new(Res::Err)
+                return Err(respan(span, ResolutionError::FailedToResolve { label, suggestion }));
             }
-            PathResult::Module(..) | PathResult::Failed { .. } => return None,
-            PathResult::Indeterminate => bug!("indetermined path result in resolve_qpath"),
+            PathResult::Module(..) | PathResult::Failed { .. } => return Ok(None),
+            PathResult::Indeterminate => bug!("indeterminate path result in resolve_qpath"),
         };
 
         if path.len() > 1
@@ -1862,7 +1948,7 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                     PathResult::Module(ModuleOrUniformRoot::Module(module)) => {
                         module.res().unwrap()
                     }
-                    _ => return Some(result),
+                    _ => return Ok(Some(result)),
                 }
             };
             if result.base_res() == unqualified_result {
@@ -1871,7 +1957,7 @@ impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
             }
         }
 
-        Some(result)
+        Ok(Some(result))
     }
 
     fn with_resolved_label(&mut self, label: Option<Label>, id: NodeId, f: impl FnOnce(&mut Self)) {

src/librustc_resolve/lib.rs

@@ -618,13 +618,13 @@ struct PrivacyError<'a> {
 
 struct UseError<'a> {
     err: DiagnosticBuilder<'a>,
-    /// Attach `use` statements for these candidates.
+    /// Candidates which user could `use` to access the missing type.
     candidates: Vec<ImportSuggestion>,
-    /// The `NodeId` of the module to place the use-statements in.
+    /// The `DefId` of the module to place the use-statements in.
     def_id: DefId,
-    /// Whether the diagnostic should state that it's "better".
-    better: bool,
-    /// Extra free form suggestion. Currently used to suggest new type parameter.
+    /// Whether the diagnostic should say "instead" (as in `consider importing ... instead`).
+    instead: bool,
+    /// Extra free-form suggestion.
     suggestion: Option<(Span, &'static str, String, Applicability)>,
 }
 
@@ -2577,12 +2577,12 @@ impl<'a> Resolver<'a> {
     }
 
     fn report_with_use_injections(&mut self, krate: &Crate) {
-        for UseError { mut err, candidates, def_id, better, suggestion } in
+        for UseError { mut err, candidates, def_id, instead, suggestion } in
             self.use_injections.drain(..)
         {
             let (span, found_use) = UsePlacementFinder::check(&self.definitions, krate, def_id);
             if !candidates.is_empty() {
-                diagnostics::show_candidates(&mut err, span, &candidates, better, found_use);
+                diagnostics::show_candidates(&mut err, span, &candidates, instead, found_use);
             } else if let Some((span, msg, sugg, appl)) = suggestion {
                 err.span_suggestion(span, msg, sugg, appl);
             }

src/test/ui/derived-errors/issue-31997-1.stderr

@@ -2,7 +2,14 @@ error[E0433]: failed to resolve: use of undeclared type or module `HashMap`
   --> $DIR/issue-31997-1.rs:20:19
    |
 LL |     let mut map = HashMap::new();
-   |                   ^^^^^^^ use of undeclared type or module `HashMap`
+   |                   ^^^^^^^ not found in this scope
+   |
+help: consider importing one of these items
+   |
+LL | use std::collections::HashMap;
+   |
+LL | use std::collections::hash_map::HashMap;
+   |
 
 error: aborting due to previous error
 

src/test/ui/error-codes/E0433.rs

@@ -1,3 +1,3 @@
 fn main () {
-    let map = HashMap::new(); //~ ERROR E0433
+    let map = NonExistingMap::new(); //~ ERROR E0433
 }