From 01eab9c5bd2db129364ca2d236e262348fd4e0ac Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Fri, 18 Feb 2022 14:29:56 +0100 Subject: [PATCH 1/5] refactor: Rename apply_ref to visit --- libflux/flux-core/src/errors.rs | 4 +- libflux/flux-core/src/semantic/bootstrap.rs | 2 +- libflux/flux-core/src/semantic/env.rs | 4 +- libflux/flux-core/src/semantic/infer.rs | 4 +- libflux/flux-core/src/semantic/nodes.rs | 4 +- libflux/flux-core/src/semantic/sub.rs | 40 ++++++++-------- libflux/flux-core/src/semantic/types.rs | 52 ++++++++++----------- 7 files changed, 56 insertions(+), 54 deletions(-) diff --git a/libflux/flux-core/src/errors.rs b/libflux/flux-core/src/errors.rs index 7d7d991af4..1d42e903d4 100644 --- a/libflux/flux-core/src/errors.rs +++ b/libflux/flux-core/src/errors.rs @@ -205,8 +205,8 @@ impl Substitutable for Located where E: Substitutable, { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - self.error.apply_ref(sub).map(|error| Located { + fn walk(&self, sub: &dyn Substituter) -> Option { + self.error.visit(sub).map(|error| Located { location: self.location.clone(), error, }) diff --git a/libflux/flux-core/src/semantic/bootstrap.rs b/libflux/flux-core/src/semantic/bootstrap.rs index d1f4082b21..abccbc0325 100644 --- a/libflux/flux-core/src/semantic/bootstrap.rs +++ b/libflux/flux-core/src/semantic/bootstrap.rs @@ -297,7 +297,7 @@ fn add_record_to_map( for field in r.fields() { let new_vars = { let new_vars = CollectBoundVars(RefCell::new(Vec::new())); - field.v.apply_ref(&new_vars); + head.v.visit(&new_vars); new_vars.0.into_inner() }; diff --git a/libflux/flux-core/src/semantic/env.rs b/libflux/flux-core/src/semantic/env.rs index 1c5610e73e..f6a5e54d1f 100644 --- a/libflux/flux-core/src/semantic/env.rs +++ b/libflux/flux-core/src/semantic/env.rs @@ -34,7 +34,7 @@ impl fmt::Display for Environment<'_> { } impl Substitutable for Environment<'_> { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match (self.readwrite, &self.parent) { // This is a performance optimization where false implies // this is the top-level of the type environment and apply @@ -43,7 +43,7 @@ impl Substitutable for Environment<'_> { // Even though this is the top-level of the type environment // and apply should be a no-op, readwrite is set to true so // we apply anyway. - (true, None) => self.values.apply_ref(sub).map(|values| Environment { + (true, None) => self.values.visit(sub).map(|values| Environment { external: self.external, parent: None, values, diff --git a/libflux/flux-core/src/semantic/infer.rs b/libflux/flux-core/src/semantic/infer.rs index 2bb395393b..bc048215c3 100644 --- a/libflux/flux-core/src/semantic/infer.rs +++ b/libflux/flux-core/src/semantic/infer.rs @@ -110,8 +110,8 @@ pub struct Error { impl std::error::Error for Error {} impl Substitutable for Error { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - self.err.apply_ref(sub).map(|err| Error { + fn walk(&self, sub: &dyn Substituter) -> Option { + self.err.visit(sub).map(|err| Error { loc: self.loc.clone(), err, }) diff --git a/libflux/flux-core/src/semantic/nodes.rs b/libflux/flux-core/src/semantic/nodes.rs index bda40cf6fc..de089861e9 100644 --- a/libflux/flux-core/src/semantic/nodes.rs +++ b/libflux/flux-core/src/semantic/nodes.rs @@ -80,9 +80,9 @@ impl AsDiagnostic for ErrorKind { } impl Substitutable for ErrorKind { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match self { - Self::Inference(err) => err.apply_ref(sub).map(Self::Inference), + Self::Inference(err) => err.visit(sub).map(Self::Inference), Self::UndefinedBuiltin(_) | Self::UndefinedIdentifier(_) | Self::InvalidBinOp(_) diff --git a/libflux/flux-core/src/semantic/sub.rs b/libflux/flux-core/src/semantic/sub.rs index 9ad5a59a26..b77a03879c 100644 --- a/libflux/flux-core/src/semantic/sub.rs +++ b/libflux/flux-core/src/semantic/sub.rs @@ -154,7 +154,7 @@ pub trait Substitutable { where Self: Sized, { - self.apply_ref(sub).unwrap_or(self) + self.visit(sub).unwrap_or(self) } /// Apply a substitution to a type variable. @@ -162,7 +162,7 @@ pub trait Substitutable { where Self: Sized, { - if let Some(new) = self.apply_ref(sub) { + if let Some(new) = self.visit(sub) { *self = new; } } @@ -172,7 +172,7 @@ pub trait Substitutable { where Self: Clone + Sized, { - match self.apply_ref(sub) { + match self.visit(sub) { Some(t) => Cow::Owned(t), None => Cow::Borrowed(self), } @@ -180,7 +180,16 @@ pub trait Substitutable { /// Apply a substitution to a type variable. Should return `None` if there was nothing to apply /// which allows for optimizations. - fn apply_ref(&self, sub: &dyn Substituter) -> Option + fn visit(&self, sub: &dyn Substituter) -> Option + where + Self: Sized, + { + self.walk(sub) + } + + /// Apply a substitution to a type variable. Should return `None` if there was nothing to apply + /// which allows for optimizations. + fn walk(&self, sub: &dyn Substituter) -> Option where Self: Sized; @@ -199,8 +208,8 @@ impl Substitutable for Box where T: Substitutable, { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - T::apply_ref(self, sub).map(Box::new) + fn walk(&self, sub: &dyn Substituter) -> Option { + T::visit(self, sub).map(Box::new) } fn free_vars(&self, vars: &mut Vec) { T::free_vars(self, vars) @@ -285,13 +294,13 @@ where { merge4( a, - a.apply_ref(sub), + a.visit(sub), b, - b.apply_ref(sub), + b.visit(sub), c, - c.apply_ref(sub), + c.visit(sub), d, - d.apply_ref(sub), + d.visit(sub), ) } @@ -301,14 +310,7 @@ where B: Substitutable + Clone, C: Substitutable + Clone, { - merge3( - a, - a.apply_ref(sub), - b, - b.apply_ref(sub), - c, - c.apply_ref(sub), - ) + merge3(a, a.visit(sub), b, b.visit(sub), c, c.visit(sub)) } pub(crate) fn apply2(a: &A, b: &B, sub: &dyn Substituter) -> Option<(A, B)> @@ -316,7 +318,7 @@ where A: Substitutable + Clone, B: Substitutable + Clone, { - merge(a, a.apply_ref(sub), b, b.apply_ref(sub)) + merge(a, a.visit(sub), b, b.visit(sub)) } #[allow(clippy::too_many_arguments, clippy::type_complexity)] diff --git a/libflux/flux-core/src/semantic/types.rs b/libflux/flux-core/src/semantic/types.rs index bf7630efe0..b528d68281 100644 --- a/libflux/flux-core/src/semantic/types.rs +++ b/libflux/flux-core/src/semantic/types.rs @@ -113,11 +113,11 @@ impl PartialEq for PolyType { } impl Substitutable for PolyType { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { // `vars` defines new distinct variables for `expr` so any substitutions applied on a // variable named the same must not be applied in `expr` self.expr - .apply_ref(&|var| { + .visit(&|var| { if self.vars.contains(&var) { None } else { @@ -298,15 +298,15 @@ impl fmt::Display for Error { } impl Substitutable for Error { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match self { Error::CannotUnify { exp, act } => { apply2(exp, act, sub).map(|(exp, act)| Error::CannotUnify { exp, act }) } Error::CannotConstrain { exp, act } => act - .apply_ref(sub) + .visit(sub) .map(|act| Error::CannotConstrain { exp: *exp, act }), - Error::OccursCheck(tv, ty) => ty.apply_ref(sub).map(|ty| Error::OccursCheck(*tv, ty)), + Error::OccursCheck(tv, ty) => ty.visit(sub).map(|ty| Error::OccursCheck(*tv, ty)), Error::CannotUnifyLabel { lab, exp, @@ -319,7 +319,7 @@ impl Substitutable for Error { cause, }), Error::CannotUnifyArgument(x, e) => e - .apply_ref(sub) + .visit(sub) .map(|e| Error::CannotUnifyArgument(x.clone(), e)), Error::CannotUnifyReturn { exp, act, cause } => apply3(exp, act, cause, sub) .map(|(exp, act, cause)| Error::CannotUnifyReturn { exp, act, cause }), @@ -415,8 +415,8 @@ impl FromStr for Kind { pub type Ptr = std::sync::Arc; impl Substitutable for Ptr { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - T::apply_ref(self, sub).map(Ptr::new) + fn walk(&self, sub: &dyn Substituter) -> Option { + T::visit(self, sub).map(Ptr::new) } fn free_vars(&self, vars: &mut Vec) { T::free_vars(self, vars) @@ -697,7 +697,7 @@ impl BuiltinType { } impl Substitutable for MonoType { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match self { MonoType::Error | MonoType::Builtin(_) => None, MonoType::BoundVar(tvr) => sub.try_apply_bound(*tvr).map(|new| { @@ -730,10 +730,10 @@ impl Substitutable for MonoType { new.apply(sub) } }), - MonoType::Collection(app) => app.apply_ref(sub).map(MonoType::app), - MonoType::Dict(dict) => dict.apply_ref(sub).map(MonoType::dict), - MonoType::Record(obj) => obj.apply_ref(sub).map(MonoType::record), - MonoType::Fun(fun) => fun.apply_ref(sub).map(MonoType::fun), + MonoType::Collection(app) => app.visit(sub).map(MonoType::app), + MonoType::Dict(dict) => dict.visit(sub).map(MonoType::dict), + MonoType::Record(obj) => obj.visit(sub).map(MonoType::record), + MonoType::Fun(fun) => fun.visit(sub).map(MonoType::fun), } } fn free_vars(&self, vars: &mut Vec) { @@ -1116,8 +1116,8 @@ impl Tvar { } impl Substitutable for Collection { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - self.arg.apply_ref(sub).map(|arg| Collection { + fn walk(&self, sub: &dyn Substituter) -> Option { + self.arg.visit(sub).map(|arg| Collection { collection: self.collection, arg, }) @@ -1174,7 +1174,7 @@ pub struct Dictionary { } impl Substitutable for Dictionary { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { apply2(&self.key, &self.val, sub).map(|(key, val)| Dictionary { key, val }) } fn free_vars(&self, vars: &mut Vec) { @@ -1274,7 +1274,7 @@ impl cmp::PartialEq for Record { } impl Substitutable for Record { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match self { Record::Empty => None, Record::Extension { head, tail } => { @@ -1672,8 +1672,8 @@ where K: Clone, V: Substitutable, { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { - self.v.apply_ref(sub).map(|v| Property { + fn walk(&self, sub: &dyn Substituter) -> Option { + self.v.visit(sub).map(|v| Property { k: self.k.clone(), v, }) @@ -1760,11 +1760,11 @@ impl fmt::Display for Function { } impl Substitutable for PolyTypeHashMap { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { merge_collect( &mut (), self.unordered_iter(), - |_, (k, v)| v.apply_ref(sub).map(|v| (k.clone(), v)), + |_, (k, v)| v.visit(sub).map(|v| (k.clone(), v)), |_, (k, v)| (k.clone(), v.clone()), ) } @@ -1776,11 +1776,11 @@ impl Substitutable for PolyTypeHashMap { } impl Substitutable for SemanticMap { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { merge_collect( &mut (), self, - |_, (k, v)| v.apply_ref(sub).map(|v| (k.clone(), v)), + |_, (k, v)| v.visit(sub).map(|v| (k.clone(), v)), |_, (k, v)| (k.clone(), v.clone()), ) } @@ -1792,10 +1792,10 @@ impl Substitutable for SemanticMap Substitutable for Option { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { match self { None => None, - Some(t) => t.apply_ref(sub).map(Some), + Some(t) => t.visit(sub).map(Some), } } fn free_vars(&self, vars: &mut Vec) { @@ -1807,7 +1807,7 @@ impl Substitutable for Option { } impl Substitutable for Function { - fn apply_ref(&self, sub: &dyn Substituter) -> Option { + fn walk(&self, sub: &dyn Substituter) -> Option { let Function { req, opt, From c0e71898663d567cda01a8e5a5014809ba23f700 Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Fri, 18 Feb 2022 14:34:09 +0100 Subject: [PATCH 2/5] refactor: Move substitution specific concerns into their Substituter impls --- libflux/flux-core/src/semantic/sub.rs | 64 ++++++++++++++++++++++++- libflux/flux-core/src/semantic/types.rs | 63 ++++++++---------------- 2 files changed, 82 insertions(+), 45 deletions(-) diff --git a/libflux/flux-core/src/semantic/sub.rs b/libflux/flux-core/src/semantic/sub.rs index b77a03879c..d93bfdb49c 100644 --- a/libflux/flux-core/src/semantic/sub.rs +++ b/libflux/flux-core/src/semantic/sub.rs @@ -1,7 +1,7 @@ //! Substitutions during type inference. use std::{borrow::Cow, cell::RefCell, iter::FusedIterator}; -use crate::semantic::types::{union, Error, MonoType, SubstitutionMap, Tvar, TvarKinds}; +use crate::semantic::types::{union, Error, MonoType, PolyType, SubstitutionMap, Tvar, TvarKinds}; /// A substitution defines a function that takes a monotype as input /// and returns a monotype as output. The output type is interpreted @@ -227,6 +227,32 @@ pub trait Substituter { let _ = var; None } + + // Hack to llow `visit_poly_type_spec` to be implemented both here as a default and in `impl` + // blocks. `self` and `sub` should refer to the same object, but passing `sub` lets us call + // `walk` without needing a `Self: Sized` bound. + #[doc(hidden)] + fn visit_poly_type_spec(&self, sub: &dyn Substituter, typ: &PolyType) -> Option { + typ.walk(sub) + } + + /// Apply a substitution to a type, returning None if there is no substitution for the + /// type. + fn visit_type(&self, typ: &MonoType) -> Option { + match *typ { + MonoType::Var(var) => self.try_apply(var), + MonoType::BoundVar(var) => self.try_apply_bound(var), + _ => None, + } + } +} + +impl<'a> dyn Substituter + 'a { + /// Apply a substitution to a polytype, returning None if there is no substitution for the + /// type. + pub fn visit_poly_type(&self, typ: &PolyType) -> Option { + self.visit_poly_type_spec(self, typ) + } } impl Substituter for F @@ -242,12 +268,48 @@ impl Substituter for SubstitutionMap { fn try_apply(&self, var: Tvar) -> Option { self.get(&var).cloned() } + + fn visit_poly_type_spec(&self, sub: &dyn Substituter, typ: &PolyType) -> Option { + // `vars` defines new distinct variables for `expr` so any substitutions applied on a + // variable named the same must not be applied in `expr` + typ.expr + .visit(&|var| { + if typ.vars.contains(&var) { + None + } else { + sub.try_apply(var) + } + }) + .map(|expr| PolyType { + vars: typ.vars.clone(), + cons: typ.cons.clone(), + expr, + }) + } } impl Substituter for Substitution { fn try_apply(&self, var: Tvar) -> Option { Substitution::try_apply(self, var) } + + fn visit_poly_type_spec(&self, sub: &dyn Substituter, typ: &PolyType) -> Option { + // `vars` defines new distinct variables for `expr` so any substitutions applied on a + // variable named the same must not be applied in `expr` + typ.expr + .visit(&|var| { + if typ.vars.contains(&var) { + None + } else { + sub.try_apply(var) + } + }) + .map(|expr| PolyType { + vars: typ.vars.clone(), + cons: typ.cons.clone(), + expr, + }) + } } pub(crate) struct BindVars<'a> { diff --git a/libflux/flux-core/src/semantic/types.rs b/libflux/flux-core/src/semantic/types.rs index b528d68281..f8aecb8ba8 100644 --- a/libflux/flux-core/src/semantic/types.rs +++ b/libflux/flux-core/src/semantic/types.rs @@ -113,22 +113,18 @@ impl PartialEq for PolyType { } impl Substitutable for PolyType { + fn visit(&self, sub: &dyn Substituter) -> Option { + sub.visit_poly_type(self) + } + fn walk(&self, sub: &dyn Substituter) -> Option { // `vars` defines new distinct variables for `expr` so any substitutions applied on a // variable named the same must not be applied in `expr` - self.expr - .visit(&|var| { - if self.vars.contains(&var) { - None - } else { - sub.try_apply(var) - } - }) - .map(|expr| PolyType { - vars: self.vars.clone(), - cons: self.cons.clone(), - expr, - }) + self.expr.visit(sub).map(|expr| PolyType { + vars: self.vars.clone(), + cons: self.cons.clone(), + expr, + }) } fn free_vars(&self, vars: &mut Vec) { self.expr.free_vars(vars); @@ -697,39 +693,18 @@ impl BuiltinType { } impl Substitutable for MonoType { + fn visit(&self, sub: &dyn Substituter) -> Option { + match sub.visit_type(self) { + Some(typ) => Some(typ.walk(sub).unwrap_or(typ)), + None => self.walk(sub), + } + } + fn walk(&self, sub: &dyn Substituter) -> Option { match self { - MonoType::Error | MonoType::Builtin(_) => None, - MonoType::BoundVar(tvr) => sub.try_apply_bound(*tvr).map(|new| { - // If a variable is the replacement we do not recurse further - // as `instantiate` breaks in cases where it generates a substitution map - // like `{ A => B, B => C }` which would replace `A` with the (fresh) variable `B` - // which would then be replaced again due to `B => C` which is very wrong (`B` != `fresh B`). - // Bit of a hack, but it works. - // - // For other replacements we need to recurse into them as well so that we may fully - // replace any variables that occur in that as well. - if let MonoType::Var(_) = new { - new - } else { - new.apply(sub) - } - }), - MonoType::Var(tvr) => sub.try_apply(*tvr).map(|new| { - // If a variable is the replacement we do not recurse further - // as `instantiate` breaks in cases where it generates a substitution map - // like `{ A => B, B => C }` which would replace `A` with the (fresh) variable `B` - // which would then be replaced again due to `B => C` which is very wrong (`B` != `fresh B`). - // Bit of a hack, but it works. - // - // For other replacements we need to recurse into them as well so that we may fully - // replace any variables that occur in that as well. - if let MonoType::Var(_) = new { - new - } else { - new.apply(sub) - } - }), + MonoType::Error | MonoType::Builtin(_) | MonoType::BoundVar(_) | MonoType::Var(_) => { + None + } MonoType::Collection(app) => app.visit(sub).map(MonoType::app), MonoType::Dict(dict) => dict.visit(sub).map(MonoType::dict), MonoType::Record(obj) => obj.visit(sub).map(MonoType::record), From df98684e69cb51572bfed0c766c2c09425952f7f Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Fri, 18 Feb 2022 15:37:30 +0100 Subject: [PATCH 3/5] refactor: Implement MaxTvar in terms of Substitutable --- libflux/flux-core/src/semantic/types.rs | 117 +++++------------------- 1 file changed, 24 insertions(+), 93 deletions(-) diff --git a/libflux/flux-core/src/semantic/types.rs b/libflux/flux-core/src/semantic/types.rs index f8aecb8ba8..53b971a4db 100644 --- a/libflux/flux-core/src/semantic/types.rs +++ b/libflux/flux-core/src/semantic/types.rs @@ -1,6 +1,7 @@ //! Semantic representations of types. use std::{ + cell::Cell, cmp, collections::{BTreeMap, BTreeSet}, fmt::{self, Write}, @@ -132,24 +133,6 @@ impl Substitutable for PolyType { } } -impl MaxTvar for [Tvar] { - fn max_tvar(&self) -> Option { - self.iter().max().cloned() - } -} - -impl MaxTvar for [Option] { - fn max_tvar(&self) -> Option { - self.iter().max().and_then(|t| *t) - } -} - -impl MaxTvar for PolyType { - fn max_tvar(&self) -> Option { - [self.vars.max_tvar(), self.expr.max_tvar()].max_tvar() - } -} - impl PolyType { pub(crate) fn error() -> Self { PolyType { @@ -729,19 +712,6 @@ impl Substitutable for MonoType { } } -impl MaxTvar for MonoType { - fn max_tvar(&self) -> Option { - match self { - MonoType::Error | MonoType::Builtin(_) => None, - MonoType::BoundVar(tvr) | MonoType::Var(tvr) => tvr.max_tvar(), - MonoType::Collection(app) => app.max_tvar(), - MonoType::Dict(dict) => dict.max_tvar(), - MonoType::Record(obj) => obj.max_tvar(), - MonoType::Fun(fun) => fun.max_tvar(), - } - } -} - impl From for MonoType { fn from(a: Tvar) -> MonoType { MonoType::Var(a) @@ -1029,12 +999,6 @@ impl fmt::Display for Tvar { } } -impl MaxTvar for Tvar { - fn max_tvar(&self) -> Option { - Some(*self) - } -} - impl Tvar { fn unify(self, with: &MonoType, unifier: &mut Unifier<'_>) { match *with { @@ -1102,12 +1066,6 @@ impl Substitutable for Collection { } } -impl MaxTvar for Collection { - fn max_tvar(&self) -> Option { - self.arg.max_tvar() - } -} - impl Collection { // self represents the expected type. fn unify(&self, with: &Self, unifier: &mut Unifier<'_>) { @@ -1158,12 +1116,6 @@ impl Substitutable for Dictionary { } } -impl MaxTvar for Dictionary { - fn max_tvar(&self) -> Option { - [self.key.max_tvar(), self.val.max_tvar()].max_tvar() - } -} - impl Dictionary { fn unify(&self, actual: &Self, unifier: &mut Unifier<'_>) { self.key.unify(&actual.key, unifier); @@ -1268,15 +1220,6 @@ impl Substitutable for Record { } } -impl MaxTvar for Record { - fn max_tvar(&self) -> Option { - match self { - Record::Empty => None, - Record::Extension { head, tail } => [head.max_tvar(), tail.max_tvar()].max_tvar(), - } - } -} - #[allow(clippy::many_single_char_names)] impl Record { /// Creates a new `Record` @@ -1658,12 +1601,6 @@ where } } -impl MaxTvar for Property { - fn max_tvar(&self) -> Option { - self.v.max_tvar() - } -} - /// Represents a function type. /// /// A function type is defined by a set of required arguments, @@ -1804,35 +1741,6 @@ impl Substitutable for Function { } } -impl MaxTvar for SemanticMap { - fn max_tvar(&self) -> Option { - self.iter() - .map(|(_, t)| t.max_tvar()) - .fold(None, |max, tv| if tv > max { tv } else { max }) - } -} - -impl MaxTvar for Option { - fn max_tvar(&self) -> Option { - match self { - None => None, - Some(t) => t.max_tvar(), - } - } -} - -impl MaxTvar for Function { - fn max_tvar(&self) -> Option { - [ - self.req.max_tvar(), - self.opt.max_tvar(), - self.pipe.max_tvar(), - self.retn.max_tvar(), - ] - .max_tvar() - } -} - impl Function { pub(crate) fn try_unify( &self, @@ -2049,6 +1957,29 @@ pub trait MaxTvar { fn max_tvar(&self) -> Option; } +impl MaxTvar for T +where + T: Substitutable, +{ + fn max_tvar(&self) -> Option { + #[derive(Default)] + struct MaxTvars { + max: Cell>, + } + + impl Substituter for MaxTvars { + fn try_apply(&self, var: Tvar) -> Option { + self.max.set(self.max.get().max(Some(var))); + None + } + } + + let max = MaxTvars::default(); + self.visit(&max); + max.max.into_inner() + } +} + #[cfg(test)] mod tests { use std::collections::BTreeMap; From 9f190fb66b7cfcbc2e7ae5f23b44f0cb3850608f Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Fri, 18 Feb 2022 15:47:22 +0100 Subject: [PATCH 4/5] refactor: Implement free_vars in terms of visit --- libflux/flux-core/src/errors.rs | 8 +- libflux/flux-core/src/semantic/bootstrap.rs | 2 +- libflux/flux-core/src/semantic/env.rs | 12 +-- libflux/flux-core/src/semantic/infer.rs | 7 +- libflux/flux-core/src/semantic/nodes.rs | 13 --- libflux/flux-core/src/semantic/sub.rs | 46 +++++++--- libflux/flux-core/src/semantic/types.rs | 95 --------------------- libflux/flux/build.rs | 4 +- 8 files changed, 42 insertions(+), 145 deletions(-) diff --git a/libflux/flux-core/src/errors.rs b/libflux/flux-core/src/errors.rs index 1d42e903d4..c25c73888d 100644 --- a/libflux/flux-core/src/errors.rs +++ b/libflux/flux-core/src/errors.rs @@ -12,10 +12,7 @@ use derive_more::Display; use crate::{ ast, - semantic::{ - sub::{Substitutable, Substituter}, - types::Tvar, - }, + semantic::sub::{Substitutable, Substituter}, }; #[derive(Clone, Debug, Eq, PartialEq, Hash)] @@ -211,9 +208,6 @@ where error, }) } - fn free_vars(&self, vars: &mut Vec) { - self.error.free_vars(vars) - } } pub(crate) trait AsDiagnostic { diff --git a/libflux/flux-core/src/semantic/bootstrap.rs b/libflux/flux-core/src/semantic/bootstrap.rs index abccbc0325..44c7dce096 100644 --- a/libflux/flux-core/src/semantic/bootstrap.rs +++ b/libflux/flux-core/src/semantic/bootstrap.rs @@ -297,7 +297,7 @@ fn add_record_to_map( for field in r.fields() { let new_vars = { let new_vars = CollectBoundVars(RefCell::new(Vec::new())); - head.v.visit(&new_vars); + field.v.visit(&new_vars); new_vars.0.into_inner() }; diff --git a/libflux/flux-core/src/semantic/env.rs b/libflux/flux-core/src/semantic/env.rs index f6a5e54d1f..6ea40f4043 100644 --- a/libflux/flux-core/src/semantic/env.rs +++ b/libflux/flux-core/src/semantic/env.rs @@ -4,7 +4,7 @@ use std::{fmt, mem}; use crate::semantic::{ nodes::Symbol, sub::{apply2, Substitutable, Substituter}, - types::{PolyType, PolyTypeHashMap, PolyTypeMap, Tvar}, + types::{PolyType, PolyTypeHashMap, PolyTypeMap}, PackageExports, }; @@ -59,16 +59,6 @@ impl Substitutable for Environment<'_> { } } } - fn free_vars(&self, vars: &mut Vec) { - match (self.readwrite, &self.parent) { - (false, None) | (false, _) => (), - (true, None) => self.values.free_vars(vars), - (true, Some(env)) => { - env.free_vars(vars); - self.values.free_vars(vars); - } - } - } } // Derive a type environment from a hash map diff --git a/libflux/flux-core/src/semantic/infer.rs b/libflux/flux-core/src/semantic/infer.rs index bc048215c3..116ce196ef 100644 --- a/libflux/flux-core/src/semantic/infer.rs +++ b/libflux/flux-core/src/semantic/infer.rs @@ -116,9 +116,6 @@ impl Substitutable for Error { err, }) } - fn free_vars(&self, vars: &mut Vec) { - self.err.free_vars(vars) - } } // Solve a set of type constraints @@ -222,7 +219,7 @@ pub(crate) fn temporary_generalize( } let generalize = Generalize { - env_free_vars: env.mk_free_vars(), + env_free_vars: env.free_vars(), vars: Default::default(), }; let t = t.apply(&generalize); @@ -282,7 +279,7 @@ pub fn generalize(env: &Environment, sub: &mut Substitution, t: MonoType) -> Pol } let generalize = Generalize { - env_free_vars: env.mk_free_vars(), + env_free_vars: env.free_vars(), sub, vars: Default::default(), }; diff --git a/libflux/flux-core/src/semantic/nodes.rs b/libflux/flux-core/src/semantic/nodes.rs index de089861e9..a1219a5f8d 100644 --- a/libflux/flux-core/src/semantic/nodes.rs +++ b/libflux/flux-core/src/semantic/nodes.rs @@ -93,19 +93,6 @@ impl Substitutable for ErrorKind { | Self::Bug(_) => None, } } - fn free_vars(&self, vars: &mut Vec) { - match self { - Self::Inference(err) => err.free_vars(vars), - Self::UndefinedBuiltin(_) - | Self::UndefinedIdentifier(_) - | Self::InvalidBinOp(_) - | Self::InvalidUnaryOp(_) - | Self::InvalidImportPath(_) - | Self::UnableToVectorize(_) - | Self::InvalidReturn - | Self::Bug(_) => (), - } - } } impl From for ErrorKind { diff --git a/libflux/flux-core/src/semantic/sub.rs b/libflux/flux-core/src/semantic/sub.rs index d93bfdb49c..7988509663 100644 --- a/libflux/flux-core/src/semantic/sub.rs +++ b/libflux/flux-core/src/semantic/sub.rs @@ -194,14 +194,41 @@ pub trait Substitutable { Self: Sized; /// Get all free type variables in a type. - fn mk_free_vars(&self) -> Vec { - let mut vars = Vec::new(); - self.free_vars(&mut vars); - vars - } + fn free_vars(&self) -> Vec + where + Self: Sized, + { + #[derive(Default)] + struct FreeVars { + vars: RefCell>, + } - /// Get all free type variables in a type. - fn free_vars(&self, vars: &mut Vec); + impl Substituter for FreeVars { + fn try_apply(&self, var: Tvar) -> Option { + let mut vars = self.vars.borrow_mut(); + if let Err(i) = vars.binary_search(&var) { + vars.insert(i, var); + } + None + } + + fn visit_poly_type_spec( + &self, + sub: &dyn Substituter, + typ: &PolyType, + ) -> Option { + typ.expr.visit(sub); + self.vars.borrow_mut().retain(|v| !typ.vars.contains(v)); + None + } + } + + let free_vars = FreeVars::default(); + + self.visit(&free_vars); + + free_vars.vars.into_inner() + } } impl Substitutable for Box @@ -211,9 +238,6 @@ where fn walk(&self, sub: &dyn Substituter) -> Option { T::visit(self, sub).map(Box::new) } - fn free_vars(&self, vars: &mut Vec) { - T::free_vars(self, vars) - } } /// Objects from which variable substitutions can be looked up. @@ -228,7 +252,7 @@ pub trait Substituter { None } - // Hack to llow `visit_poly_type_spec` to be implemented both here as a default and in `impl` + // Hack to allow `visit_poly_type_spec` to be implemented both here as a default and in `impl` // blocks. `self` and `sub` should refer to the same object, but passing `sub` lets us call // `walk` without needing a `Self: Sized` bound. #[doc(hidden)] diff --git a/libflux/flux-core/src/semantic/types.rs b/libflux/flux-core/src/semantic/types.rs index 53b971a4db..64378be33f 100644 --- a/libflux/flux-core/src/semantic/types.rs +++ b/libflux/flux-core/src/semantic/types.rs @@ -127,10 +127,6 @@ impl Substitutable for PolyType { expr, }) } - fn free_vars(&self, vars: &mut Vec) { - self.expr.free_vars(vars); - vars.retain(|v| !self.vars.contains(v)); - } } impl PolyType { @@ -310,37 +306,6 @@ impl Substitutable for Error { | Error::MultiplePipeArguments { .. } => None, } } - fn free_vars(&self, vars: &mut Vec) { - match self { - Error::CannotUnify { exp, act } => { - exp.free_vars(vars); - act.free_vars(vars); - } - Error::CannotConstrain { exp: _, act } => act.free_vars(vars), - Error::OccursCheck(tv, ty) => { - ty.free_vars(vars); - if let Err(i) = vars.binary_search(tv) { - vars.insert(i, *tv); - } - } - Error::CannotUnifyLabel { exp, act, .. } => { - exp.free_vars(vars); - act.free_vars(vars); - } - Error::CannotUnifyArgument(_, e) => e.free_vars(vars), - Error::CannotUnifyReturn { exp, act, cause } => { - exp.free_vars(vars); - act.free_vars(vars); - cause.free_vars(vars); - } - Error::MissingLabel(_) - | Error::ExtraLabel(_) - | Error::MissingArgument(_) - | Error::ExtraArgument(_) - | Error::MissingPipeArgument - | Error::MultiplePipeArguments { .. } => (), - } - } } impl Error { @@ -397,9 +362,6 @@ impl Substitutable for Ptr { fn walk(&self, sub: &dyn Substituter) -> Option { T::visit(self, sub).map(Ptr::new) } - fn free_vars(&self, vars: &mut Vec) { - T::free_vars(self, vars) - } } /// An ordered map of string identifiers to monotypes. @@ -694,22 +656,6 @@ impl Substitutable for MonoType { MonoType::Fun(fun) => fun.visit(sub).map(MonoType::fun), } } - fn free_vars(&self, vars: &mut Vec) { - match self { - MonoType::Error | MonoType::Builtin(_) => (), - // By definition a variable that is bound isn't free - MonoType::BoundVar(_) => (), - MonoType::Var(tvr) => { - if let Err(i) = vars.binary_search(tvr) { - vars.insert(i, *tvr); - } - } - MonoType::Collection(app) => app.free_vars(vars), - MonoType::Dict(dict) => dict.free_vars(vars), - MonoType::Record(obj) => obj.free_vars(vars), - MonoType::Fun(fun) => fun.free_vars(vars), - } - } } impl From for MonoType { @@ -1061,9 +1007,6 @@ impl Substitutable for Collection { arg, }) } - fn free_vars(&self, vars: &mut Vec) { - self.arg.free_vars(vars) - } } impl Collection { @@ -1110,10 +1053,6 @@ impl Substitutable for Dictionary { fn walk(&self, sub: &dyn Substituter) -> Option { apply2(&self.key, &self.val, sub).map(|(key, val)| Dictionary { key, val }) } - fn free_vars(&self, vars: &mut Vec) { - self.key.free_vars(vars); - self.val.free_vars(vars); - } } impl Dictionary { @@ -1209,15 +1148,6 @@ impl Substitutable for Record { } } } - fn free_vars(&self, vars: &mut Vec) { - match self { - Record::Empty => (), - Record::Extension { head, tail } => { - tail.free_vars(vars); - head.v.free_vars(vars); - } - } - } } #[allow(clippy::many_single_char_names)] @@ -1596,9 +1526,6 @@ where v, }) } - fn free_vars(&self, vars: &mut Vec) { - self.v.free_vars(vars) - } } /// Represents a function type. @@ -1680,11 +1607,6 @@ impl Substitutable for PolyTypeHashMap { |_, (k, v)| (k.clone(), v.clone()), ) } - fn free_vars(&self, vars: &mut Vec) { - for t in self.unordered_values() { - t.free_vars(vars); - } - } } impl Substitutable for SemanticMap { @@ -1696,11 +1618,6 @@ impl Substitutable for SemanticMap) { - for t in self.values() { - t.free_vars(vars); - } - } } impl Substitutable for Option { @@ -1710,12 +1627,6 @@ impl Substitutable for Option { Some(t) => t.visit(sub).map(Some), } } - fn free_vars(&self, vars: &mut Vec) { - match self { - Some(t) => t.free_vars(vars), - None => (), - } - } } impl Substitutable for Function { @@ -1733,12 +1644,6 @@ impl Substitutable for Function { retn, }) } - fn free_vars(&self, vars: &mut Vec) { - self.req.free_vars(vars); - self.opt.free_vars(vars); - self.pipe.free_vars(vars); - self.retn.free_vars(vars); - } } impl Function { diff --git a/libflux/flux/build.rs b/libflux/flux/build.rs index 560db92707..deed7e5e49 100644 --- a/libflux/flux/build.rs +++ b/libflux/flux/build.rs @@ -61,13 +61,13 @@ fn main() -> Result<()> { // Validate there aren't any free type variables in the environment for (name, ty) in prelude.iter() { - if !ty.mk_free_vars().is_empty() { + if !ty.free_vars().is_empty() { bail!("found free variables in type of {}: {}", name, ty); } } for (name, package) in &imports { let ty = package.typ(); - if !ty.mk_free_vars().is_empty() { + if !ty.free_vars().is_empty() { bail!("found free variables in type of package {}: {}", name, ty); } } From 819b200a360901e84eb80a0633588084b340dad8 Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Tue, 29 Mar 2022 16:09:59 +0200 Subject: [PATCH 5/5] chore: make generate --- libflux/go/libflux/buildinfo.gen.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/libflux/go/libflux/buildinfo.gen.go b/libflux/go/libflux/buildinfo.gen.go index f557c0beaa..2e384f03e7 100644 --- a/libflux/go/libflux/buildinfo.gen.go +++ b/libflux/go/libflux/buildinfo.gen.go @@ -26,7 +26,7 @@ var sourceHashes = map[string]string{ "libflux/flux-core/src/bin/fluxdoc.rs": "bad4b12bcf4a8bc1a94cb37cda004bf7fb593abf3f0c6c3a2af6fabc60337c5d", "libflux/flux-core/src/doc/example.rs": "6414756b3c74df1b58fdb739592e74ded3f89d85d647809333f72a3f6aad146f", "libflux/flux-core/src/doc/mod.rs": "3a42d710039b9fa7c51116291b58040ffa84969e3036ee538a2e5f55666dae5e", - "libflux/flux-core/src/errors.rs": "7eb977a67a5f26801ab4622f54722bac1e11945690891a7f98c11337e6b86fde", + "libflux/flux-core/src/errors.rs": "5ee16ec2fd281f7c115ba0b7bcf3380749f64b7a282eb0fab7e6afe9858f3d4d", "libflux/flux-core/src/formatter/mod.rs": "3d26f82f929e4a1f1c188b96b2a8a8c5e2a2fb8f0338fcfebaceead47cd2c4ef", "libflux/flux-core/src/lib.rs": "153f4b1d98494f5cab6e46cb2492c217e3dfec1e14368b251a8b48e9c2838fff", "libflux/flux-core/src/map.rs": "342c1cc111d343f01b97f38be10a9f1097bdd57cdc56f55e92fd3ed5028e6973", @@ -38,10 +38,10 @@ var sourceHashes = map[string]string{ "libflux/flux-core/src/scanner/token.rs": "5090c2ac4b341566a85f7d9ed9b48746dd2084ec2f3effdb4a7dc16cf34d44b9", "libflux/flux-core/src/scanner/unicode.rl": "f923f3b385ddfa65c74427b11971785fc25ea806ca03d547045de808e16ef9a1", "libflux/flux-core/src/scanner/unicode.rl.COPYING": "6cf2d5d26d52772ded8a5f0813f49f83dfa76006c5f398713be3854fe7bc4c7e", - "libflux/flux-core/src/semantic/bootstrap.rs": "2cbf817169403b662295c717f2a3b6721420b134fb708f33949e023cb1b69323", + "libflux/flux-core/src/semantic/bootstrap.rs": "0a182c6512b815caaeb99e07266563df8eebb58bb43eb75ad53d0ee28ff268d8", "libflux/flux-core/src/semantic/check.rs": "d0228a0a8176a5360d88cfe48acb1ffd036817b6aaadfadb94af446492603305", "libflux/flux-core/src/semantic/convert.rs": "e6d4d01d887434b73dc2663eeb88294b67a64d15ffed7fb61af0e401fb5670ea", - "libflux/flux-core/src/semantic/env.rs": "802257307bbe18137cd495ba450838dbca1ac9ae34da930b1822374bf2b33739", + "libflux/flux-core/src/semantic/env.rs": "db424704eece030a76dee968c3d1959e94d21b7f7400c35f8e233cb3089218a4", "libflux/flux-core/src/semantic/flatbuffers/mod.rs": "89007a9f09e42f3a096d1a028275734ec2b82906c308e37dfd783c99f2a43e81", "libflux/flux-core/src/semantic/flatbuffers/semantic_generated.rs": "0f54e652b45b1515c71160a555200a062f9c3c5a681a14f7a0ac36cc19dd4e6f", "libflux/flux-core/src/semantic/flatbuffers/types.rs": "3b03937ab3b84746e0a242be659409827eba63b687a72d145c6455067468c374", @@ -49,18 +49,18 @@ var sourceHashes = map[string]string{ "libflux/flux-core/src/semantic/fresh.rs": "f0baf3e08c4fee667921dc465a7dab73c840deddd3ac113b0f3f6b4fc384b807", "libflux/flux-core/src/semantic/fs.rs": "f7f609bc8149769d99b737150e184a2d54029c0b768365dbcf08ff193b0e1f6f", "libflux/flux-core/src/semantic/import.rs": "73adc35c735c317af0fa7625e77a58f6824667d8e59f35c7d05687bd02d79a84", - "libflux/flux-core/src/semantic/infer.rs": "a9cf793d640ece528f702cc850c031ddf7c003f0b24591d77165a66e1a164d69", + "libflux/flux-core/src/semantic/infer.rs": "4f51f0d551ea2ef7e26c3447e18f36de3ecdcc4bf0dffa7da724178bc00c7e5c", "libflux/flux-core/src/semantic/mod.rs": "7633dfe325c9db6fdc1e835c70ac1d00f5d2aaa71a66b19523e4044833f39c99", - "libflux/flux-core/src/semantic/nodes.rs": "4b8ff2abdd637df15b913b49ca4eea2e1af30fe616bce29f2429b5a310e594d6", - "libflux/flux-core/src/semantic/sub.rs": "05cf04fa0dd1bb04a85c249a5e65dcf5635fe3e49ef48e4620378a31d26d798f", - "libflux/flux-core/src/semantic/types.rs": "65feac91b580d9ab42c9f6539f647c479da2da3d3875425fd49a831f893805a7", + "libflux/flux-core/src/semantic/nodes.rs": "6af781a31eb7af76f5cbe8ea7164a3e6c34e680c62aecc18c8341402b059c5dd", + "libflux/flux-core/src/semantic/sub.rs": "362460a981f4dbcf73e1fd740301d5dd07b9f476604ca9c841259da2b03f8606", + "libflux/flux-core/src/semantic/types.rs": "e1072fe9acf33a957ed50952d25c5b532f75dcda739e3571fc0f020cfc119110", "libflux/flux-core/src/semantic/walk/_walk.rs": "198e6c546f73f78b1de4b963084033a06a6d71e72b0294b16c5e4de874f97a38", "libflux/flux-core/src/semantic/walk/mod.rs": "8f6bcfc7d309ad821016d6622b49cfde3de5de4d7d0315e55c5cd7cfed279eb8", "libflux/flux-core/src/semantic/walk/test_utils.rs": "b980587707038c420d16b99f99587e69b71f02c32277db37c7e4a094d32f2b38", "libflux/flux-core/src/semantic/walk/walk_mut.rs": "4ef980dd4e64a4522d1d79d7bb72f5e3743c3e092387d46c8295d9364b5e2c12", "libflux/flux/Cargo.toml": "6152dbad3f3e73120a54088b207d37ad1bd61cdf7abca6be27364dc7b809594b", "libflux/flux/FLUXDOC.md": "92e6dd8043bd87b4924e09aa28fb5346630aee1214de28ea2c8fc0687cad0785", - "libflux/flux/build.rs": "c3257f0014596c8f41396a115ffef11413aebd52006ec5fd5b7fc81cbe82ebd0", + "libflux/flux/build.rs": "eea9cfad417042bdd0908c5f7bcf2f004ee70031d973292e2cf54fab526a1807", "libflux/flux/src/lib.rs": "de4e758c4e5a639e5a59d591074ef4580e2a0d203fd28469bde32a3582bf32be", "libflux/flux/templates/base.html": "a818747b9621828bb96b94291c60922db54052bbe35d5e354f8e589d2a4ebd02", "libflux/flux/templates/home.html": "f9927514dd42ca7271b4817ad1ca33ec79c03a77a783581b4dcafabd246ebf3f",