Auto merge of #14261 - Veykril:ty-perf, r=Veykril

internal: Re-use the resolver in `InferenceContext` instead of rebuilding it whenever needed

This reduced inference time on my local build by roughly ~1 sec (out of like 60)

crates/hir-def/src/body/scope.rs

@@ -66,6 +66,7 @@ impl ExprScopes {
         self.scopes[scope].label.clone()
     }
 
+    /// Returns the scopes in ascending order.
     pub fn scope_chain(&self, scope: Option<ScopeId>) -> impl Iterator<Item = ScopeId> + '_ {
         std::iter::successors(scope, move |&scope| self.scopes[scope].parent)
     }

crates/hir-def/src/resolver.rs

@@ -1,5 +1,5 @@
 //! Name resolution façade.
-use std::{hash::BuildHasherDefault, sync::Arc};
+use std::{fmt, hash::BuildHasherDefault, sync::Arc};
 
 use base_db::CrateId;
 use hir_expand::name::{name, Name};
@@ -36,19 +36,34 @@ pub struct Resolver {
     module_scope: ModuleItemMap,
 }
 
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 struct ModuleItemMap {
     def_map: Arc<DefMap>,
     module_id: LocalModuleId,
 }
 
-#[derive(Debug, Clone)]
+impl fmt::Debug for ModuleItemMap {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("ModuleItemMap").field("module_id", &self.module_id).finish()
+    }
+}
+
+#[derive(Clone)]
 struct ExprScope {
     owner: DefWithBodyId,
     expr_scopes: Arc<ExprScopes>,
     scope_id: ScopeId,
 }
 
+impl fmt::Debug for ExprScope {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("ExprScope")
+            .field("owner", &self.owner)
+            .field("scope_id", &self.scope_id)
+            .finish()
+    }
+}
+
 #[derive(Debug, Clone)]
 enum Scope {
     /// All the items and imported names of a module
@@ -240,55 +255,67 @@ impl Resolver {
             return self.module_scope.resolve_path_in_value_ns(db, path);
         }
 
-        for scope in self.scopes() {
+        if n_segments <= 1 {
-            match scope {
+            for scope in self.scopes() {
-                Scope::ExprScope(_) if n_segments > 1 => continue,
+                match scope {
-                Scope::ExprScope(scope) => {
+                    Scope::ExprScope(scope) => {
-                    let entry = scope
+                        let entry = scope
-                        .expr_scopes
+                            .expr_scopes
-                        .entries(scope.scope_id)
+                            .entries(scope.scope_id)
-                        .iter()
+                            .iter()
-                        .find(|entry| entry.name() == first_name);
+                            .find(|entry| entry.name() == first_name);
 
-                    if let Some(e) = entry {
+                        if let Some(e) = entry {
-                        return Some(ResolveValueResult::ValueNs(ValueNs::LocalBinding(e.pat())));
+                            return Some(ResolveValueResult::ValueNs(ValueNs::LocalBinding(
+                                e.pat(),
+                            )));
+                        }
+                    }
+                    Scope::GenericParams { params, def } => {
+                        if let Some(id) = params.find_const_by_name(first_name, *def) {
+                            let val = ValueNs::GenericParam(id);
+                            return Some(ResolveValueResult::ValueNs(val));
+                        }
+                    }
+                    &Scope::ImplDefScope(impl_) => {
+                        if first_name == &name![Self] {
+                            return Some(ResolveValueResult::ValueNs(ValueNs::ImplSelf(impl_)));
+                        }
+                    }
+                    // bare `Self` doesn't work in the value namespace in a struct/enum definition
+                    Scope::AdtScope(_) => continue,
+                    Scope::BlockScope(m) => {
+                        if let Some(def) = m.resolve_path_in_value_ns(db, path) {
+                            return Some(def);
+                        }
                     }
                 }
-                Scope::GenericParams { params, def } if n_segments > 1 => {
+            }
-                    if let Some(id) = params.find_type_by_name(first_name, *def) {
+        } else {
-                        let ty = TypeNs::GenericParam(id);
+            for scope in self.scopes() {
-                        return Some(ResolveValueResult::Partial(ty, 1));
+                match scope {
+                    Scope::ExprScope(_) => continue,
+                    Scope::GenericParams { params, def } => {
+                        if let Some(id) = params.find_type_by_name(first_name, *def) {
+                            let ty = TypeNs::GenericParam(id);
+                            return Some(ResolveValueResult::Partial(ty, 1));
+                        }
                     }
-                }
+                    &Scope::ImplDefScope(impl_) => {
-                Scope::GenericParams { .. } if n_segments != 1 => continue,
+                        if first_name == &name![Self] {
-                Scope::GenericParams { params, def } => {
+                            return Some(ResolveValueResult::Partial(TypeNs::SelfType(impl_), 1));
-                    if let Some(id) = params.find_const_by_name(first_name, *def) {
+                        }
-                        let val = ValueNs::GenericParam(id);
-                        return Some(ResolveValueResult::ValueNs(val));
                     }
-                }
+                    Scope::AdtScope(adt) => {
-
+                        if first_name == &name![Self] {
-                &Scope::ImplDefScope(impl_) => {
+                            let ty = TypeNs::AdtSelfType(*adt);
-                    if first_name == &name![Self] {
+                            return Some(ResolveValueResult::Partial(ty, 1));
-                        return Some(if n_segments > 1 {
+                        }
-                            ResolveValueResult::Partial(TypeNs::SelfType(impl_), 1)
-                        } else {
-                            ResolveValueResult::ValueNs(ValueNs::ImplSelf(impl_))
-                        });
                     }
-                }
+                    Scope::BlockScope(m) => {
-                // bare `Self` doesn't work in the value namespace in a struct/enum definition
+                        if let Some(def) = m.resolve_path_in_value_ns(db, path) {
-                Scope::AdtScope(_) if n_segments == 1 => continue,
+                            return Some(def);
-                Scope::AdtScope(adt) => {
+                        }
-                    if first_name == &name![Self] {
-                        let ty = TypeNs::AdtSelfType(*adt);
-                        return Some(ResolveValueResult::Partial(ty, 1));
-                    }
-                }
-
-                Scope::BlockScope(m) => {
-                    if let Some(def) = m.resolve_path_in_value_ns(db, path) {
-                        return Some(def);
                     }
                 }
             }
@@ -301,8 +328,8 @@ impl Resolver {
         // If a path of the shape `u16::from_le_bytes` failed to resolve at all, then we fall back
         // to resolving to the primitive type, to allow this to still work in the presence of
         // `use core::u16;`.
-        if path.kind == PathKind::Plain && path.segments().len() > 1 {
+        if path.kind == PathKind::Plain && n_segments > 1 {
-            if let Some(builtin) = BuiltinType::by_name(&path.segments()[0]) {
+            if let Some(builtin) = BuiltinType::by_name(first_name) {
                 return Some(ResolveValueResult::Partial(TypeNs::BuiltinType(builtin), 1));
             }
         }
@@ -434,6 +461,15 @@ impl Resolver {
         traits
     }
 
+    pub fn traits_in_scope_from_block_scopes(&self) -> impl Iterator<Item = TraitId> + '_ {
+        self.scopes()
+            .filter_map(|scope| match scope {
+                Scope::BlockScope(m) => Some(m.def_map[m.module_id].scope.traits()),
+                _ => None,
+            })
+            .flatten()
+    }
+
     pub fn module(&self) -> ModuleId {
         let (def_map, local_id) = self.item_scope();
         def_map.module_id(local_id)
@@ -478,8 +514,72 @@ impl Resolver {
             _ => None,
         })
     }
+    /// `expr_id` is required to be an expression id that comes after the top level expression scope in the given resolver
+    #[must_use]
+    pub fn update_to_inner_scope(
+        &mut self,
+        db: &dyn DefDatabase,
+        owner: DefWithBodyId,
+        expr_id: ExprId,
+    ) -> UpdateGuard {
+        #[inline(always)]
+        fn append_expr_scope(
+            db: &dyn DefDatabase,
+            resolver: &mut Resolver,
+            owner: DefWithBodyId,
+            expr_scopes: &Arc<ExprScopes>,
+            scope_id: ScopeId,
+        ) {
+            resolver.scopes.push(Scope::ExprScope(ExprScope {
+                owner,
+                expr_scopes: expr_scopes.clone(),
+                scope_id,
+            }));
+            if let Some(block) = expr_scopes.block(scope_id) {
+                if let Some(def_map) = db.block_def_map(block) {
+                    let root = def_map.root();
+                    resolver
+                        .scopes
+                        .push(Scope::BlockScope(ModuleItemMap { def_map, module_id: root }));
+                    // FIXME: This adds as many module scopes as there are blocks, but resolving in each
+                    // already traverses all parents, so this is O(n²). I think we could only store the
+                    // innermost module scope instead?
+                }
+            }
+        }
+
+        let start = self.scopes.len();
+        let innermost_scope = self.scopes().next();
+        match innermost_scope {
+            Some(&Scope::ExprScope(ExprScope { scope_id, ref expr_scopes, owner })) => {
+                let expr_scopes = expr_scopes.clone();
+                let scope_chain = expr_scopes
+                    .scope_chain(expr_scopes.scope_for(expr_id))
+                    .take_while(|&it| it != scope_id);
+                for scope_id in scope_chain {
+                    append_expr_scope(db, self, owner, &expr_scopes, scope_id);
+                }
+            }
+            _ => {
+                let expr_scopes = db.expr_scopes(owner);
+                let scope_chain = expr_scopes.scope_chain(expr_scopes.scope_for(expr_id));
+
+                for scope_id in scope_chain {
+                    append_expr_scope(db, self, owner, &expr_scopes, scope_id);
+                }
+            }
+        }
+        self.scopes[start..].reverse();
+        UpdateGuard(start)
+    }
+
+    pub fn reset_to_guard(&mut self, UpdateGuard(start): UpdateGuard) {
+        self.scopes.truncate(start);
+    }
 }
 
+pub struct UpdateGuard(usize);
+
 impl Resolver {
     fn scopes(&self) -> impl Iterator<Item = &Scope> {
         self.scopes.iter().rev()
@@ -576,10 +676,11 @@ impl Scope {
     }
 }
 
-// needs arbitrary_self_types to be a method... or maybe move to the def?
 pub fn resolver_for_expr(db: &dyn DefDatabase, owner: DefWithBodyId, expr_id: ExprId) -> Resolver {
+    let r = owner.resolver(db);
     let scopes = db.expr_scopes(owner);
-    resolver_for_scope(db, owner, scopes.scope_for(expr_id))
+    let scope_id = scopes.scope_for(expr_id);
+    resolver_for_scope_(db, scopes, scope_id, r, owner)
 }
 
 pub fn resolver_for_scope(
@@ -587,8 +688,18 @@ pub fn resolver_for_scope(
     owner: DefWithBodyId,
     scope_id: Option<ScopeId>,
 ) -> Resolver {
-    let mut r = owner.resolver(db);
+    let r = owner.resolver(db);
     let scopes = db.expr_scopes(owner);
+    resolver_for_scope_(db, scopes, scope_id, r, owner)
+}
+
+fn resolver_for_scope_(
+    db: &dyn DefDatabase,
+    scopes: Arc<ExprScopes>,
+    scope_id: Option<ScopeId>,
+    mut r: Resolver,
+    owner: DefWithBodyId,
+) -> Resolver {
     let scope_chain = scopes.scope_chain(scope_id).collect::<Vec<_>>();
     r.scopes.reserve(scope_chain.len());
 

crates/hir-ty/src/infer.rs

@@ -33,7 +33,7 @@ use hir_def::{
 };
 use hir_expand::name::{name, Name};
 use la_arena::ArenaMap;
-use rustc_hash::FxHashMap;
+use rustc_hash::{FxHashMap, FxHashSet};
 use stdx::always;
 
 use crate::{
@@ -423,6 +423,8 @@ pub(crate) struct InferenceContext<'a> {
     pub(crate) resolver: Resolver,
     table: unify::InferenceTable<'a>,
     trait_env: Arc<TraitEnvironment>,
+    /// The traits in scope, disregarding block modules. This is used for caching purposes.
+    traits_in_scope: FxHashSet<TraitId>,
     pub(crate) result: InferenceResult,
     /// The return type of the function being inferred, the closure or async block if we're
     /// currently within one.
@@ -505,6 +507,7 @@ impl<'a> InferenceContext<'a> {
             db,
             owner,
             body,
+            traits_in_scope: resolver.traits_in_scope(db.upcast()),
             resolver,
             diverges: Diverges::Maybe,
             breakables: Vec::new(),
@@ -706,7 +709,6 @@ impl<'a> InferenceContext<'a> {
     }
 
     fn make_ty(&mut self, type_ref: &TypeRef) -> Ty {
-        // FIXME use right resolver for block
         let ctx = crate::lower::TyLoweringContext::new(self.db, &self.resolver);
         let ty = ctx.lower_ty(type_ref);
         let ty = self.insert_type_vars(ty);
@@ -822,12 +824,11 @@ impl<'a> InferenceContext<'a> {
             Some(path) => path,
             None => return (self.err_ty(), None),
         };
-        let resolver = &self.resolver;
         let ctx = crate::lower::TyLoweringContext::new(self.db, &self.resolver);
         // FIXME: this should resolve assoc items as well, see this example:
         // https://play.rust-lang.org/?gist=087992e9e22495446c01c0d4e2d69521
         let (resolution, unresolved) = if value_ns {
-            match resolver.resolve_path_in_value_ns(self.db.upcast(), path.mod_path()) {
+            match self.resolver.resolve_path_in_value_ns(self.db.upcast(), path.mod_path()) {
                 Some(ResolveValueResult::ValueNs(value)) => match value {
                     ValueNs::EnumVariantId(var) => {
                         let substs = ctx.substs_from_path(path, var.into(), true);
@@ -848,7 +849,7 @@ impl<'a> InferenceContext<'a> {
                 None => return (self.err_ty(), None),
             }
         } else {
-            match resolver.resolve_path_in_type_ns(self.db.upcast(), path.mod_path()) {
+            match self.resolver.resolve_path_in_type_ns(self.db.upcast(), path.mod_path()) {
                 Some(it) => it,
                 None => return (self.err_ty(), None),
             }
@@ -1058,6 +1059,15 @@ impl<'a> InferenceContext<'a> {
         let struct_ = self.resolve_lang_item(LangItem::VaList)?.as_struct()?;
         Some(struct_.into())
     }
+
+    fn get_traits_in_scope(&self) -> Either<FxHashSet<TraitId>, &FxHashSet<TraitId>> {
+        let mut b_traits = self.resolver.traits_in_scope_from_block_scopes().peekable();
+        if b_traits.peek().is_some() {
+            Either::Left(self.traits_in_scope.iter().copied().chain(b_traits).collect())
+        } else {
+            Either::Right(&self.traits_in_scope)
+        }
+    }
 }
 
 /// When inferring an expression, we propagate downward whatever type hint we

crates/hir-ty/src/infer/expr.rs

@@ -15,7 +15,6 @@ use hir_def::{
     generics::TypeOrConstParamData,
     lang_item::LangItem,
     path::{GenericArg, GenericArgs},
-    resolver::resolver_for_expr,
     ConstParamId, FieldId, ItemContainerId, Lookup,
 };
 use hir_expand::name::{name, Name};
@@ -457,9 +456,10 @@ impl<'a> InferenceContext<'a> {
                 }
             }
             Expr::Path(p) => {
-                // FIXME this could be more efficient...
+                let g = self.resolver.update_to_inner_scope(self.db.upcast(), self.owner, tgt_expr);
-                let resolver = resolver_for_expr(self.db.upcast(), self.owner, tgt_expr);
+                let ty = self.infer_path(p, tgt_expr.into()).unwrap_or_else(|| self.err_ty());
-                self.infer_path(&resolver, p, tgt_expr.into()).unwrap_or_else(|| self.err_ty())
+                self.resolver.reset_to_guard(g);
+                ty
             }
             Expr::Continue { label } => {
                 if let None = find_continuable(&mut self.breakables, label.as_ref()) {
@@ -1168,8 +1168,8 @@ impl<'a> InferenceContext<'a> {
         expected: &Expectation,
     ) -> Ty {
         let coerce_ty = expected.coercion_target_type(&mut self.table);
-        let old_resolver =
+        let g = self.resolver.update_to_inner_scope(self.db.upcast(), self.owner, expr);
-            mem::replace(&mut self.resolver, resolver_for_expr(self.db.upcast(), self.owner, expr));
+
         let (break_ty, ty) =
             self.with_breakable_ctx(BreakableKind::Block, Some(coerce_ty.clone()), label, |this| {
                 for stmt in statements {
@@ -1256,7 +1256,7 @@ impl<'a> InferenceContext<'a> {
                     }
                 }
             });
-        self.resolver = old_resolver;
+        self.resolver.reset_to_guard(g);
 
         break_ty.unwrap_or(ty)
     }
@@ -1349,14 +1349,14 @@ impl<'a> InferenceContext<'a> {
             None => {
                 // no field found,
                 let method_with_same_name_exists = {
-                    let canonicalized_receiver = self.canonicalize(receiver_ty.clone());
+                    self.get_traits_in_scope();
-                    let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast());
 
+                    let canonicalized_receiver = self.canonicalize(receiver_ty.clone());
                     method_resolution::lookup_method(
                         self.db,
                         &canonicalized_receiver.value,
                         self.trait_env.clone(),
-                        &traits_in_scope,
+                        self.get_traits_in_scope().as_ref().left_or_else(|&it| it),
                         VisibleFromModule::Filter(self.resolver.module()),
                         name,
                     )
@@ -1385,13 +1385,11 @@ impl<'a> InferenceContext<'a> {
         let receiver_ty = self.infer_expr(receiver, &Expectation::none());
         let canonicalized_receiver = self.canonicalize(receiver_ty.clone());
 
-        let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast());
-
         let resolved = method_resolution::lookup_method(
             self.db,
             &canonicalized_receiver.value,
             self.trait_env.clone(),
-            &traits_in_scope,
+            self.get_traits_in_scope().as_ref().left_or_else(|&it| it),
             VisibleFromModule::Filter(self.resolver.module()),
             method_name,
         );

crates/hir-ty/src/infer/pat.rs

@@ -245,9 +245,8 @@ impl<'a> InferenceContext<'a> {
                 self.infer_record_pat_like(p.as_deref(), &expected, default_bm, pat, subs)
             }
             Pat::Path(path) => {
-                // FIXME use correct resolver for the surrounding expression
+                // FIXME update resolver for the surrounding expression
-                let resolver = self.resolver.clone();
+                self.infer_path(path, pat.into()).unwrap_or_else(|| self.err_ty())
-                self.infer_path(&resolver, path, pat.into()).unwrap_or_else(|| self.err_ty())
             }
             Pat::Bind { mode, name: _, subpat } => {
                 return self.infer_bind_pat(pat, *mode, default_bm, *subpat, &expected);

crates/hir-ty/src/infer/path.rs

@@ -3,7 +3,7 @@
 use chalk_ir::cast::Cast;
 use hir_def::{
     path::{Path, PathSegment},
-    resolver::{ResolveValueResult, Resolver, TypeNs, ValueNs},
+    resolver::{ResolveValueResult, TypeNs, ValueNs},
     AdtId, AssocItemId, EnumVariantId, ItemContainerId, Lookup,
 };
 use hir_expand::name::Name;
@@ -21,41 +21,31 @@ use crate::{
 use super::{ExprOrPatId, InferenceContext, TraitRef};
 
 impl<'a> InferenceContext<'a> {
-    pub(super) fn infer_path(
+    pub(super) fn infer_path(&mut self, path: &Path, id: ExprOrPatId) -> Option<Ty> {
-        &mut self,
+        let ty = self.resolve_value_path(path, id)?;
-        resolver: &Resolver,
-        path: &Path,
-        id: ExprOrPatId,
-    ) -> Option<Ty> {
-        let ty = self.resolve_value_path(resolver, path, id)?;
         let ty = self.insert_type_vars(ty);
         let ty = self.normalize_associated_types_in(ty);
         Some(ty)
     }
 
-    fn resolve_value_path(
+    fn resolve_value_path(&mut self, path: &Path, id: ExprOrPatId) -> Option<Ty> {
-        &mut self,
-        resolver: &Resolver,
-        path: &Path,
-        id: ExprOrPatId,
-    ) -> Option<Ty> {
         let (value, self_subst) = if let Some(type_ref) = path.type_anchor() {
             let Some(last) = path.segments().last() else { return None };
             let ty = self.make_ty(type_ref);
             let remaining_segments_for_ty = path.segments().take(path.segments().len() - 1);
-            let ctx = crate::lower::TyLoweringContext::new(self.db, resolver);
+            let ctx = crate::lower::TyLoweringContext::new(self.db, &self.resolver);
             let (ty, _) = ctx.lower_ty_relative_path(ty, None, remaining_segments_for_ty);
-            self.resolve_ty_assoc_item(ty, last.name, id)?
+            self.resolve_ty_assoc_item(ty, last.name, id).map(|(it, substs)| (it, Some(substs)))?
         } else {
             // FIXME: report error, unresolved first path segment
             let value_or_partial =
-                resolver.resolve_path_in_value_ns(self.db.upcast(), path.mod_path())?;
+                self.resolver.resolve_path_in_value_ns(self.db.upcast(), path.mod_path())?;
 
             match value_or_partial {
                 ResolveValueResult::ValueNs(it) => (it, None),
-                ResolveValueResult::Partial(def, remaining_index) => {
+                ResolveValueResult::Partial(def, remaining_index) => self
-                    self.resolve_assoc_item(def, path, remaining_index, id)?
+                    .resolve_assoc_item(def, path, remaining_index, id)
-                }
+                    .map(|(it, substs)| (it, Some(substs)))?,
             }
         };
 
@@ -123,7 +113,7 @@ impl<'a> InferenceContext<'a> {
         path: &Path,
         remaining_index: usize,
         id: ExprOrPatId,
-    ) -> Option<(ValueNs, Option<Substitution>)> {
+    ) -> Option<(ValueNs, Substitution)> {
         assert!(remaining_index < path.segments().len());
         // there may be more intermediate segments between the resolved one and
         // the end. Only the last segment needs to be resolved to a value; from
@@ -176,7 +166,7 @@ impl<'a> InferenceContext<'a> {
         trait_ref: TraitRef,
         segment: PathSegment<'_>,
         id: ExprOrPatId,
-    ) -> Option<(ValueNs, Option<Substitution>)> {
+    ) -> Option<(ValueNs, Substitution)> {
         let trait_ = trait_ref.hir_trait_id();
         let item =
             self.db.trait_data(trait_).items.iter().map(|(_name, id)| (*id)).find_map(|item| {
@@ -212,7 +202,7 @@ impl<'a> InferenceContext<'a> {
         };
 
         self.write_assoc_resolution(id, item, trait_ref.substitution.clone());
-        Some((def, Some(trait_ref.substitution)))
+        Some((def, trait_ref.substitution))
     }
 
     fn resolve_ty_assoc_item(
@@ -220,7 +210,7 @@ impl<'a> InferenceContext<'a> {
         ty: Ty,
         name: &Name,
         id: ExprOrPatId,
-    ) -> Option<(ValueNs, Option<Substitution>)> {
+    ) -> Option<(ValueNs, Substitution)> {
         if let TyKind::Error = ty.kind(Interner) {
             return None;
         }
@@ -230,70 +220,66 @@ impl<'a> InferenceContext<'a> {
         }
 
         let canonical_ty = self.canonicalize(ty.clone());
-        let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast());
 
         let mut not_visible = None;
         let res = method_resolution::iterate_method_candidates(
             &canonical_ty.value,
             self.db,
             self.table.trait_env.clone(),
-            &traits_in_scope,
+            self.get_traits_in_scope().as_ref().left_or_else(|&it| it),
             VisibleFromModule::Filter(self.resolver.module()),
             Some(name),
             method_resolution::LookupMode::Path,
             |_ty, item, visible| {
-                let (def, container) = match item {
-                    AssocItemId::FunctionId(f) => {
-                        (ValueNs::FunctionId(f), f.lookup(self.db.upcast()).container)
-                    }
-                    AssocItemId::ConstId(c) => {
-                        (ValueNs::ConstId(c), c.lookup(self.db.upcast()).container)
-                    }
-                    AssocItemId::TypeAliasId(_) => unreachable!(),
-                };
-                let substs = match container {
-                    ItemContainerId::ImplId(impl_id) => {
-                        let impl_substs = TyBuilder::subst_for_def(self.db, impl_id, None)
-                            .fill_with_inference_vars(&mut self.table)
-                            .build();
-                        let impl_self_ty =
-                            self.db.impl_self_ty(impl_id).substitute(Interner, &impl_substs);
-                        self.unify(&impl_self_ty, &ty);
-                        impl_substs
-                    }
-                    ItemContainerId::TraitId(trait_) => {
-                        // we're picking this method
-                        let trait_ref = TyBuilder::trait_ref(self.db, trait_)
-                            .push(ty.clone())
-                            .fill_with_inference_vars(&mut self.table)
-                            .build();
-                        self.push_obligation(trait_ref.clone().cast(Interner));
-                        trait_ref.substitution
-                    }
-                    ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => {
-                        never!("assoc item contained in module/extern block");
-                        return None;
-                    }
-                };
-
                 if visible {
-                    Some((def, item, Some(substs), true))
+                    Some((item, true))
                 } else {
                     if not_visible.is_none() {
-                        not_visible = Some((def, item, Some(substs), false));
+                        not_visible = Some((item, false));
                     }
                     None
                 }
             },
         );
         let res = res.or(not_visible);
-        if let Some((_, item, Some(ref substs), visible)) = res {
+        let (item, visible) = res?;
-            self.write_assoc_resolution(id, item, substs.clone());
+
-            if !visible {
+        let (def, container) = match item {
-                self.push_diagnostic(InferenceDiagnostic::PrivateAssocItem { id, item })
+            AssocItemId::FunctionId(f) => {
+                (ValueNs::FunctionId(f), f.lookup(self.db.upcast()).container)
             }
+            AssocItemId::ConstId(c) => (ValueNs::ConstId(c), c.lookup(self.db.upcast()).container),
+            AssocItemId::TypeAliasId(_) => unreachable!(),
+        };
+        let substs = match container {
+            ItemContainerId::ImplId(impl_id) => {
+                let impl_substs = TyBuilder::subst_for_def(self.db, impl_id, None)
+                    .fill_with_inference_vars(&mut self.table)
+                    .build();
+                let impl_self_ty = self.db.impl_self_ty(impl_id).substitute(Interner, &impl_substs);
+                self.unify(&impl_self_ty, &ty);
+                impl_substs
+            }
+            ItemContainerId::TraitId(trait_) => {
+                // we're picking this method
+                let trait_ref = TyBuilder::trait_ref(self.db, trait_)
+                    .push(ty.clone())
+                    .fill_with_inference_vars(&mut self.table)
+                    .build();
+                self.push_obligation(trait_ref.clone().cast(Interner));
+                trait_ref.substitution
+            }
+            ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => {
+                never!("assoc item contained in module/extern block");
+                return None;
+            }
+        };
+
+        self.write_assoc_resolution(id, item, substs.clone());
+        if !visible {
+            self.push_diagnostic(InferenceDiagnostic::PrivateAssocItem { id, item });
         }
-        res.map(|(def, _, substs, _)| (def, substs))
+        Some((def, substs))
     }
 
     fn resolve_enum_variant_on_ty(
@@ -301,7 +287,7 @@ impl<'a> InferenceContext<'a> {
         ty: &Ty,
         name: &Name,
         id: ExprOrPatId,
-    ) -> Option<(ValueNs, Option<Substitution>)> {
+    ) -> Option<(ValueNs, Substitution)> {
         let ty = self.resolve_ty_shallow(ty);
         let (enum_id, subst) = match ty.as_adt() {
             Some((AdtId::EnumId(e), subst)) => (e, subst),
@@ -311,6 +297,6 @@ impl<'a> InferenceContext<'a> {
         let local_id = enum_data.variant(name)?;
         let variant = EnumVariantId { parent: enum_id, local_id };
         self.write_variant_resolution(id, variant.into());
-        Some((ValueNs::EnumVariantId(variant), Some(subst.clone())))
+        Some((ValueNs::EnumVariantId(variant), subst.clone()))
     }
 }

crates/hir-ty/src/tests.rs

@@ -163,7 +163,7 @@ fn check_impl(ra_fixture: &str, allow_none: bool, only_types: bool, display_sour
                 } else {
                     ty.display_test(&db).to_string()
                 };
-                assert_eq!(actual, expected);
+                assert_eq!(actual, expected, "type annotation differs at {:#?}", range.range);
             }
         }
 
@@ -179,7 +179,7 @@ fn check_impl(ra_fixture: &str, allow_none: bool, only_types: bool, display_sour
                 } else {
                     ty.display_test(&db).to_string()
                 };
-                assert_eq!(actual, expected);
+                assert_eq!(actual, expected, "type annotation differs at {:#?}", range.range);
             }
             if let Some(expected) = adjustments.remove(&range) {
                 let adjustments = inference_result