Unnamed repository; edit this file 'description' to name the repository.
Merge pull request #22129 from ChayimFriedman2/destructuring-assign-path
internal: Do not lower normal assignments into `Pat::Path`
Shoyu Vanilla (Flint) 4 weeks ago
parent 0e2d810 · parent 9a95464 · commit 0ca8613
-rw-r--r--crates/hir-def/src/expr_store/lower.rs50
-rw-r--r--crates/hir-def/src/hir.rs1
-rw-r--r--crates/hir-ty/src/infer/closure/analysis/expr_use_visitor.rs39
-rw-r--r--crates/hir-ty/src/infer/expr.rs25
-rw-r--r--crates/hir-ty/src/infer/pat.rs4
-rw-r--r--crates/hir-ty/src/tests/simple.rs1
-rw-r--r--crates/hir-ty/src/upvars.rs18
-rw-r--r--crates/hir/src/semantics.rs19
8 files changed, 55 insertions, 102 deletions
diff --git a/crates/hir-def/src/expr_store/lower.rs b/crates/hir-def/src/expr_store/lower.rs
index 04437a59ac..4ace4aef16 100644
--- a/crates/hir-def/src/expr_store/lower.rs
+++ b/crates/hir-def/src/expr_store/lower.rs
@@ -13,6 +13,7 @@ use cfg::CfgOptions;
use either::Either;
use hir_expand::{
HirFileId, InFile, MacroDefId,
+ mod_path::ModPath,
name::{AsName, Name},
span_map::SpanMapRef,
};
@@ -1611,6 +1612,37 @@ impl<'db> ExprCollector<'db> {
})
}
+ /// Whether this path should be lowered as destructuring assignment, or as a normal assignment.
+ fn path_is_destructuring_assignment(&self, path: &ModPath) -> bool {
+ // rustc has access to a full resolver here, including local variables and generic params, and it checks the following
+ // criteria: a path not lowered as destructuring assignment if it can *fully resolve* to something that is *not*
+ // a const, a unit struct or a variant.
+ // We don't have access to a full resolver here. So we should do the same as rustc, but assuming that local variables
+ // could be resolved to nothing (fortunately, there cannot be a local variable shadowing a unit struct/variant/const,
+ // as that is an error). We don't need to consider const params as it's an error to refer to these in patterns.
+ let (resolution, unresolved_idx, _) = self.def_map.resolve_path_locally(
+ self.local_def_map,
+ self.db,
+ self.module,
+ path,
+ BuiltinShadowMode::Other,
+ );
+ match unresolved_idx {
+ Some(_) => {
+ // If `Some(_)`, path could be resolved to unit struct/variant/const with type information, i.e. an assoc type or const.
+ // If `None`, path could be a local variable.
+ resolution.take_types().is_some()
+ }
+ None => match resolution.take_values() {
+ // We don't need to consider non-unit structs/variants, as those are not value types.
+ Some(ModuleDefId::EnumVariantId(_))
+ | Some(ModuleDefId::AdtId(_))
+ | Some(ModuleDefId::ConstId(_)) => true,
+ _ => false,
+ },
+ }
+ }
+
fn collect_expr_as_pat_opt(&mut self, expr: Option<ast::Expr>) -> PatId {
match expr {
Some(expr) => self.collect_expr_as_pat(expr),
@@ -1676,15 +1708,17 @@ impl<'db> ExprCollector<'db> {
self.alloc_pat_from_expr(Pat::TupleStruct { path, args, ellipsis }, syntax_ptr)
}
ast::Expr::PathExpr(e) => {
- let (path, hygiene) = self
- .collect_expr_path(e.clone())
- .map(|(path, hygiene)| (Pat::Path(path), hygiene))
- .unwrap_or((Pat::Missing, HygieneId::ROOT));
- let pat_id = self.alloc_pat_from_expr(path, syntax_ptr);
- if !hygiene.is_root() {
- self.store.ident_hygiene.insert(pat_id.into(), hygiene);
+ let (path, hygiene) = self.collect_expr_path(e.clone())?;
+ let mod_path = path.mod_path().expect("should not lower to lang path");
+ if self.path_is_destructuring_assignment(mod_path) {
+ let pat_id = self.alloc_pat_from_expr(Pat::Path(path), syntax_ptr);
+ if !hygiene.is_root() {
+ self.store.ident_hygiene.insert(pat_id.into(), hygiene);
+ }
+ pat_id
+ } else {
+ return None;
}
- pat_id
}
ast::Expr::MacroExpr(e) => {
let e = e.macro_call()?;
diff --git a/crates/hir-def/src/hir.rs b/crates/hir-def/src/hir.rs
index 9e51d0eac9..031280b6e9 100644
--- a/crates/hir-def/src/hir.rs
+++ b/crates/hir-def/src/hir.rs
@@ -687,7 +687,6 @@ pub enum Pat {
slice: Option<PatId>,
suffix: Box<[PatId]>,
},
- /// This might refer to a variable if a single segment path (specifically, on destructuring assignment).
Path(Path),
Lit(ExprId),
Bind {
diff --git a/crates/hir-ty/src/infer/closure/analysis/expr_use_visitor.rs b/crates/hir-ty/src/infer/closure/analysis/expr_use_visitor.rs
index 099fa18168..45696c402a 100644
--- a/crates/hir-ty/src/infer/closure/analysis/expr_use_visitor.rs
+++ b/crates/hir-ty/src/infer/closure/analysis/expr_use_visitor.rs
@@ -446,14 +446,6 @@ pub(crate) struct ExprUseVisitor<'a, 'b, 'db, D: Delegate<'db>> {
upvars: UpvarsRef<'db>,
}
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-enum PatWalkMode {
- /// `let`, `match`.
- Declaration,
- /// Destructuring assignment.
- Assignment,
-}
-
impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
/// Creates the ExprUseVisitor, configuring it with the various options provided:
///
@@ -477,7 +469,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
let param_place = self.cat_rvalue(param.into(), param_ty);
self.fake_read_scrutinee(param_place.clone(), false);
- self.walk_pat(param_place, param, false, PatWalkMode::Declaration)?;
+ self.walk_pat(param_place, param, false)?;
}
self.consume_expr(body)?;
@@ -701,7 +693,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
let expr_place = self.cat_expr(value)?;
let update_guard =
self.cx.resolver.update_to_inner_scope(self.cx.db, self.cx.owner, expr);
- self.walk_pat(expr_place, target, false, PatWalkMode::Assignment)?;
+ self.walk_pat(expr_place, target, false)?;
self.cx.resolver.reset_to_guard(update_guard);
}
@@ -784,7 +776,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
let expr_place = self.cat_expr(expr)?;
f(self)?;
self.fake_read_scrutinee(expr_place.clone(), els.is_some());
- self.walk_pat(expr_place, pat, false, PatWalkMode::Declaration)?;
+ self.walk_pat(expr_place, pat, false)?;
if let Some(els) = els {
self.walk_expr(els)?;
}
@@ -896,7 +888,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
}
fn walk_arm(&mut self, discr_place: PlaceWithOrigin, arm: &MatchArm) -> Result {
- self.walk_pat(discr_place, arm.pat, arm.guard.is_some(), PatWalkMode::Declaration)?;
+ self.walk_pat(discr_place, arm.pat, arm.guard.is_some())?;
if let Some(e) = arm.guard {
self.consume_expr(e)?;
@@ -920,13 +912,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
/// Do note that discrepancies like these do still create obscure corners
/// in the semantics of the language, and should be avoided if possible.
#[instrument(skip(self), level = "debug")]
- fn walk_pat(
- &mut self,
- discr_place: PlaceWithOrigin,
- pat: PatId,
- has_guard: bool,
- mode: PatWalkMode,
- ) -> Result {
+ fn walk_pat(&mut self, discr_place: PlaceWithOrigin, pat: PatId, has_guard: bool) -> Result {
self.cat_pattern(discr_place.clone(), pat, &mut |this, place, pat| {
debug!("walk_pat: pat.kind={:?}", this.cx.store[pat]);
let read_discriminant = {
@@ -987,13 +973,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
this.cx.store.pat_path_hygiene(pat),
);
let is_normal_const = matches!(resolution, Some(ValueNs::ConstId(_)));
- if mode == PatWalkMode::Assignment
- && let Some(ValueNs::LocalBinding(local)) = resolution
- {
- let pat_ty = this.pat_ty(pat)?;
- let place = this.cat_local(pat.into(), pat_ty, local)?;
- this.delegate.mutate(place, this.cx);
- } else if is_assoc_const || is_normal_const {
+ if is_assoc_const || is_normal_const {
// Named constants have to be equated with the value
// being matched, so that's a read of the value being matched.
//
@@ -1035,7 +1015,7 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
read_discriminant(this);
}
}
- Pat::Expr(expr) if mode == PatWalkMode::Assignment => {
+ Pat::Expr(expr) => {
// Destructuring assignment.
this.mutate_expr(expr)?;
}
@@ -1050,7 +1030,6 @@ impl<'a, 'b, 'db, D: Delegate<'db>> ExprUseVisitor<'a, 'b, 'db, D> {
// If the PatKind is Missing, Wild or Err, any relevant accesses are made when processing
// the other patterns that are part of the match
}
- Pat::Expr(_) => {}
}
Ok(())
@@ -1196,10 +1175,6 @@ impl<'db, D: Delegate<'db>> ExprUseVisitor<'_, '_, 'db, D> {
self.node_ty(expr.into())
}
- fn pat_ty(&mut self, pat: PatId) -> Result<Ty<'db>> {
- self.node_ty(pat.into())
- }
-
fn expr_ty_adjusted(&mut self, expr: ExprId) -> Result<Ty<'db>> {
self.expect_and_resolve_type(self.cx.result.type_of_expr_with_adjust(expr))
}
diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs
index d80ea71674..f26be63806 100644
--- a/crates/hir-ty/src/infer/expr.rs
+++ b/crates/hir-ty/src/infer/expr.rs
@@ -299,7 +299,7 @@ impl<'db> InferenceContext<'_, 'db> {
}
#[tracing::instrument(level = "debug", skip(self, is_read), ret)]
- fn infer_expr_inner(
+ pub(super) fn infer_expr_inner(
&mut self,
tgt_expr: ExprId,
expected: &Expectation<'db>,
@@ -715,29 +715,6 @@ impl<'db> InferenceContext<'_, 'db> {
&Pat::Expr(expr) => {
Some(self.infer_expr(expr, &Expectation::none(), ExprIsRead::No))
}
- Pat::Path(path) => {
- let resolver_guard =
- self.resolver.update_to_inner_scope(self.db, self.owner, tgt_expr);
- let resolution = self.resolver.resolve_path_in_value_ns_fully(
- self.db,
- path,
- self.store.pat_path_hygiene(target),
- );
- self.resolver.reset_to_guard(resolver_guard);
-
- if matches!(
- resolution,
- Some(
- ValueNs::ConstId(_)
- | ValueNs::StructId(_)
- | ValueNs::EnumVariantId(_)
- )
- ) {
- None
- } else {
- Some(self.infer_expr_path(path, target.into(), tgt_expr))
- }
- }
_ => None,
};
let is_destructuring_assignment = lhs_ty.is_none();
diff --git a/crates/hir-ty/src/infer/pat.rs b/crates/hir-ty/src/infer/pat.rs
index 8033680dcc..a437253672 100644
--- a/crates/hir-ty/src/infer/pat.rs
+++ b/crates/hir-ty/src/infer/pat.rs
@@ -241,8 +241,6 @@ impl<'db> InferenceContext<'_, 'db> {
pat_ty
}
- /// The resolver needs to be updated to the surrounding expression when inside assignment
- /// (because there, `Pat::Path` can refer to a variable).
pub(super) fn infer_top_pat(
&mut self,
pat: PatId,
@@ -405,7 +403,7 @@ impl<'db> InferenceContext<'_, 'db> {
// LHS of assignment doesn't constitute reads.
let expr_is_read = ExprIsRead::No;
let result =
- self.infer_expr_coerce(*expr, &Expectation::has_type(expected), expr_is_read);
+ self.infer_expr_inner(*expr, &Expectation::has_type(expected), expr_is_read);
// We are returning early to avoid the unifiability check below.
let lhs_ty = self.insert_type_vars_shallow(result);
let ty = match self.coerce(
diff --git a/crates/hir-ty/src/tests/simple.rs b/crates/hir-ty/src/tests/simple.rs
index 3ea21f8265..c30b24cd27 100644
--- a/crates/hir-ty/src/tests/simple.rs
+++ b/crates/hir-ty/src/tests/simple.rs
@@ -2271,6 +2271,7 @@ fn infer_generic_from_later_assignment() {
89..127 'loop {... }': !
94..127 '{ ... }': ()
104..107 'end': Option<bool>
+ 104..107 'end': Option<bool>
104..120 'end = ...(true)': ()
110..114 'Some': fn Some<bool>(bool) -> Option<bool>
110..120 'Some(true)': Option<bool>
diff --git a/crates/hir-ty/src/upvars.rs b/crates/hir-ty/src/upvars.rs
index 48f3c803d8..026e6ab183 100644
--- a/crates/hir-ty/src/upvars.rs
+++ b/crates/hir-ty/src/upvars.rs
@@ -3,7 +3,7 @@
use hir_def::{
DefWithBodyId, ExpressionStoreOwnerId, GenericDefId, VariantId,
expr_store::{ExpressionStore, path::Path},
- hir::{BindingId, Expr, ExprId, ExprOrPatId, Pat},
+ hir::{BindingId, Expr, ExprId, ExprOrPatId},
resolver::{HasResolver, Resolver, ValueNs},
};
use hir_expand::mod_path::PathKind;
@@ -181,22 +181,6 @@ pub fn upvars_mentioned_impl(
path,
);
}
- &Expr::Assignment { target, .. } => {
- body.walk_pats(target, &mut |pat| {
- let Pat::Path(path) = &body[pat] else { return };
- resolve_maybe_upvar(
- db,
- resolver,
- owner,
- body,
- current_closure,
- expr,
- pat.into(),
- upvars,
- path,
- );
- });
- }
&Expr::Closure { body: body_expr, .. } => {
let mut closure_upvars = FxHashSet::default();
handle_expr_inside_closure(
diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs
index 38656e1870..9d97dc7421 100644
--- a/crates/hir/src/semantics.rs
+++ b/crates/hir/src/semantics.rs
@@ -17,7 +17,7 @@ use hir_def::{
TraitId, VariantId,
attrs::parse_extra_crate_attrs,
expr_store::{Body, ExprOrPatSource, ExpressionStore, HygieneId, path::Path},
- hir::{BindingId, Expr, ExprId, ExprOrPatId, Pat},
+ hir::{BindingId, Expr, ExprId, ExprOrPatId},
nameres::{ModuleOrigin, crate_def_map},
resolver::{self, HasResolver, Resolver, TypeNs, ValueNs},
type_ref::Mutability,
@@ -2409,13 +2409,7 @@ impl<'db> SemanticsImpl<'db> {
None
}
}
- ExprOrPatId::PatId(pat_id) => {
- if let Pat::Path(path) = &store[pat_id] {
- Some(path)
- } else {
- None
- }
- }
+ ExprOrPatId::PatId(_) => None,
};
if let Some(path) = path
@@ -2799,15 +2793,6 @@ impl RenameConflictsVisitor<'_> {
self.resolve_path(expr.into(), path);
self.resolver.reset_to_guard(guard);
}
- &Expr::Assignment { target, .. } => {
- let guard = self.resolver.update_to_inner_scope(self.db, self.owner, expr);
- self.body.walk_pats(target, &mut |pat| {
- if let Pat::Path(path) = &self.body[pat] {
- self.resolve_path(pat.into(), path);
- }
- });
- self.resolver.reset_to_guard(guard);
- }
_ => {}
}