fix: Panic while canonicalizing erroneous projection type #17882

ShoyuVanilla · 2024-08-13T15:49:03Z

The root cause of #17866 is quite horrifyng 😨

trait T {
    type A;
}

type Foo = <S as T>::A; // note that S isn't defined

fn main() {
    Foo {}
}

While inferencing alias type Foo = <S as T>::A;

rust-analyzer/crates/hir-ty/src/infer.rs

Lines 1388 to 1398 in 78c2bdc

    
           TypeNs::TypeAliasId(it) => { 
        
               let resolved_seg = match unresolved { 
        
                   None => path.segments().last().unwrap(), 
        
                   Some(n) => path.segments().get(path.segments().len() - n - 1).unwrap(), 
        
               }; 
        
               let substs = 
        
                   ctx.substs_from_path_segment(resolved_seg, Some(it.into()), true, None); 
        
               let ty = self.db.ty(it.into()); 
        
               let ty = self.insert_type_vars(ty.substitute(Interner, &substs)); 
        
               self.resolve_variant_on_alias(ty, unresolved, mod_path)

the error type S in it is substituted by inference var in L1396 above as below;

rust-analyzer/crates/hir-ty/src/infer/unify.rs

Lines 866 to 869 in 78c2bdc

    
               /// Replaces `Ty::Error` by a new type var, so we can maybe still infer it. 
        
               pub(super) fn insert_type_vars_shallow(&mut self, ty: Ty) -> Ty { 
        
                   match ty.kind(Interner) { 
        
                       TyKind::Error => self.new_type_var(),

This new inference var's index is 1, as the type inferecing procedure here previously inserted another inference var into same InferenceTable.

But after that, the projection type made from the above then passed to the following function;

rust-analyzer/crates/hir-ty/src/traits.rs

Lines 88 to 96 in 78c2bdc

    
           pub(crate) fn normalize_projection_query( 
        
               db: &dyn HirDatabase, 
        
               projection: ProjectionTy, 
        
               env: Arc<TraitEnvironment>, 
        
           ) -> Ty { 
        
               let mut table = InferenceTable::new(db, env); 
        
               let ty = table.normalize_projection_ty(projection); 
        
               table.resolve_completely(ty) 
        
           }

here, a whole new InferenceTable is made, without any inference var and in the L94, this table calls;

rust-analyzer/crates/hir-ty/src/infer/unify.rs

Lines 364 to 370 in 78c2bdc

    
           pub(crate) fn normalize_projection_ty(&mut self, proj_ty: ProjectionTy) -> Ty { 
        
               let var = self.new_type_var(); 
        
               let alias_eq = AliasEq { alias: AliasTy::Projection(proj_ty), ty: var.clone() }; 
        
               let obligation = alias_eq.cast(Interner); 
        
               self.register_obligation(obligation); 
        
               var 
        
           }

And while registering AliasEq obligation, this obligation contains inference var ?1 made from the previous table, but this table has only one inference var ?0 made at L365.
So, the chalk panics when we try to canonicalize that obligation to register it, because the obligation contains an inference var ?1 that the canonicalizing table doesn't have.

Currently, we are calling InferenceTable::new() to do some normalizing, unifying or coercing things to some targets that might contain inference var that the new table doesn't have.
I think that this is quite dangerous footgun because the inference var is just an index that does not contain the information which table does it made from, so sometimes this "foreign" index might cause panic like this case, or point at the wrong variable.

This PR mitigates such behaviour simply by inserting sufficient number of inference vars to new table to avoid such problem.
This strategy doesn't harm current r-a's intention because the inference vars that passed into new tables are just "unresolved" variables in current r-a, so this is just making sure that such "unresolved" variables exist in the new table

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

davidbarsky · 2024-08-13T16:44:41Z

And while registering AliasEq obligation, this obligation contains inference var ?1 made from the previous table, but this table has only one inference var ?0 made at L365.
I think that this is quite dangerous footgun because the inference var is just an index that does not contain the information which table does it made from, so sometimes this "foreign" index might cause panic like this case, or point at the wrong variable.

I'm trying to remember their names, but there were a few arena crates whose keys tracked its creator's provenance. Do you think an approach like this would help here?

ShoyuVanilla · 2024-08-13T16:51:23Z

I'm trying to remember their names, but there were a few arena crates whose keys tracked its creator's provenance. Do you think an approach like this would help here?

I think that this PR is just a mitigation and what you said is the "correct" way to fix this, maybe.

davidbarsky · 2024-08-13T16:55:27Z

Yes, sorry! Lemme merge this. @bors r+

I'll make a new issue.

bors · 2024-08-13T16:55:30Z

📌 Commit 8bcaba1 has been approved by davidbarsky

It is now in the queue for this repository.

fix: Panic while canonicalizing erroneous projection type Fixes #17866 The root cause of #17866 is quite horrifyng 😨 ```rust trait T { type A; } type Foo = <S as T>::A; // note that S isn't defined fn main() { Foo {} } ``` While inferencing alias type `Foo = <S as T>::A`; https://github.com/rust-lang/rust-analyzer/blob/78c2bdce860dbd996a8083224d01a96660dd6a15/crates/hir-ty/src/infer.rs#L1388-L1398 the error type `S` in it is substituted by inference var in L1396 above as below; https://github.com/rust-lang/rust-analyzer/blob/78c2bdce860dbd996a8083224d01a96660dd6a15/crates/hir-ty/src/infer/unify.rs#L866-L869 This new inference var's index is `1`, as the type inferecing procedure here previously inserted another inference var into same `InferenceTable`. But after that, the projection type made from the above then passed to the following function; https://github.com/rust-lang/rust-analyzer/blob/78c2bdce860dbd996a8083224d01a96660dd6a15/crates/hir-ty/src/traits.rs#L88-L96 here, a whole new `InferenceTable` is made, without any inference var and in the L94, this table calls; https://github.com/rust-lang/rust-analyzer/blob/78c2bdce860dbd996a8083224d01a96660dd6a15/crates/hir-ty/src/infer/unify.rs#L364-L370 And while registering `AliasEq` `obligation`, this obligation contains inference var `?1` made from the previous table, but this table has only one inference var `?0` made at L365. So, the chalk panics when we try to canonicalize that obligation to register it, because the obligation contains an inference var `?1` that the canonicalizing table doesn't have. Currently, we are calling `InferenceTable::new()` to do some normalizing, unifying or coercing things to some targets that might contain inference var that the new table doesn't have. I think that this is quite dangerous footgun because the inference var is just an index that does not contain the information which table does it made from, so sometimes this "foreign" index might cause panic like this case, or point at the wrong variable. This PR mitigates such behaviour simply by inserting sufficient number of inference vars to new table to avoid such problem. This strategy doesn't harm current r-a's intention because the inference vars that passed into new tables are just "unresolved" variables in current r-a, so this is just making sure that such "unresolved" variables exist in the new table

bors · 2024-08-13T16:56:38Z

⌛ Testing commit 8bcaba1 with merge 0bc8501...

flodiebold · 2024-08-13T16:57:44Z

normalize_projection_query is a Salsa query, so its input should really be canonicalized (i.e. it should take a Canonical<ProjectionTy>). Canonicalization replaces inference vars by bound vars in order of appearance, which has the two effects of 1. "freeing" the type of the inference table it came from, and 2. making types that are equal in structure actually equal (you don't care if you have Option<?2> or Option<?45>), which is important for proper caching of the query. Taking a Canonical also serves as a check that there are no inference variables in the type.

On the other side, apparently this normalize_projection query gets called during inference, which is probably the cause of this bug (the only other place it gets called is in hir). That code should rather be using table.normalize_projection_ty directly.

flodiebold · 2024-08-13T16:57:54Z

@bors r-

davidbarsky · 2024-08-13T16:58:40Z

@bors r-

Sorry about that!

flodiebold · 2024-08-13T16:59:14Z

A bit too slow in writing my comment 😅

ShoyuVanilla · 2024-08-13T17:02:48Z

Okay, I'll rethink about this tommorrow(it's past midnight here 😅)

bors · 2024-08-13T17:08:35Z

☀️ Try build successful - checks-actions
Build commit: 0bc8501 (0bc85018e9660cd35a81007ed8ca8b58587fab9e)

flodiebold · 2024-08-13T17:14:55Z

Currently, we are calling InferenceTable::new() to do some normalizing, unifying or coercing things to some targets that might contain inference var that the new table doesn't have.

Basically, a type that still contains inference vars "crossing over" to a new inference table is always a bug. Most of the places you changed already take a Canonical, so they should be fine; a few are only intended for use by hir or analysis after type-checking. For these a debug assertion that the type doesn't contain inference vars might be good.

lnicola · 2024-08-13T17:34:05Z

there were a few arena crates whose keys tracked its creator's provenance

It no longer matters, but I think they're called generational indexes or arenas.

ShoyuVanilla · 2024-08-14T00:41:51Z

normalize_projection_query is a Salsa query, so its input should really be canonicalized (i.e. it should take a Canonical). Canonicalization replaces inference vars by bound vars in order of appearance, which has the two effects of 1. "freeing" the type of the inference table it came from, and 2. making types that are equal in structure actually equal (you don't care if you have Option<?2> or Option<?45>), which is important for proper caching of the query. Taking a Canonical also serves as a check that there are no inference variables in the type.

On the other side, apparently this normalize_projection query gets called during inference, which is probably the cause of this bug (the only other place it gets called is in hir). That code should rather be using table.normalize_projection_ty directly.

I was too obssesed with the projection type containing ivs and learned a lot from your comments. Thanks 😄

flodiebold

@bors delegate+

flodiebold · 2024-08-14T07:08:49Z

crates/hir-ty/src/infer.rs

            TyKind::Alias(AliasTy::Projection(proj_ty)) => {
-                self.db.normalize_projection(proj_ty.clone(), self.table.trait_env.clone())
+                let ty = self.table.normalize_projection_ty(proj_ty.clone());
+                self.table.resolve_completely(ty)


resolve_completely is the wrong thing to use during inference since it applies fallback. I think this should just be resolve_ty_shallow

flodiebold · 2024-08-14T07:13:54Z

crates/hir-ty/src/traits.rs

+    struct InferenceVarVisitor(Option<()>);
+    impl TypeVisitor<Interner> for InferenceVarVisitor {
+        type BreakTy = ();
+
+        fn as_dyn(&mut self) -> &mut dyn TypeVisitor<Interner, BreakTy = Self::BreakTy> {
+            self
+        }
+
+        fn interner(&self) -> Interner {
+            Interner
+        }
+
+        fn visit_inference_var(
+            &mut self,
+            _var: chalk_ir::InferenceVar,
+            _outer_binder: DebruijnIndex,
+        ) -> std::ops::ControlFlow<Self::BreakTy> {
+            self.0 = Some(());
+            std::ops::ControlFlow::Break(())
+        }
+    }
+
+    let mut visitor = InferenceVarVisitor(None);
+    projection.visit_with(visitor.as_dyn(), DebruijnIndex::INNERMOST);
+    if visitor.0.is_some() {


this could be a function has_inference_variables. Also, the flags in a Ty already store whether it contains any inference variable, so it could be easier to use that :)

ShoyuVanilla · 2024-08-14T15:09:35Z

@bors r+

bors · 2024-08-14T15:09:38Z

@ShoyuVanilla: 🔑 Insufficient privileges: Not in reviewers

lnicola · 2024-08-14T15:10:54Z

@bors r+

bors · 2024-08-14T15:10:56Z

📌 Commit e6d8970 has been approved by lnicola

It is now in the queue for this repository.

bors · 2024-08-14T15:12:05Z

⌛ Testing commit e6d8970 with merge 64a1405...

bors · 2024-08-14T15:25:58Z

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing 64a1405 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2024

ShoyuVanilla changed the title ~~fix: Panic while canonicalizing errornous projection type~~ fix: Panic while canonicalizing erroneous projection type Aug 13, 2024

ShoyuVanilla force-pushed the issue-17866 branch from 2b18567 to 84c562f Compare August 13, 2024 15:52

davidbarsky approved these changes Aug 13, 2024

View reviewed changes

crates/hir-ty/src/infer/unify.rs Outdated Show resolved Hide resolved

ShoyuVanilla force-pushed the issue-17866 branch from 84c562f to 8bcaba1 Compare August 13, 2024 16:36

ShoyuVanilla marked this pull request as draft August 13, 2024 17:07

ShoyuVanilla force-pushed the issue-17866 branch from 8bcaba1 to b01360c Compare August 14, 2024 00:37

ShoyuVanilla marked this pull request as ready for review August 14, 2024 00:39

This comment was marked as spam.

Sign in to view

flodiebold reviewed Aug 14, 2024

View reviewed changes

fix: Panic while canonicalizing erroneous projection type

e6d8970

ShoyuVanilla force-pushed the issue-17866 branch from b01360c to e6d8970 Compare August 14, 2024 14:17

bors merged commit 64a1405 into rust-lang:master Aug 14, 2024

ShoyuVanilla deleted the issue-17866 branch August 14, 2024 15:55

lnicola mentioned this pull request Aug 14, 2024

keeping panic when initialize struct with automatic type inference on associated type in alias declaration #17651

Closed

ShoyuVanilla mentioned this pull request Feb 19, 2025

Usage of Idx without its containing Arena context might be a footgun #19183

Open

	TypeNs::TypeAliasId(it) => {
	let resolved_seg = match unresolved {
	None => path.segments().last().unwrap(),
	Some(n) => path.segments().get(path.segments().len() - n - 1).unwrap(),
	};
	let substs =
	ctx.substs_from_path_segment(resolved_seg, Some(it.into()), true, None);
	let ty = self.db.ty(it.into());
	let ty = self.insert_type_vars(ty.substitute(Interner, &substs));

	self.resolve_variant_on_alias(ty, unresolved, mod_path)

	/// Replaces `Ty::Error` by a new type var, so we can maybe still infer it.
	pub(super) fn insert_type_vars_shallow(&mut self, ty: Ty) -> Ty {
	match ty.kind(Interner) {
	TyKind::Error => self.new_type_var(),

	pub(crate) fn normalize_projection_query(
	db: &dyn HirDatabase,
	projection: ProjectionTy,
	env: Arc<TraitEnvironment>,
	) -> Ty {
	let mut table = InferenceTable::new(db, env);
	let ty = table.normalize_projection_ty(projection);
	table.resolve_completely(ty)
	}

	pub(crate) fn normalize_projection_ty(&mut self, proj_ty: ProjectionTy) -> Ty {
	let var = self.new_type_var();
	let alias_eq = AliasEq { alias: AliasTy::Projection(proj_ty), ty: var.clone() };
	let obligation = alias_eq.cast(Interner);
	self.register_obligation(obligation);
	var
	}

fix: Panic while canonicalizing erroneous projection type #17882

fix: Panic while canonicalizing erroneous projection type #17882

Uh oh!

Conversation

ShoyuVanilla commented Aug 13, 2024

Uh oh!

Uh oh!

davidbarsky commented Aug 13, 2024

Uh oh!

ShoyuVanilla commented Aug 13, 2024

Uh oh!

davidbarsky commented Aug 13, 2024

Uh oh!

bors commented Aug 13, 2024

Uh oh!

bors commented Aug 13, 2024

Uh oh!

flodiebold commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flodiebold commented Aug 13, 2024

Uh oh!

davidbarsky commented Aug 13, 2024

Uh oh!

flodiebold commented Aug 13, 2024

Uh oh!

ShoyuVanilla commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 13, 2024

Uh oh!

flodiebold commented Aug 13, 2024

Uh oh!

lnicola commented Aug 13, 2024

Uh oh!

ShoyuVanilla commented Aug 14, 2024

Uh oh!

This comment was marked as spam.

Uh oh!

flodiebold left a comment

Choose a reason for hiding this comment

Uh oh!

flodiebold Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

flodiebold Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

ShoyuVanilla commented Aug 14, 2024

Uh oh!

bors commented Aug 14, 2024

Uh oh!

lnicola commented Aug 14, 2024

Uh oh!

bors commented Aug 14, 2024

Uh oh!

bors commented Aug 14, 2024

Uh oh!

bors commented Aug 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

flodiebold commented Aug 13, 2024 •

edited

Loading

ShoyuVanilla commented Aug 13, 2024 •

edited

Loading