From e3f200aa291a00298a4ca3e0ddfc4ebfed2ddafc Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Thu, 25 Mar 2021 20:31:22 +0100 Subject: [PATCH] Add binders validator This is a visitor which checks that all bound vars refer to an existing binder of the right type. This has been quite useful for me: we somewhat regularly have errors in the binders which then cause a crash some time down the line when something tries to substitute them. At that point it's usually harder to find the actual place the error comes from; also you don't always hit the error. For the latest of these bugs, I added this validator, and immediately had failing tests that pointed me to the right location. The disadvantage is that I needed to add some rather specific methods to `Visitor`. I didn't find a better way to model this (and this is also the reason I couldn't just implement it in rust-analyzer). --- chalk-ir/src/visit.rs | 9 ++ chalk-ir/src/visit/binder_impls.rs | 20 ++- chalk-ir/src/visit/visitors.rs | 130 ++++++++++++++++++ .../src/clauses/builtin_traits/unsize.rs | 4 +- 4 files changed, 156 insertions(+), 7 deletions(-) diff --git a/chalk-ir/src/visit.rs b/chalk-ir/src/visit.rs index c7180d76966..4c0c8de1f74 100644 --- a/chalk-ir/src/visit.rs +++ b/chalk-ir/src/visit.rs @@ -228,6 +228,15 @@ where } } + /// Invoked for each occurence of a `Binders`, before visiting them. + fn before_binders(&mut self, _kinds: &crate::VariableKinds) {} + /// Invoked for each occurence of a `Canonical`, before visiting them. + fn before_canonical(&mut self, _kinds: &crate::CanonicalVarKinds) {} + /// Invoked for each occurence of a `FnPointer`, before visiting them. + fn before_fn_pointer_substs(&mut self, _number: usize) {} + /// Invoked for each occurence of a type introducing binders, after visiting them. + fn after_any_binders(&mut self) {} + /// Gets the visitor's interner. fn interner(&self) -> &'i I; } diff --git a/chalk-ir/src/visit/binder_impls.rs b/chalk-ir/src/visit/binder_impls.rs index 3907dcba452..f745674f5e4 100644 --- a/chalk-ir/src/visit/binder_impls.rs +++ b/chalk-ir/src/visit/binder_impls.rs @@ -15,14 +15,18 @@ impl Visit for FnPointer { where I: 'i, { - self.substitution - .visit_with(visitor, outer_binder.shifted_in()) + visitor.before_fn_pointer_substs(self.num_binders); + let result = self + .substitution + .visit_with(visitor, outer_binder.shifted_in()); + visitor.after_any_binders(); + result } } impl Visit for Binders where - T: HasInterner + Visit, + T: HasInterner + Visit, { fn visit_with<'i, B>( &self, @@ -32,7 +36,10 @@ where where I: 'i, { - self.value.visit_with(visitor, outer_binder.shifted_in()) + visitor.before_binders(&self.binders); + let result = self.value.visit_with(visitor, outer_binder.shifted_in()); + visitor.after_any_binders(); + result } } @@ -49,6 +56,9 @@ where where I: 'i, { - self.value.visit_with(visitor, outer_binder.shifted_in()) + visitor.before_canonical(&self.binders); + let result = self.value.visit_with(visitor, outer_binder.shifted_in()); + visitor.after_any_binders(); + result } } diff --git a/chalk-ir/src/visit/visitors.rs b/chalk-ir/src/visit/visitors.rs index a34514fe725..c81f8e54c31 100644 --- a/chalk-ir/src/visit/visitors.rs +++ b/chalk-ir/src/visit/visitors.rs @@ -12,6 +12,18 @@ pub trait VisitExt: Visit { ) .is_break() } + + /// Validates binders. + fn validate_binders(&self, interner: &I) -> bool { + self.visit_with( + &mut binders_check::BindersValidator { + interner, + stack: Vec::new(), + }, + DebruijnIndex::INNERMOST, + ) + .is_continue() + } } impl VisitExt for T where T: Visit {} @@ -39,3 +51,121 @@ impl<'i, I: Interner> Visitor<'i, I> for FindFreeVarsVisitor<'i, I> { ControlFlow::BREAK } } + +mod binders_check { + use crate::{ + interner::Interner, + visit::{ControlFlow, SuperVisit, Visitor}, + Const, DebruijnIndex, Lifetime, Ty, VariableKind, + }; + + pub struct BindersValidator<'i, I: Interner> { + pub(crate) interner: &'i I, + pub(crate) stack: Vec>>, + } + + impl<'i, I: Interner> Visitor<'i, I> for BindersValidator<'i, I> { + type BreakTy = (); + + fn as_dyn(&mut self) -> &mut dyn Visitor<'i, I, BreakTy = Self::BreakTy> { + self + } + + fn interner(&self) -> &'i I { + self.interner + } + + fn visit_ty( + &mut self, + ty: &Ty, + outer_binder: DebruijnIndex, + ) -> ControlFlow { + if let Some(bv) = ty.bound_var(self.interner) { + assert_eq!(self.stack.len(), outer_binder.depth() as usize); + if bv.debruijn < outer_binder { + let kinds = &self.stack[self.stack.len() - 1 - bv.debruijn.depth() as usize]; + match kinds.get(bv.index) { + Some(VariableKind::Ty(_)) => {} + _ => { + return ControlFlow::BREAK; + } + } + } else { + return ControlFlow::BREAK; + } + } + ty.super_visit_with(self.as_dyn(), outer_binder) + } + + fn visit_const( + &mut self, + constant: &Const, + outer_binder: DebruijnIndex, + ) -> ControlFlow { + if let Some(bv) = constant.bound_var(self.interner) { + assert_eq!(self.stack.len(), outer_binder.depth() as usize); + if bv.debruijn < outer_binder { + let kinds = &self.stack[self.stack.len() - 1 - bv.debruijn.depth() as usize]; + match kinds.get(bv.index) { + Some(VariableKind::Const(_ty)) => { + // FIXME: validate that type can match? + } + _ => { + return ControlFlow::BREAK; + } + } + } else { + return ControlFlow::BREAK; + } + } + constant.super_visit_with(self.as_dyn(), outer_binder) + } + + fn visit_lifetime( + &mut self, + lifetime: &Lifetime, + outer_binder: DebruijnIndex, + ) -> ControlFlow { + if let Some(bv) = lifetime.bound_var(self.interner) { + assert_eq!(self.stack.len(), outer_binder.depth() as usize); + if bv.debruijn < outer_binder { + let kinds = &self.stack[self.stack.len() - 1 - bv.debruijn.depth() as usize]; + match kinds.get(bv.index) { + Some(VariableKind::Lifetime) => {} + _ => { + return ControlFlow::BREAK; + } + } + } else { + return ControlFlow::BREAK; + } + } + lifetime.super_visit_with(self.as_dyn(), outer_binder) + } + + fn before_binders(&mut self, kinds: &crate::VariableKinds) { + self.stack.push(kinds.as_slice(self.interner).to_vec()); + } + + fn before_canonical(&mut self, kinds: &crate::CanonicalVarKinds) { + self.stack.push( + kinds + .iter(self.interner) + .map(|wk| wk.kind.clone()) + .collect(), + ); + } + + fn before_fn_pointer_substs(&mut self, number: usize) { + self.stack.push( + std::iter::repeat_with(|| VariableKind::Lifetime) + .take(number) + .collect(), + ) + } + + fn after_any_binders(&mut self) { + self.stack.pop().unwrap(); + } + } +} diff --git a/chalk-solve/src/clauses/builtin_traits/unsize.rs b/chalk-solve/src/clauses/builtin_traits/unsize.rs index 667380836ff..41eba734fbb 100644 --- a/chalk-solve/src/clauses/builtin_traits/unsize.rs +++ b/chalk-solve/src/clauses/builtin_traits/unsize.rs @@ -59,7 +59,7 @@ impl<'a, I: Interner> Visitor<'a, I> for UnsizeParameterCollector<'a, I> { fn outer_binder_parameters_used( interner: &I, - v: &Binders + HasInterner>, + v: &Binders + HasInterner>, ) -> HashSet { let mut visitor = UnsizeParameterCollector { interner, @@ -123,7 +123,7 @@ impl<'a, 'p, I: Interner> Visitor<'a, I> for ParameterOccurenceCheck<'a, 'p, I> fn uses_outer_binder_params( interner: &I, - v: &Binders + HasInterner>, + v: &Binders + HasInterner>, parameters: &HashSet, ) -> bool { let mut visitor = ParameterOccurenceCheck {