From 830f0ac35f724e6a7f35d77a367efd03d76480d9 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 1 Jan 2025 17:27:45 -0600 Subject: [PATCH 1/5] Bugfix association of inferred-tag-extension variables These are inferred vars, not rigids. --- crates/compiler/constrain/src/expr.rs | 44 +++++++++++++++++++-------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/crates/compiler/constrain/src/expr.rs b/crates/compiler/constrain/src/expr.rs index a90566161d..d27e47d56f 100644 --- a/crates/compiler/constrain/src/expr.rs +++ b/crates/compiler/constrain/src/expr.rs @@ -1959,6 +1959,7 @@ fn constrain_function_def( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables: _, } = instantiate_rigids_simple( types, &annotation.signature, @@ -2313,6 +2314,7 @@ fn constrain_destructure_def( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables: _, } = instantiate_rigids( types, constraints, @@ -2420,6 +2422,7 @@ fn constrain_value_def( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables: _, } = instantiate_rigids_simple( types, &annotation.signature, @@ -2913,6 +2916,7 @@ fn constrain_typed_def( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables: _, } = instantiate_rigids( types, constraints, @@ -3752,8 +3756,13 @@ fn constrain_closure_size( pub struct InstantiateRigids { pub signature: Index, - pub new_rigid_variables: Vec, - pub new_infer_variables: Vec, + pub new_rigid_variables: Vec>, + pub new_infer_variables: Vec>, + /// Whether the annotation has explicit inference variables `_`. + /// Annotations with inference variables are handled specially during typechecking of mutually recursive defs, + /// because they are not guaranteed to be generalized (XREF(rec-def-strategy)). + /// Ideally, this special-casing would be removed in the future. + pub has_explicit_inference_variables: bool, } #[derive(PartialEq, Eq)] @@ -3802,19 +3811,23 @@ fn instantiate_rigids( new_rigid_variables.extend(introduced_vars.lambda_sets.iter().copied()); // ext-infer vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); + new_infer_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); - new_infer_variables.extend(introduced_vars.inferred.iter().map(|v| v.value)); + let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); + new_infer_variables.extend(introduced_vars.inferred.iter().copied()); // Instantiate rigid variables if !rigid_substitution.is_empty() { annotation.substitute_variables(&rigid_substitution); } - types.from_old_type(&annotation) + ( + types.from_old_type(&annotation), + has_explicit_inference_variables, + ) }; - let signature = generate_fresh_ann(types); + let (signature, has_explicit_inference_variables) = generate_fresh_ann(types); { // If this is a recursive def, we must also generate a fresh annotation to be used as the // type annotation that will be used in the first def headers introduced during the solving @@ -3825,7 +3838,7 @@ fn instantiate_rigids( // So, we generate a fresh annotation here, and return a separate fresh annotation below; // the latter annotation is the one used to construct the finalized type. let annotation_index = if is_recursive_def == IsRecursiveDef::Yes { - generate_fresh_ann(types) + generate_fresh_ann(types).0 } else { signature }; @@ -3848,6 +3861,7 @@ fn instantiate_rigids( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables, } } @@ -3883,11 +3897,11 @@ fn instantiate_rigids_simple( // lambda set vars are always freshly introduced in this annotation new_rigid_variables.extend(introduced_vars.lambda_sets.iter().copied()); - // ext-infer vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); + let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); + let mut new_infer_variables: Vec> = introduced_vars.inferred.clone(); - let new_infer_variables: Vec = - introduced_vars.inferred.iter().map(|v| v.value).collect(); + // ext-infer vars are always freshly introduced in this annotation + new_infer_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); // Instantiate rigid variables if !rigid_substitution.is_empty() { @@ -3898,6 +3912,7 @@ fn instantiate_rigids_simple( signature: types.from_old_type(&annotation), new_rigid_variables, new_infer_variables, + has_explicit_inference_variables, } } @@ -3981,6 +3996,7 @@ fn constraint_recursive_function( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables: _, } = instantiate_rigids_simple( types, &annotation.signature, @@ -4250,6 +4266,7 @@ pub fn rec_defs_help_simple( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables, } = instantiate_rigids_simple( types, &annotation.signature, @@ -4260,7 +4277,7 @@ pub fn rec_defs_help_simple( let loc_pattern = Loc::at(loc_symbol.region, Pattern::Identifier(loc_symbol.value)); - let is_hybrid = !new_infer_variables.is_empty(); + let is_hybrid = has_explicit_inference_variables; hybrid_and_flex_info.vars.extend(new_infer_variables); @@ -4537,6 +4554,7 @@ fn rec_defs_help( signature, new_rigid_variables, new_infer_variables, + has_explicit_inference_variables, } = instantiate_rigids( types, constraints, @@ -4548,7 +4566,7 @@ fn rec_defs_help( IsRecursiveDef::Yes, ); - let is_hybrid = !new_infer_variables.is_empty(); + let is_hybrid = has_explicit_inference_variables; hybrid_and_flex_info.vars.extend(&new_infer_variables); From bd2dd66c96cfba5fb4ce26349b7c81547b968d14 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 1 Jan 2025 18:15:30 -0600 Subject: [PATCH 2/5] Store rigid vars with location during constraining --- crates/compiler/can/src/constraint.rs | 35 +++++++-- crates/compiler/constrain/src/expr.rs | 99 ++++++++++++++----------- crates/compiler/constrain/src/module.rs | 10 ++- crates/compiler/solve/src/solve.rs | 18 ++--- 4 files changed, 101 insertions(+), 61 deletions(-) diff --git a/crates/compiler/can/src/constraint.rs b/crates/compiler/can/src/constraint.rs index c217e4adc1..f03a82afe0 100644 --- a/crates/compiler/can/src/constraint.rs +++ b/crates/compiler/can/src/constraint.rs @@ -18,6 +18,7 @@ pub struct Constraints { pub constraints: Vec, pub type_slices: Vec, pub variables: Vec, + loc_variables: Vec>, pub loc_symbols: Vec<(Symbol, Region)>, pub let_constraints: Vec, pub categories: Vec, @@ -42,6 +43,7 @@ impl std::fmt::Debug for Constraints { .field("types", &"") .field("type_slices", &self.type_slices) .field("variables", &self.variables) + .field("loc_variables", &self.loc_variables) .field("loc_symbols", &self.loc_symbols) .field("let_constraints", &self.let_constraints) .field("categories", &self.categories) @@ -75,6 +77,7 @@ impl Constraints { let constraints = Vec::new(); let type_slices = Vec::with_capacity(16); let variables = Vec::new(); + let loc_variables = Vec::new(); let loc_symbols = Vec::new(); let let_constraints = Vec::new(); let mut categories = Vec::with_capacity(16); @@ -126,6 +129,7 @@ impl Constraints { constraints, type_slices, variables, + loc_variables, loc_symbols, let_constraints, categories, @@ -377,6 +381,17 @@ impl Constraints { Slice::new(start as _, length as _) } + pub fn loc_variable_slice(&mut self, it: I) -> Slice> + where + I: IntoIterator>, + { + let start = self.loc_variables.len(); + self.loc_variables.extend(it); + let length = self.loc_variables.len() - start; + + Slice::new(start as _, length as _) + } + fn def_types_slice(&mut self, it: I) -> DefTypes where I: IntoIterator)>, @@ -473,8 +488,8 @@ impl Constraints { generalizable: Generalizable, ) -> Constraint where - I1: IntoIterator, - I2: IntoIterator, + I1: IntoIterator>, + I2: IntoIterator>, I3: IntoIterator)>, I3::IntoIter: ExactSizeIterator, { @@ -485,8 +500,8 @@ impl Constraints { self.constraints.push(ret_constraint); let let_constraint = LetConstraint { - rigid_vars: self.variable_slice(rigid_vars), - flex_vars: self.variable_slice(flex_vars), + rigid_vars: self.loc_variable_slice(rigid_vars), + flex_vars: self.variable_slice(flex_vars.into_iter().map(|v| v.value)), def_types: self.def_types_slice(def_types), defs_and_ret_constraint, generalizable, @@ -534,7 +549,7 @@ impl Constraints { self.constraints.push(module_constraint); let let_contraint = LetConstraint { - rigid_vars: self.variable_slice(rigid_vars), + rigid_vars: self.loc_variable_slice(rigid_vars.into_iter().map(Loc::at_zero)), flex_vars: self.variable_slice(flex_vars), def_types: self.def_types_slice(def_types), defs_and_ret_constraint, @@ -809,6 +824,14 @@ impl std::ops::Index for Constraints { } } +impl std::ops::Index>> for Constraints { + type Output = [Loc]; + + fn index(&self, slice: Slice>) -> &Self::Output { + &self.loc_variables[slice.indices()] + } +} + #[derive(Clone, Copy, Debug)] pub struct Eq( pub TypeOrVar, @@ -914,7 +937,7 @@ pub struct Generalizable(pub bool); #[derive(Debug, Clone)] pub struct LetConstraint { - pub rigid_vars: Slice, + pub rigid_vars: Slice>, pub flex_vars: Slice, pub def_types: DefTypes, pub defs_and_ret_constraint: Index<(Constraint, Constraint)>, diff --git a/crates/compiler/constrain/src/expr.rs b/crates/compiler/constrain/src/expr.rs index d27e47d56f..0e7254d950 100644 --- a/crates/compiler/constrain/src/expr.rs +++ b/crates/compiler/constrain/src/expr.rs @@ -37,8 +37,8 @@ use soa::{Index, Slice}; /// This is for constraining Defs #[derive(Default, Debug)] -pub struct Info { - pub vars: Vec, +struct Info { + pub vars: Vec>, pub constraints: Vec, pub def_types: VecMap>, } @@ -224,7 +224,7 @@ fn constrain_untyped_closure( let cons = [ constraints.let_constraint( [], - pattern_state.vars, + pattern_state.vars.into_iter().map(Loc::at_zero), pattern_state.headers, pattern_state_constraints, returns_constraint, @@ -1273,7 +1273,7 @@ pub fn constrain_expr( let body_constraints = constraints.and_constraint(body_cons); let when_body_con = constraints.let_constraint( [], - pattern_vars, + pattern_vars.into_iter().map(Loc::at_zero), pattern_headers, pattern_constraints, body_constraints, @@ -2202,7 +2202,7 @@ fn constrain_function_def( ), constraints.let_constraint( [], - argument_pattern_state.vars, + argument_pattern_state.vars.into_iter().map(Loc::at_zero), argument_pattern_state.headers, defs_constraint, returns_constraint, @@ -3065,7 +3065,7 @@ fn constrain_typed_def( constraints.store(fx_type_index, fx_var, std::file!(), std::line!()), constraints.let_constraint( [], - argument_pattern_state.vars, + argument_pattern_state.vars.into_iter().map(Loc::at_zero), argument_pattern_state.headers, defs_constraint, returns_constraint, @@ -3624,14 +3624,15 @@ fn constrain_stmt_def( /// Recursive defs should always use `constrain_recursive_defs`. pub(crate) fn constrain_def_make_constraint( constraints: &mut Constraints, - annotation_rigid_variables: impl Iterator, - annotation_infer_variables: impl Iterator, + annotation_rigid_variables: impl Iterator>, + annotation_infer_variables: impl Iterator>, def_expr_con: Constraint, after_def_con: Constraint, def_pattern_state: PatternState, generalizable: Generalizable, ) -> Constraint { - let all_flex_variables = (def_pattern_state.vars.into_iter()).chain(annotation_infer_variables); + let all_flex_variables = + (def_pattern_state.vars.into_iter().map(Loc::at_zero)).chain(annotation_infer_variables); let pattern_constraints = constraints.and_constraint(def_pattern_state.constraints); let def_pattern_and_body_con = constraints.and_constraint([pattern_constraints, def_expr_con]); @@ -3648,8 +3649,8 @@ pub(crate) fn constrain_def_make_constraint( fn constrain_value_def_make_constraint( constraints: &mut Constraints, - new_rigid_variables: Vec, - new_infer_variables: Vec, + new_rigid_variables: Vec>, + new_infer_variables: Vec>, expr_con: Constraint, body_con: Constraint, symbol: Loc, @@ -3670,7 +3671,7 @@ fn constrain_value_def_make_constraint( constraints.let_constraint( new_rigid_variables, - [expr_var], + [Loc::at(symbol.region, expr_var)], headers, def_con, body_con, @@ -3680,8 +3681,8 @@ fn constrain_value_def_make_constraint( fn constrain_function_def_make_constraint( constraints: &mut Constraints, - new_rigid_variables: Vec, - new_infer_variables: Vec, + new_rigid_variables: Vec>, + new_infer_variables: Vec>, expr_con: Constraint, body_con: Constraint, def_pattern_state: PatternState, @@ -3699,7 +3700,7 @@ fn constrain_function_def_make_constraint( constraints.let_constraint( new_rigid_variables, - def_pattern_state.vars, + def_pattern_state.vars.into_iter().map(Loc::at_zero), def_pattern_state.headers, def_con, body_con, @@ -3781,8 +3782,8 @@ fn instantiate_rigids( headers: &mut VecMap>, is_recursive_def: IsRecursiveDef, ) -> InstantiateRigids { - let mut new_rigid_variables = vec![]; - let mut new_infer_variables = vec![]; + let mut new_rigid_variables: Vec> = vec![]; + let mut new_infer_variables: Vec> = vec![]; let mut generate_fresh_ann = |types: &mut Types| { let mut annotation = annotation.clone(); @@ -3799,19 +3800,24 @@ fn instantiate_rigids( Vacant(vacant) => { // It's possible to use this rigid in nested defs vacant.insert(named.variable()); - new_rigid_variables.push(named.variable()); + new_rigid_variables.push(Loc::at(named.first_seen(), named.variable())); } } } // wildcards are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.wildcards.iter().map(|v| v.value)); + new_rigid_variables.extend(introduced_vars.wildcards.iter().copied()); // lambda set vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.lambda_sets.iter().copied()); + new_rigid_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); // ext-infer vars are always freshly introduced in this annotation - new_infer_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); + new_infer_variables.extend( + introduced_vars + .infer_ext_in_output + .iter() + .map(|&v| Loc::at_zero(v)), + ); let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); new_infer_variables.extend(introduced_vars.inferred.iter().copied()); @@ -3872,7 +3878,7 @@ fn instantiate_rigids_simple( ftv: &mut MutMap, // rigids defined before the current annotation ) -> InstantiateRigids { let mut annotation = annotation.clone(); - let mut new_rigid_variables: Vec = Vec::new(); + let mut new_rigid_variables: Vec> = Vec::new(); let mut rigid_substitution: MutMap = MutMap::default(); for named in introduced_vars.iter_named() { @@ -3886,22 +3892,27 @@ fn instantiate_rigids_simple( Vacant(vacant) => { // It's possible to use this rigid in nested defs vacant.insert(named.variable()); - new_rigid_variables.push(named.variable()); + new_rigid_variables.push(Loc::at(named.first_seen(), named.variable())); } } } // wildcards are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.wildcards.iter().map(|v| v.value)); + new_rigid_variables.extend(introduced_vars.wildcards.iter().copied()); // lambda set vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.lambda_sets.iter().copied()); + new_rigid_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); let mut new_infer_variables: Vec> = introduced_vars.inferred.clone(); // ext-infer vars are always freshly introduced in this annotation - new_infer_variables.extend(introduced_vars.infer_ext_in_output.iter().copied()); + new_infer_variables.extend( + introduced_vars + .infer_ext_in_output + .iter() + .map(|&v| Loc::at_zero(v)), + ); // Instantiate rigid variables if !rigid_substitution.is_empty() { @@ -3979,7 +3990,7 @@ fn constraint_recursive_function( let expr_con = attach_resolution_constraints(constraints, env, expr_con); let def_con = expr_con; - flex_info.vars.push(expr_var); + flex_info.vars.push(Loc::at_zero(expr_var)); flex_info.constraints.push(def_con); flex_info.def_types.insert( loc_symbol.value, @@ -4125,7 +4136,7 @@ fn constraint_recursive_function( constraints.store(fx_type_index, fx_var, std::file!(), std::line!()), constraints.let_constraint( [], - argument_pattern_state.vars, + argument_pattern_state.vars.into_iter().map(Loc::at_zero), argument_pattern_state.headers, state_constraints, returns_constraint, @@ -4157,10 +4168,8 @@ fn constraint_recursive_function( // aligns with what the (mutually-)recursive signature says, so finish // generalization of the function. let rigids = new_rigid_variables; - let flex = def_pattern_state - .vars - .into_iter() - .chain(new_infer_variables); + let flex_pattern_vars = def_pattern_state.vars.into_iter().map(Loc::at_zero); + let flex = flex_pattern_vars.chain(new_infer_variables); constraints.let_constraint( rigids, @@ -4233,7 +4242,8 @@ pub fn rec_defs_help_simple( let opt_annotation = &declarations.annotations[index]; let loc_expr = &declarations.expressions[index]; - expr_regions.push(loc_expr.region); + let expr_region = loc_expr.region; + expr_regions.push(expr_region); match opt_annotation { None => { @@ -4251,7 +4261,9 @@ pub fn rec_defs_help_simple( let def_con = expr_con; - hybrid_and_flex_info.vars.push(expr_var); + hybrid_and_flex_info + .vars + .push(Loc::at(expr_region, expr_var)); hybrid_and_flex_info.constraints.push(def_con); hybrid_and_flex_info .def_types @@ -4329,7 +4341,7 @@ pub fn rec_defs_help_simple( rigid_info.constraints.push(constraints.let_constraint( new_rigid_variables, - [expr_var], + [Loc::at(expr_region, expr_var)], [], // no headers introduced (at this level) def_con, Constraint::True, @@ -4539,7 +4551,9 @@ fn rec_defs_help( let def_con = expr_con; - hybrid_and_flex_info.vars.extend(def_pattern_state.vars); + hybrid_and_flex_info + .vars + .extend(def_pattern_state.vars.into_iter().map(Loc::at_zero)); hybrid_and_flex_info.constraints.push(def_con); hybrid_and_flex_info .def_types @@ -4688,7 +4702,7 @@ fn rec_defs_help( constraints.store(fx_type_index, fx_var, std::file!(), std::line!()), constraints.let_constraint( [], - argument_pattern_state.vars, + argument_pattern_state.vars.into_iter().map(Loc::at_zero), argument_pattern_state.headers, state_constraints, returns_constraint, @@ -4724,7 +4738,6 @@ fn rec_defs_help( if is_hybrid { // TODO this is not quite right, types that are purely rigid should not // be stored as hybrid! - // However it might not be possible to fix this before types SoA lands. hybrid_and_flex_info.vars.extend(&new_rigid_variables); hybrid_and_flex_info.constraints.push(def_con); hybrid_and_flex_info @@ -4734,10 +4747,9 @@ fn rec_defs_help( rigid_info.vars.extend(&new_rigid_variables); let rigids = new_rigid_variables; - let flex = def_pattern_state - .vars - .into_iter() - .chain(new_infer_variables); + let flex_pattern_vars = + def_pattern_state.vars.into_iter().map(Loc::at_zero); + let flex = flex_pattern_vars.chain(new_infer_variables); rigid_info.constraints.push(constraints.let_constraint( rigids, @@ -4784,10 +4796,11 @@ fn rec_defs_help( .extend(def_pattern_state.headers); } else { rigid_info.vars.extend(&new_rigid_variables); + let flex_vars = def_pattern_state.vars.into_iter().map(Loc::at_zero); rigid_info.constraints.push(constraints.let_constraint( new_rigid_variables, - def_pattern_state.vars, + flex_vars, [], // no headers introduced (at this level) def_con, Constraint::True, diff --git a/crates/compiler/constrain/src/module.rs b/crates/compiler/constrain/src/module.rs index 09e29b5211..470039d4c8 100644 --- a/crates/compiler/constrain/src/module.rs +++ b/crates/compiler/constrain/src/module.rs @@ -83,7 +83,7 @@ fn constrain_params( let cons = constraints.let_constraint( [], - state.vars, + state.vars.into_iter().map(Loc::at_zero), state.headers, pattern_constraints, constraint, @@ -199,8 +199,12 @@ pub fn frontload_ability_constraints( def_pattern_state.vars.push(signature_var); - let rigid_variables = vars.rigid_vars.iter().chain(vars.able_vars.iter()).copied(); - let infer_variables = vars.flex_vars.iter().copied(); + let rigid_variables = (vars.rigid_vars.iter()) + .chain(vars.able_vars.iter()) + .copied() + .map(Loc::at_zero); + + let infer_variables = vars.flex_vars.iter().copied().map(Loc::at_zero); let signature_expectation = constraints.push_expected_type(Expected::NoExpectation(signature_index)); diff --git a/crates/compiler/solve/src/solve.rs b/crates/compiler/solve/src/solve.rs index 34078a6fec..a0e7a0f9ca 100644 --- a/crates/compiler/solve/src/solve.rs +++ b/crates/compiler/solve/src/solve.rs @@ -419,7 +419,7 @@ fn solve( // check that things went well dbg_do!(ROC_VERIFY_RIGID_LET_GENERALIZED, { - let rigid_vars = &env.constraints.variables[let_con.rigid_vars.indices()]; + let rigid_vars = &env.constraints[let_con.rigid_vars]; // NOTE the `subs.redundant` check does not come from elm. // It's unclear whether this is a bug with our implementation @@ -427,9 +427,9 @@ fn solve( // or that it just never came up in elm. let mut it = rigid_vars .iter() - .filter(|&var| { - !env.subs.redundant(*var) - && env.subs.get_rank(*var) != Rank::GENERALIZED + .filter(|loc_var| { + let var = loc_var.value; + !env.subs.redundant(var) && env.subs.get_rank(var) != Rank::GENERALIZED }) .peekable(); @@ -1007,11 +1007,11 @@ fn solve( let ret_constraint = &env.constraints.constraints[offset + 1]; let flex_vars = &env.constraints.variables[let_con.flex_vars.indices()]; - let rigid_vars = &env.constraints.variables[let_con.rigid_vars.indices()]; + let rigid_vars = &env.constraints[let_con.rigid_vars]; let pool_variables = &env.constraints.variables[pool_slice.indices()]; - if matches!(&ret_constraint, True) && let_con.rigid_vars.is_empty() { + if matches!(&ret_constraint, True) && rigid_vars.is_empty() { debug_assert!(pool_variables.is_empty()); env.introduce(rank, flex_vars); @@ -1025,7 +1025,7 @@ fn solve( }); state - } else if let_con.rigid_vars.is_empty() && let_con.flex_vars.is_empty() { + } else if rigid_vars.is_empty() && let_con.flex_vars.is_empty() { // items are popped from the stack in reverse order. That means that we'll // first solve then defs_constraint, and then (eventually) the ret_constraint. // @@ -1068,11 +1068,11 @@ fn solve( // Introduce the variables of this binding, and extend the pool at our binding // rank. - for &var in rigid_vars.iter().chain(flex_vars.iter()) { + for &var in rigid_vars.iter().map(|v| &v.value).chain(flex_vars.iter()) { env.subs.set_rank(var, binding_rank); } pool.reserve(rigid_vars.len() + flex_vars.len()); - pool.extend(rigid_vars.iter()); + pool.extend(rigid_vars.iter().map(|v| &v.value)); pool.extend(flex_vars.iter()); // Now, run our binding constraint, generalize, then solve the rest of the From 54cc5e4c297dc3da0a18d1b282ec8fe0fd8644c7 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 1 Jan 2025 21:46:11 -0600 Subject: [PATCH 3/5] Unify let-introduction in a single path Remove branches on determining how let-bindings are introduced to the scope. This is maybe a little more inefficient, but I think it is a huge simplification. One additional change this required was changing how fx suffixes are checked. The current implementation would add additional constraints for patterns in let bindings conditionally. However, this is unnecessary. I believe it is sufficient to check the fx suffix by running the checks on all introduced symbols after the type is well known (i.e. the body is checked). --- crates/compiler/can/src/constraint.rs | 17 -- crates/compiler/constrain/src/pattern.rs | 41 +-- crates/compiler/load/tests/test_reporting.rs | 4 +- crates/compiler/solve/src/solve.rs | 257 +++++++------------ 4 files changed, 99 insertions(+), 220 deletions(-) diff --git a/crates/compiler/can/src/constraint.rs b/crates/compiler/can/src/constraint.rs index f03a82afe0..569ea9a07b 100644 --- a/crates/compiler/can/src/constraint.rs +++ b/crates/compiler/can/src/constraint.rs @@ -619,23 +619,6 @@ impl Constraints { Constraint::FxCall(constraint_index) } - pub fn fx_pattern_suffix( - &mut self, - symbol: Symbol, - type_index: TypeOrVar, - region: Region, - ) -> Constraint { - let constraint = FxSuffixConstraint { - kind: FxSuffixKind::Pattern(symbol), - type_index, - region, - }; - - let constraint_index = index_push_new(&mut self.fx_suffix_constraints, constraint); - - Constraint::FxSuffix(constraint_index) - } - pub fn fx_record_field_unsuffixed(&mut self, variable: Variable, region: Region) -> Constraint { let type_index = Self::push_type_variable(variable); let constraint = FxSuffixConstraint { diff --git a/crates/compiler/constrain/src/pattern.rs b/crates/compiler/constrain/src/pattern.rs index d896ca9bf8..8c2d06c08c 100644 --- a/crates/compiler/constrain/src/pattern.rs +++ b/crates/compiler/constrain/src/pattern.rs @@ -6,7 +6,7 @@ use roc_can::pattern::Pattern::{self, *}; use roc_can::pattern::{DestructType, ListPatterns, RecordDestruct, TupleDestruct}; use roc_collections::all::{HumanIndex, SendMap}; use roc_collections::VecMap; -use roc_module::ident::{IdentSuffix, Lowercase}; +use roc_module::ident::Lowercase; use roc_module::symbol::Symbol; use roc_region::all::{Loc, Region}; use roc_types::subs::Variable; @@ -249,16 +249,7 @@ pub fn constrain_pattern( expected: PExpectedTypeIndex, state: &mut PatternState, ) { - constrain_pattern_help( - types, - constraints, - env, - pattern, - region, - expected, - state, - true, - ); + constrain_pattern_help(types, constraints, env, pattern, region, expected, state); } #[allow(clippy::too_many_arguments)] @@ -270,7 +261,6 @@ pub fn constrain_pattern_help( region: Region, expected: PExpectedTypeIndex, state: &mut PatternState, - is_shallow: bool, ) { match pattern { Underscore => { @@ -300,27 +290,6 @@ pub fn constrain_pattern_help( .push(constraints.is_open_type(type_index)); } - // Identifiers introduced in nested patterns get let constraints - // and therefore don't need fx_pattern_suffix constraints. - if is_shallow { - match symbol.suffix() { - IdentSuffix::None => { - // Unsuffixed identifiers should be constrained after we know if they're functions - state - .delayed_fx_suffix_constraints - .push(constraints.fx_pattern_suffix(*symbol, type_index, region)); - } - IdentSuffix::Bang => { - // Bang suffixed identifiers are always required to be functions - // We constrain this before the function's body, - // so that we don't think it's pure and complain about leftover statements - state - .constraints - .push(constraints.fx_pattern_suffix(*symbol, type_index, region)); - } - } - } - state.headers.insert( *symbol, Loc { @@ -350,7 +319,6 @@ pub fn constrain_pattern_help( subpattern.region, expected, state, - false, ) } @@ -584,7 +552,6 @@ pub fn constrain_pattern_help( loc_pattern.region, expected, state, - false, ); pat_type @@ -683,7 +650,6 @@ pub fn constrain_pattern_help( loc_guard.region, expected, state, - false, ); RecordField::Demanded(pat_type) @@ -807,7 +773,6 @@ pub fn constrain_pattern_help( loc_pat.region, expected, state, - false, ); } @@ -864,7 +829,6 @@ pub fn constrain_pattern_help( loc_pattern.region, expected, state, - false, ); } @@ -930,7 +894,6 @@ pub fn constrain_pattern_help( loc_arg_pattern.region, arg_pattern_expected, state, - false, ); // Next, link `whole_var` to the opaque type of "@Id who" diff --git a/crates/compiler/load/tests/test_reporting.rs b/crates/compiler/load/tests/test_reporting.rs index d3bbf36b60..0d45c48db5 100644 --- a/crates/compiler/load/tests/test_reporting.rs +++ b/crates/compiler/load/tests/test_reporting.rs @@ -15666,7 +15666,7 @@ All branches in an `if` must have the same type! @r###" ── MISSING EXCLAMATION in /code/proj/Main.roc ────────────────────────────────── - This is an effectful function, but its name does not indicate so: + This function is effectful, but its name does not indicate so: 10│ forEach! = \l, f -> ^ @@ -15703,7 +15703,7 @@ All branches in an `if` must have the same type! @r###" ── UNNECESSARY EXCLAMATION in /code/proj/Main.roc ────────────────────────────── - This is a pure function, but its name suggests otherwise: + This function is pure, but its name suggests otherwise: 12│ mapOk = \result, fn! -> ^^^ diff --git a/crates/compiler/solve/src/solve.rs b/crates/compiler/solve/src/solve.rs index a0e7a0f9ca..20f3e00d66 100644 --- a/crates/compiler/solve/src/solve.rs +++ b/crates/compiler/solve/src/solve.rs @@ -211,18 +211,7 @@ enum Work<'a> { constraint: &'a Constraint, }, CheckForInfiniteTypes(LocalDefVarsVec<(Symbol, Loc)>), - /// The ret_con part of a let constraint that does NOT introduces rigid and/or flex variables - LetConNoVariables { - scope: &'a Scope, - rank: Rank, - let_con: &'a LetConstraint, - - /// The variables used to store imported types in the Subs. - /// The `Contents` are copied from the source module, but to - /// mimic `type_to_var`, we must add these variables to `Pools` - /// at the correct rank - pool_variables: &'a [Variable], - }, + CheckSuffixFx(LocalDefVarsVec<(Symbol, Loc)>), /// The ret_con part of a let constraint that introduces rigid and/or flex variables /// /// These introduced variables must be generalized, hence this variant @@ -288,56 +277,6 @@ fn solve( continue; } - Work::LetConNoVariables { - scope, - rank, - let_con, - pool_variables, - } => { - // NOTE be extremely careful with shadowing here - let offset = let_con.defs_and_ret_constraint.index(); - let ret_constraint = &env.constraints.constraints[offset + 1]; - - // Add a variable for each def to new_vars_by_env. - let local_def_vars = LocalDefVarsVec::from_def_types( - env, - rank, - problems, - abilities_store, - obligation_cache, - &mut can_types, - aliases, - let_con.def_types, - ); - - env.pools.get_mut(rank).extend(pool_variables); - - let mut new_scope = scope.clone(); - for (symbol, loc_var) in local_def_vars.iter() { - check_ability_specialization( - env, - rank, - abilities_store, - obligation_cache, - awaiting_specializations, - problems, - *symbol, - *loc_var, - ); - - new_scope.insert_symbol_var_if_vacant(*symbol, loc_var.value); - } - - stack.push(Work::Constraint { - scope: env.arena.alloc(new_scope), - rank, - constraint: ret_constraint, - }); - // Check for infinite types first - stack.push(Work::CheckForInfiniteTypes(local_def_vars)); - - continue; - } Work::LetConIntroducesVariables { scope, rank, @@ -456,14 +395,8 @@ fn solve( new_scope.insert_symbol_var_if_vacant(*symbol, loc_var.value); - solve_suffix_fx( - env, - problems, - host_exposed_symbols, - FxSuffixKind::Let(*symbol), - loc_var.value, - &loc_var.region, - ); + // At the time of introduction, promote explicitly-effectful symbols. + promote_effectful_symbol(env, FxSuffixKind::Let(*symbol), loc_var.value); } // Note that this vars_by_symbol is the one returned by the @@ -473,20 +406,42 @@ fn solve( mark: final_mark, }; - // Now solve the body, using the new vars_by_symbol which includes - // the assignments' name-to-variable mappings. - stack.push(Work::Constraint { - scope: env.arena.alloc(new_scope), - rank, - constraint: ret_constraint, - }); - // Check for infinite types first - stack.push(Work::CheckForInfiniteTypes(local_def_vars)); + let next_work = [ + // Check for infinite types first + Work::CheckForInfiniteTypes(local_def_vars.clone()), + // Now solve the body, using the new vars_by_symbol which includes + // the assignments' name-to-variable mappings. + Work::Constraint { + scope: env.arena.alloc(new_scope), + rank, + constraint: ret_constraint, + }, + // Finally, check the suffix fx, after we have solved all types. + Work::CheckSuffixFx(local_def_vars), + ]; + + for work in next_work.into_iter().rev() { + stack.push(work); + } state = state_for_ret_con; continue; } + + Work::CheckSuffixFx(local_def_vars) => { + for (symbol, loc_var) in local_def_vars.iter() { + solve_suffix_fx( + env, + problems, + host_exposed_symbols, + FxSuffixKind::Let(*symbol), + loc_var.value, + &loc_var.region, + ); + } + continue; + } }; state = match constraint { @@ -1004,100 +959,64 @@ fn solve( let offset = let_con.defs_and_ret_constraint.index(); let defs_constraint = &env.constraints.constraints[offset]; - let ret_constraint = &env.constraints.constraints[offset + 1]; let flex_vars = &env.constraints.variables[let_con.flex_vars.indices()]; let rigid_vars = &env.constraints[let_con.rigid_vars]; let pool_variables = &env.constraints.variables[pool_slice.indices()]; - if matches!(&ret_constraint, True) && rigid_vars.is_empty() { - debug_assert!(pool_variables.is_empty()); - - env.introduce(rank, flex_vars); - - // If the return expression is guaranteed to solve, - // solve the assignments themselves and move on. - stack.push(Work::Constraint { - scope, - rank, - constraint: defs_constraint, - }); - - state - } else if rigid_vars.is_empty() && let_con.flex_vars.is_empty() { - // items are popped from the stack in reverse order. That means that we'll - // first solve then defs_constraint, and then (eventually) the ret_constraint. - // - // Note that the LetConSimple gets the current env and rank, - // and not the env/rank from after solving the defs_constraint - stack.push(Work::LetConNoVariables { - scope, - rank, - let_con, - pool_variables, - }); - stack.push(Work::Constraint { - scope, - rank, - constraint: defs_constraint, - }); - - state + // If the let-binding is generalizable, work at the next rank (which will be + // the rank at which introduced variables will become generalized, if they end up + // staying there); otherwise, stay at the current level. + let binding_rank = if let_con.generalizable.0 { + rank.next() } else { - // If the let-binding is generalizable, work at the next rank (which will be - // the rank at which introduced variables will become generalized, if they end up - // staying there); otherwise, stay at the current level. - let binding_rank = if let_con.generalizable.0 { - rank.next() - } else { - rank - }; + rank + }; - // determine the next pool - if binding_rank.into_usize() < env.pools.len() { - // Nothing to do, we already accounted for the next rank, no need to - // adjust the pools - } else { - // we should be off by one at this point - debug_assert_eq!(binding_rank.into_usize(), 1 + env.pools.len()); - env.pools.extend_to(binding_rank.into_usize()); - } + // determine the next pool + if binding_rank.into_usize() < env.pools.len() { + // Nothing to do, we already accounted for the next rank, no need to + // adjust the pools + } else { + // we should be off by one at this point + debug_assert_eq!(binding_rank.into_usize(), 1 + env.pools.len()); + env.pools.extend_to(binding_rank.into_usize()); + } - let pool: &mut Vec = env.pools.get_mut(binding_rank); + let pool: &mut Vec = env.pools.get_mut(binding_rank); - // Introduce the variables of this binding, and extend the pool at our binding - // rank. - for &var in rigid_vars.iter().map(|v| &v.value).chain(flex_vars.iter()) { - env.subs.set_rank(var, binding_rank); - } - pool.reserve(rigid_vars.len() + flex_vars.len()); - pool.extend(rigid_vars.iter().map(|v| &v.value)); - pool.extend(flex_vars.iter()); + // Introduce the variables of this binding, and extend the pool at our binding + // rank. + for &var in rigid_vars.iter().map(|v| &v.value).chain(flex_vars.iter()) { + env.subs.set_rank(var, binding_rank); + } + pool.reserve(rigid_vars.len() + flex_vars.len()); + pool.extend(rigid_vars.iter().map(|v| &v.value)); + pool.extend(flex_vars.iter()); - // Now, run our binding constraint, generalize, then solve the rest of the - // program. - // - // Items are popped from the stack in reverse order. That means that we'll - // first solve the defs_constraint, and then (eventually) the ret_constraint. - // - // NB: LetCon gets the current scope's env and rank, not the env/rank from after solving the defs_constraint. - // That's because the defs constraints will be solved in next_rank if it is eligible for generalization. - // The LetCon will then generalize variables that are at a higher rank than the rank of the current scope. - stack.push(Work::LetConIntroducesVariables { - scope, - rank, - let_con, - pool_variables, - }); - stack.push(Work::Constraint { - scope, - rank: binding_rank, - constraint: defs_constraint, - }); + // Now, run our binding constraint, generalize, then solve the rest of the + // program. + // + // Items are popped from the stack in reverse order. That means that we'll + // first solve the defs_constraint, and then (eventually) the ret_constraint. + // + // NB: LetCon gets the current scope's env and rank, not the env/rank from after solving the defs_constraint. + // That's because the defs constraints will be solved in next_rank if it is eligible for generalization. + // The LetCon will then generalize variables that are at a higher rank than the rank of the current scope. + stack.push(Work::LetConIntroducesVariables { + scope, + rank, + let_con, + pool_variables, + }); + stack.push(Work::Constraint { + scope, + rank: binding_rank, + constraint: defs_constraint, + }); - state - } + state } IsOpenType(type_index) => { let actual = either_type_index_to_var( @@ -1773,6 +1692,20 @@ fn solve_suffix_fx( } } +fn promote_effectful_symbol(env: &mut InferenceEnv<'_>, kind: FxSuffixKind, variable: Variable) { + if kind.suffix() != IdentSuffix::Bang { + return; + } + if !matches!( + env.subs.get_content_without_compacting(variable), + Content::FlexVar(_) + ) { + return; + } + env.subs + .set_content(variable, Content::Structure(FlatType::EffectfulFunc)); +} + fn chase_alias_content(subs: &Subs, mut var: Variable) -> (Variable, &Content) { loop { match subs.get_content_without_compacting(var) { @@ -2147,7 +2080,7 @@ fn check_ability_specialization( } } -#[derive(Debug)] +#[derive(Debug, Clone)] enum LocalDefVarsVec { Stack(arrayvec::ArrayVec), Heap(Vec), From 561f3d9711645f511a055d5dba56e95803ffa31c Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 1 Jan 2025 23:12:35 -0600 Subject: [PATCH 4/5] Store lambda set variables as flex inference variables This is actually correct - the rigid approach is not. Lambda set variables should be inferred in-scope. --- crates/compiler/constrain/src/expr.rs | 15 ++++++--------- .../calls_result_in_unique_specializations.txt | 2 +- ...ursive_annotation_independently_issue_5256.txt | 4 ++-- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/compiler/constrain/src/expr.rs b/crates/compiler/constrain/src/expr.rs index 0e7254d950..f628ba42b1 100644 --- a/crates/compiler/constrain/src/expr.rs +++ b/crates/compiler/constrain/src/expr.rs @@ -3808,9 +3808,9 @@ fn instantiate_rigids( // wildcards are always freshly introduced in this annotation new_rigid_variables.extend(introduced_vars.wildcards.iter().copied()); - // lambda set vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); + let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); + new_infer_variables.extend(introduced_vars.inferred.iter().copied()); // ext-infer vars are always freshly introduced in this annotation new_infer_variables.extend( introduced_vars @@ -3818,9 +3818,8 @@ fn instantiate_rigids( .iter() .map(|&v| Loc::at_zero(v)), ); - - let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); - new_infer_variables.extend(introduced_vars.inferred.iter().copied()); + // lambda set vars are always freshly introduced in this annotation + new_infer_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); // Instantiate rigid variables if !rigid_substitution.is_empty() { @@ -3900,12 +3899,8 @@ fn instantiate_rigids_simple( // wildcards are always freshly introduced in this annotation new_rigid_variables.extend(introduced_vars.wildcards.iter().copied()); - // lambda set vars are always freshly introduced in this annotation - new_rigid_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); - let has_explicit_inference_variables = !introduced_vars.inferred.is_empty(); let mut new_infer_variables: Vec> = introduced_vars.inferred.clone(); - // ext-infer vars are always freshly introduced in this annotation new_infer_variables.extend( introduced_vars @@ -3913,6 +3908,8 @@ fn instantiate_rigids_simple( .iter() .map(|&v| Loc::at_zero(v)), ); + // lambda set vars are always freshly introduced in this annotation + new_infer_variables.extend(introduced_vars.lambda_sets.iter().map(|&v| Loc::at_zero(v))); // Instantiate rigid variables if !rigid_substitution.is_empty() { diff --git a/crates/compiler/uitest/tests/recursion/calls_result_in_unique_specializations.txt b/crates/compiler/uitest/tests/recursion/calls_result_in_unique_specializations.txt index 94dc9e3d5d..5f8d20fae3 100644 --- a/crates/compiler/uitest/tests/recursion/calls_result_in_unique_specializations.txt +++ b/crates/compiler/uitest/tests/recursion/calls_result_in_unique_specializations.txt @@ -7,7 +7,7 @@ main = # ^^^ a, Bool -[[foo(1)]]-> Str bar = \_ -> foo {} Bool.true -# ^^^ {}, Bool -[[]]-> Str +# ^^^ {}, Bool -[[foo(1)]]-> Str foo "" Bool.false # ^^^{inst} Str, Bool -[[foo(1)]]-> Str diff --git a/crates/compiler/uitest/tests/recursion/constrain_recursive_annotation_independently_issue_5256.txt b/crates/compiler/uitest/tests/recursion/constrain_recursive_annotation_independently_issue_5256.txt index 537ed97f2a..433d4aef27 100644 --- a/crates/compiler/uitest/tests/recursion/constrain_recursive_annotation_independently_issue_5256.txt +++ b/crates/compiler/uitest/tests/recursion/constrain_recursive_annotation_independently_issue_5256.txt @@ -5,8 +5,8 @@ main = map : { f1: (I64 -> I64) } -> List I64 map = \{ f1 } -> List.concat [f1 1] (map { f1 }) -# ^^^ { f1 : I64 -[[]]-> I64 } -[[map(2)]]-> List I64 -# ^^^ { f1 : I64 -[[]]-> I64 } -[[map(2)]]-> List I64 +# ^^^ { f1 : I64 -[[f(1)]]-> I64 } -[[map(2)]]-> List I64 +# ^^^ { f1 : I64 -[[f(1)]]-> I64 } -[[map(2)]]-> List I64 map { f1: f } From be99b82319d2aeca5020230a0d642ba7d3fbbb3d Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Thu, 2 Jan 2025 00:51:48 -0600 Subject: [PATCH 5/5] Drop debug assert I don't think this assert is actually accurate. --- crates/compiler/solve/src/deep_copy.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/compiler/solve/src/deep_copy.rs b/crates/compiler/solve/src/deep_copy.rs index 0ea8b2e657..c2a75a29b2 100644 --- a/crates/compiler/solve/src/deep_copy.rs +++ b/crates/compiler/solve/src/deep_copy.rs @@ -329,10 +329,6 @@ fn deep_copy_var_help( } let new_ambient_function_var = work!(ambient_function_var); - debug_assert_ne!( - ambient_function_var, new_ambient_function_var, - "lambda set cloned but its ambient function wasn't?" - ); subs.set_content_unchecked( lambda_set_var,