From a8606e536347d21fa13768354f17048c54d7d89d Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 5 Mar 2023 14:37:44 +0100 Subject: [PATCH] Re-use the resolver in InferenceContext instead of rebuilding it on every expression change --- crates/hir-def/src/body/scope.rs | 1 + crates/hir-def/src/resolver.rs | 102 +++++++++++++++++++++++++++++-- crates/hir-ty/src/infer.rs | 6 +- crates/hir-ty/src/infer/expr.rs | 14 ++--- crates/hir-ty/src/infer/pat.rs | 5 +- crates/hir-ty/src/infer/path.rs | 22 ++----- crates/hir-ty/src/tests.rs | 4 +- 7 files changed, 116 insertions(+), 38 deletions(-) diff --git a/crates/hir-def/src/body/scope.rs b/crates/hir-def/src/body/scope.rs index e7078b7953..cab657b807 100644 --- a/crates/hir-def/src/body/scope.rs +++ b/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) -> impl Iterator + '_ { std::iter::successors(scope, move |&scope| self.scopes[scope].parent) } diff --git a/crates/hir-def/src/resolver.rs b/crates/hir-def/src/resolver.rs index 664db292a7..620e9202aa 100644 --- a/crates/hir-def/src/resolver.rs +++ b/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, 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, 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 @@ -478,8 +493,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, + 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 { self.scopes.iter().rev() @@ -576,10 +655,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 +667,18 @@ pub fn resolver_for_scope( owner: DefWithBodyId, scope_id: Option, ) -> 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, + scope_id: Option, + mut r: Resolver, + owner: DefWithBodyId, +) -> Resolver { let scope_chain = scopes.scope_chain(scope_id).collect::>(); r.scopes.reserve(scope_chain.len()); diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs index 22dcea8fcd..56ae786193 100644 --- a/crates/hir-ty/src/infer.rs +++ b/crates/hir-ty/src/infer.rs @@ -706,7 +706,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 +821,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 +846,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), } diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs index 8895dc095f..13ca053472 100644 --- a/crates/hir-ty/src/infer/expr.rs +++ b/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 resolver = resolver_for_expr(self.db.upcast(), self.owner, tgt_expr); - self.infer_path(&resolver, p, tgt_expr.into()).unwrap_or_else(|| self.err_ty()) + let g = self.resolver.update_to_inner_scope(self.db.upcast(), self.owner, tgt_expr); + let ty = self.infer_path(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 = - mem::replace(&mut self.resolver, resolver_for_expr(self.db.upcast(), self.owner, expr)); + let g = self.resolver.update_to_inner_scope(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) } diff --git a/crates/hir-ty/src/infer/pat.rs b/crates/hir-ty/src/infer/pat.rs index 3d03c2a527..a7bd009e34 100644 --- a/crates/hir-ty/src/infer/pat.rs +++ b/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 - let resolver = self.resolver.clone(); - self.infer_path(&resolver, path, pat.into()).unwrap_or_else(|| self.err_ty()) + // FIXME update resolver for the surrounding expression + self.infer_path(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); diff --git a/crates/hir-ty/src/infer/path.rs b/crates/hir-ty/src/infer/path.rs index b3867623f3..93dbd8b363 100644 --- a/crates/hir-ty/src/infer/path.rs +++ b/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,35 +21,25 @@ use crate::{ use super::{ExprOrPatId, InferenceContext, TraitRef}; impl<'a> InferenceContext<'a> { - pub(super) fn infer_path( - &mut self, - resolver: &Resolver, - path: &Path, - id: ExprOrPatId, - ) -> Option { - let ty = self.resolve_value_path(resolver, path, id)?; + pub(super) fn infer_path(&mut self, path: &Path, id: ExprOrPatId) -> Option { + let ty = self.resolve_value_path(path, id)?; let ty = self.insert_type_vars(ty); let ty = self.normalize_associated_types_in(ty); Some(ty) } - fn resolve_value_path( - &mut self, - resolver: &Resolver, - path: &Path, - id: ExprOrPatId, - ) -> Option { + fn resolve_value_path(&mut self, path: &Path, id: ExprOrPatId) -> Option { 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)? } 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), diff --git a/crates/hir-ty/src/tests.rs b/crates/hir-ty/src/tests.rs index 759878b10b..bcd63d9472 100644 --- a/crates/hir-ty/src/tests.rs +++ b/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