From 7a53534d41cc16f693511cd20e24d5b5c05cad09 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 16:14:15 +0200 Subject: [PATCH 01/13] rework how we filter captured symbols, and check for unused symbols --- compiler/can/src/expr.rs | 73 +++++++++++++++------------------------- 1 file changed, 28 insertions(+), 45 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index ce46645be0..836d35467f 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -6,10 +6,10 @@ use crate::num::{ finish_parsing_base, finish_parsing_float, finish_parsing_num, float_expr_from_result, int_expr_from_result, num_expr_from_result, FloatBound, IntBound, NumericBound, }; -use crate::pattern::{canonicalize_pattern, Pattern}; +use crate::pattern::{bindings_from_patterns, canonicalize_pattern, Pattern}; use crate::procedure::References; use crate::scope::Scope; -use roc_collections::{MutSet, SendMap, VecMap, VecSet}; +use roc_collections::{SendMap, VecMap, VecSet}; use roc_module::called_via::CalledVia; use roc_module::ident::{ForeignSymbol, Lowercase, TagName}; use roc_module::low_level::LowLevel; @@ -682,6 +682,7 @@ pub fn canonicalize_expr<'a>( // rest of this block, but keep the original around for later diffing. let original_scope = scope; let mut scope = original_scope.clone(); + let mut can_args = Vec::with_capacity(loc_arg_patterns.len()); let mut output = Output::default(); @@ -699,8 +700,7 @@ pub fn canonicalize_expr<'a>( can_args.push((var_store.fresh(), can_argument_pattern)); } - let bound_by_argument_patterns: Vec<_> = - output.references.bound_symbols().copied().collect(); + let bound_by_argument_patterns = bindings_from_patterns(can_args.iter().map(|x| &x.1)); let (loc_body_expr, new_output) = canonicalize_expr( env, @@ -710,50 +710,38 @@ pub fn canonicalize_expr<'a>( &loc_body_expr.value, ); - let mut captured_symbols: MutSet = - new_output.references.value_lookups().copied().collect(); - - // filter out the closure's name itself - captured_symbols.remove(&symbol); - - // symbols bound either in this pattern or deeper down are not captured! - captured_symbols.retain(|s| !new_output.references.bound_symbols().any(|x| x == s)); - captured_symbols.retain(|s| !bound_by_argument_patterns.contains(s)); - - // filter out top-level symbols - // those will be globally available, and don't need to be captured - captured_symbols.retain(|s| !env.top_level_symbols.contains(s)); - - // filter out imported symbols - // those will be globally available, and don't need to be captured - captured_symbols.retain(|s| s.module_id() == env.home); - - // TODO any Closure that has an empty `captured_symbols` list could be excluded! + let mut captured_symbols: Vec<_> = new_output + .references + .value_lookups() + .copied() + // filter out the closure's name itself + .filter(|s| *s != symbol) + // symbols bound either in this pattern or deeper down are not captured! + .filter(|s| !new_output.references.bound_symbols().any(|x| x == s)) + .filter(|s| bound_by_argument_patterns.iter().all(|(k, _)| s != k)) + // filter out top-level symbols those will be globally available, and don't need to be captured + .filter(|s| !env.top_level_symbols.contains(s)) + // filter out imported symbols those will be globally available, and don't need to be captured + .filter(|s| s.module_id() == env.home) + // filter out functions that don't close over anything + .filter(|s| !new_output.non_closures.contains(s)) + .filter(|s| !output.non_closures.contains(s)) + .map(|s| (s, var_store.fresh())) + .collect(); output.union(new_output); - // filter out aliases - debug_assert!(captured_symbols - .iter() - .all(|s| !output.references.references_type_def(*s))); - // captured_symbols.retain(|s| !output.references.referenced_type_defs.contains(s)); - - // filter out functions that don't close over anything - captured_symbols.retain(|s| !output.non_closures.contains(s)); - // Now that we've collected all the references, check to see if any of the args we defined // went unreferenced. If any did, report them as unused arguments. - for (sub_symbol, region) in scope.symbols() { - if !original_scope.contains_symbol(*sub_symbol) { - if !output.references.has_value_lookup(*sub_symbol) { - // The body never referenced this argument we declared. It's an unused argument! - env.problem(Problem::UnusedArgument(symbol, *sub_symbol, *region)); - } - + for (sub_symbol, region) in bound_by_argument_patterns { + if !output.references.has_value_lookup(sub_symbol) { + // The body never referenced this argument we declared. It's an unused argument! + env.problem(Problem::UnusedArgument(symbol, sub_symbol, region)); + } else { // We shouldn't ultimately count arguments as referenced locals. Otherwise, // we end up with weird conclusions like the expression (\x -> x + 1) // references the (nonexistent) local variable x! - output.references.remove_value_lookup(sub_symbol); + output.references.remove_value_lookup(&sub_symbol); } } @@ -761,11 +749,6 @@ pub fn canonicalize_expr<'a>( // when we canonicalize a surrounding def (if it exists) env.closures.insert(symbol, output.references.clone()); - let mut captured_symbols: Vec<_> = captured_symbols - .into_iter() - .map(|s| (s, var_store.fresh())) - .collect(); - // sort symbols, so we know the order in which they're stored in the closure record captured_symbols.sort(); From c65f90b8c5a6bfabce5cae8849278a439ecefe9b Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 16:22:00 +0200 Subject: [PATCH 02/13] refactor closure canonicalization --- compiler/can/src/expr.rs | 218 +++++++++++++++++++++------------------ 1 file changed, 115 insertions(+), 103 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 836d35467f..5a0c404ed0 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -668,110 +668,10 @@ pub fn canonicalize_expr<'a>( unreachable!("Backpassing should have been desugared by now") } ast::Expr::Closure(loc_arg_patterns, loc_body_expr) => { - // The globally unique symbol that will refer to this closure once it gets converted - // into a top-level procedure for code gen. - // - // In the Foo module, this will look something like Foo.$1 or Foo.$2. - let symbol = env - .closure_name_symbol - .unwrap_or_else(|| env.gen_unique_symbol()); - env.closure_name_symbol = None; + let (closure_data, output) = + canonicalize_closure(env, var_store, scope, loc_arg_patterns, loc_body_expr); - // The body expression gets a new scope for canonicalization. - // Shadow `scope` to make sure we don't accidentally use the original one for the - // rest of this block, but keep the original around for later diffing. - let original_scope = scope; - let mut scope = original_scope.clone(); - - let mut can_args = Vec::with_capacity(loc_arg_patterns.len()); - let mut output = Output::default(); - - for loc_pattern in loc_arg_patterns.iter() { - let can_argument_pattern = canonicalize_pattern( - env, - var_store, - &mut scope, - &mut output, - FunctionArg, - &loc_pattern.value, - loc_pattern.region, - ); - - can_args.push((var_store.fresh(), can_argument_pattern)); - } - - let bound_by_argument_patterns = bindings_from_patterns(can_args.iter().map(|x| &x.1)); - - let (loc_body_expr, new_output) = canonicalize_expr( - env, - var_store, - &mut scope, - loc_body_expr.region, - &loc_body_expr.value, - ); - - let mut captured_symbols: Vec<_> = new_output - .references - .value_lookups() - .copied() - // filter out the closure's name itself - .filter(|s| *s != symbol) - // symbols bound either in this pattern or deeper down are not captured! - .filter(|s| !new_output.references.bound_symbols().any(|x| x == s)) - .filter(|s| bound_by_argument_patterns.iter().all(|(k, _)| s != k)) - // filter out top-level symbols those will be globally available, and don't need to be captured - .filter(|s| !env.top_level_symbols.contains(s)) - // filter out imported symbols those will be globally available, and don't need to be captured - .filter(|s| s.module_id() == env.home) - // filter out functions that don't close over anything - .filter(|s| !new_output.non_closures.contains(s)) - .filter(|s| !output.non_closures.contains(s)) - .map(|s| (s, var_store.fresh())) - .collect(); - - output.union(new_output); - - // Now that we've collected all the references, check to see if any of the args we defined - // went unreferenced. If any did, report them as unused arguments. - for (sub_symbol, region) in bound_by_argument_patterns { - if !output.references.has_value_lookup(sub_symbol) { - // The body never referenced this argument we declared. It's an unused argument! - env.problem(Problem::UnusedArgument(symbol, sub_symbol, region)); - } else { - // We shouldn't ultimately count arguments as referenced locals. Otherwise, - // we end up with weird conclusions like the expression (\x -> x + 1) - // references the (nonexistent) local variable x! - output.references.remove_value_lookup(&sub_symbol); - } - } - - // store the references of this function in the Env. This information is used - // when we canonicalize a surrounding def (if it exists) - env.closures.insert(symbol, output.references.clone()); - - // sort symbols, so we know the order in which they're stored in the closure record - captured_symbols.sort(); - - // store that this function doesn't capture anything. It will be promoted to a - // top-level function, and does not need to be captured by other surrounding functions. - if captured_symbols.is_empty() { - output.non_closures.insert(symbol); - } - - ( - Closure(ClosureData { - function_type: var_store.fresh(), - closure_type: var_store.fresh(), - closure_ext_var: var_store.fresh(), - return_type: var_store.fresh(), - name: symbol, - captured_symbols, - recursive: Recursive::NotRecursive, - arguments: can_args, - loc_body: Box::new(loc_body_expr), - }), - output, - ) + (Closure(closure_data), output) } ast::Expr::When(loc_cond, branches) => { // Infer the condition expression's type. @@ -1043,6 +943,118 @@ pub fn canonicalize_expr<'a>( ) } +pub fn canonicalize_closure<'a>( + env: &mut Env<'a>, + var_store: &mut VarStore, + scope: &mut Scope, + loc_arg_patterns: &'a [Loc>], + loc_body_expr: &'a Loc>, +) -> (ClosureData, Output) { + // The globally unique symbol that will refer to this closure once it gets converted + // into a top-level procedure for code gen. + // + // In the Foo module, this will look something like Foo.$1 or Foo.$2. + let symbol = env + .closure_name_symbol + .unwrap_or_else(|| env.gen_unique_symbol()); + env.closure_name_symbol = None; + + // The body expression gets a new scope for canonicalization. + // Shadow `scope` to make sure we don't accidentally use the original one for the + // rest of this block, but keep the original around for later diffing. + let original_scope = scope; + let mut scope = original_scope.clone(); + + let mut can_args = Vec::with_capacity(loc_arg_patterns.len()); + let mut output = Output::default(); + + for loc_pattern in loc_arg_patterns.iter() { + let can_argument_pattern = canonicalize_pattern( + env, + var_store, + &mut scope, + &mut output, + FunctionArg, + &loc_pattern.value, + loc_pattern.region, + ); + + can_args.push((var_store.fresh(), can_argument_pattern)); + } + + let bound_by_argument_patterns = bindings_from_patterns(can_args.iter().map(|x| &x.1)); + + let (loc_body_expr, new_output) = canonicalize_expr( + env, + var_store, + &mut scope, + loc_body_expr.region, + &loc_body_expr.value, + ); + + let mut captured_symbols: Vec<_> = new_output + .references + .value_lookups() + .copied() + // filter out the closure's name itself + .filter(|s| *s != symbol) + // symbols bound either in this pattern or deeper down are not captured! + .filter(|s| !new_output.references.bound_symbols().any(|x| x == s)) + .filter(|s| bound_by_argument_patterns.iter().all(|(k, _)| s != k)) + // filter out top-level symbols those will be globally available, and don't need to be captured + .filter(|s| !env.top_level_symbols.contains(s)) + // filter out imported symbols those will be globally available, and don't need to be captured + .filter(|s| s.module_id() == env.home) + // filter out functions that don't close over anything + .filter(|s| !new_output.non_closures.contains(s)) + .filter(|s| !output.non_closures.contains(s)) + .map(|s| (s, var_store.fresh())) + .collect(); + + output.union(new_output); + + // Now that we've collected all the references, check to see if any of the args we defined + // went unreferenced. If any did, report them as unused arguments. + for (sub_symbol, region) in bound_by_argument_patterns { + if !output.references.has_value_lookup(sub_symbol) { + // The body never referenced this argument we declared. It's an unused argument! + env.problem(Problem::UnusedArgument(symbol, sub_symbol, region)); + } else { + // We shouldn't ultimately count arguments as referenced locals. Otherwise, + // we end up with weird conclusions like the expression (\x -> x + 1) + // references the (nonexistent) local variable x! + output.references.remove_value_lookup(&sub_symbol); + } + } + + // store the references of this function in the Env. This information is used + // when we canonicalize a surrounding def (if it exists) + env.closures.insert(symbol, output.references.clone()); + + // sort symbols, so we know the order in which they're stored in the closure record + captured_symbols.sort(); + + // store that this function doesn't capture anything. It will be promoted to a + // top-level function, and does not need to be captured by other surrounding functions. + if captured_symbols.is_empty() { + output.non_closures.insert(symbol); + } + + let closure_data = ClosureData { + function_type: var_store.fresh(), + closure_type: var_store.fresh(), + closure_ext_var: var_store.fresh(), + return_type: var_store.fresh(), + name: symbol, + captured_symbols, + recursive: Recursive::NotRecursive, + arguments: can_args, + loc_body: Box::new(loc_body_expr), + }; + + (closure_data, output) +} + #[inline(always)] fn canonicalize_when_branch<'a>( env: &mut Env<'a>, From 41ee2c3e6ab176f452fb61e3e0c6acc13d14c36e Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 16:38:53 +0200 Subject: [PATCH 03/13] unwrapping of an Opaque does not count as a binding of the opaque name --- compiler/can/src/pattern.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index c35d214645..52ee8eaaed 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -682,12 +682,9 @@ fn add_bindings_from_patterns( add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer); } } - UnwrappedOpaque { - argument, opaque, .. - } => { + UnwrappedOpaque { argument, .. } => { let (_, loc_arg) = &**argument; add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer); - answer.push((*opaque, *region)); } RecordDestructure { destructs, .. } => { for Loc { From c28a0af9323ac8534a7aca20afe5ea08699ce571 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 16:39:21 +0200 Subject: [PATCH 04/13] refactor: special-case the canonicalization of a Closure def --- compiler/can/src/def.rs | 94 ++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 53 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 982f88068a..6115566172 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1146,62 +1146,51 @@ fn canonicalize_pending_value_def<'a>( .introduced_variables .union(&type_annotation.introduced_variables); - // bookkeeping for tail-call detection. If we're assigning to an - // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called. - let outer_identifier = env.tailcallable_symbol; - - if let Pattern::Identifier(ref defined_symbol) = &loc_can_pattern.value { - env.tailcallable_symbol = Some(*defined_symbol); - }; - - // register the name of this closure, to make sure the closure won't capture it's own name - if let (Pattern::Identifier(ref defined_symbol), &ast::Expr::Closure(_, _)) = - (&loc_can_pattern.value, &loc_expr.value) - { - env.closure_name_symbol = Some(*defined_symbol); - }; - pattern_to_vars_by_symbol(&mut vars_by_symbol, &loc_can_pattern.value, expr_var); - let (mut loc_can_expr, can_output) = - canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); - - output.references.union_mut(&can_output.references); - - // reset the tailcallable_symbol - env.tailcallable_symbol = outer_identifier; - // First, make sure we are actually assigning an identifier instead of (for example) a tag. // // If we're assigning (UserId userId) = ... then this is certainly not a closure declaration, // which also implies it's not a self tail call! // // Only defs of the form (foo = ...) can be closure declarations or self tail calls. - - match (&loc_can_pattern.value, &loc_can_expr.value) { + match (&loc_can_pattern.value, &loc_expr.value) { ( Pattern::Identifier(symbol) | Pattern::AbilityMemberSpecialization { ident: symbol, .. }, - Closure(ClosureData { - function_type, - closure_type, - closure_ext_var, - return_type, - name: closure_name, - arguments, - loc_body: body, - captured_symbols, - .. - }), + ast::Expr::Closure(arguments, body), ) => { + // bookkeeping for tail-call detection. If we're assigning to an + // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called. + let outer_identifier = env.tailcallable_symbol; + + if let Pattern::Identifier(ref defined_symbol) = &loc_can_pattern.value { + env.tailcallable_symbol = Some(*defined_symbol); + }; + + // register the name of this closure, to make sure the closure won't capture it's own name + if let (Pattern::Identifier(ref defined_symbol), &ast::Expr::Closure(_, _)) = + (&loc_can_pattern.value, &loc_expr.value) + { + env.closure_name_symbol = Some(*defined_symbol); + }; + + let (mut closure_data, can_output) = + crate::expr::canonicalize_closure(env, var_store, scope, arguments, body); + + // reset the tailcallable_symbol + env.tailcallable_symbol = outer_identifier; + + output.references.union_mut(&can_output.references); + // Since everywhere in the code it'll be referred to by its defined name, // remove its generated name from the closure map. (We'll re-insert it later.) - let closure_references = env.closures.remove(closure_name).unwrap_or_else(|| { - panic!( - "Tried to remove symbol {:?} from procedures, but it was not found: {:?}", - closure_name, env.closures - ) - }); + let closure_references = env.closures.remove(&closure_data.name).unwrap_or_else(|| { + panic!( + "Tried to remove symbol {:?} from procedures, but it was not found: {:?}", + closure_data.name, env.closures + ) + }); // The closure is self tail recursive iff it tail calls itself (by defined name). let is_recursive = match can_output.tail_call { @@ -1209,17 +1198,10 @@ fn canonicalize_pending_value_def<'a>( _ => Recursive::NotRecursive, }; - loc_can_expr.value = Closure(ClosureData { - function_type: *function_type, - closure_type: *closure_type, - closure_ext_var: *closure_ext_var, - return_type: *return_type, - name: *symbol, - captured_symbols: captured_symbols.clone(), - recursive: is_recursive, - arguments: arguments.clone(), - loc_body: body.clone(), - }); + closure_data.recursive = is_recursive; + closure_data.name = *symbol; + + let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data)); let def = single_can_def( loc_can_pattern, @@ -1237,7 +1219,13 @@ fn canonicalize_pending_value_def<'a>( def, } } + _ => { + let (loc_can_expr, can_output) = + canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); + + output.references.union_mut(&can_output.references); + let refs = can_output.references.clone(); let def = single_can_def( From 6783b66db70b20baf49a0054e963cf2424b11ce1 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 16:44:29 +0200 Subject: [PATCH 05/13] stop using env.closures --- compiler/can/src/def.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 6115566172..f41b8cd377 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1181,17 +1181,9 @@ fn canonicalize_pending_value_def<'a>( // reset the tailcallable_symbol env.tailcallable_symbol = outer_identifier; + let closure_references = can_output.references.clone(); output.references.union_mut(&can_output.references); - // Since everywhere in the code it'll be referred to by its defined name, - // remove its generated name from the closure map. (We'll re-insert it later.) - let closure_references = env.closures.remove(&closure_data.name).unwrap_or_else(|| { - panic!( - "Tried to remove symbol {:?} from procedures, but it was not found: {:?}", - closure_data.name, env.closures - ) - }); - // The closure is self tail recursive iff it tail calls itself (by defined name). let is_recursive = match can_output.tail_call { Some(tail_symbol) if tail_symbol == *symbol => Recursive::TailRecursive, From 465fad9da1035da324cfdb6b9e433f4702de06fd Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 17:01:18 +0200 Subject: [PATCH 06/13] refactor it all again --- compiler/can/src/def.rs | 298 +++++++++++++++------------------------- 1 file changed, 109 insertions(+), 189 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index f41b8cd377..17bdc0487e 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1020,12 +1020,12 @@ fn canonicalize_pending_value_def<'a>( ) -> DefOutput { use PendingValueDef::*; - // Make types for the body expr, even if we won't end up having a body. - let expr_var = var_store.fresh(); - let mut vars_by_symbol = SendMap::default(); - match pending_def { AnnotationOnly(_, loc_can_pattern, loc_ann) => { + // Make types for the body expr, even if we won't end up having a body. + let expr_var = var_store.fresh(); + let mut vars_by_symbol = SendMap::default(); + // annotation sans body cannot introduce new rigids that are visible in other annotations // but the rigids can show up in type error messages, so still register them let type_annotation = canonicalize_annotation( @@ -1146,210 +1146,130 @@ fn canonicalize_pending_value_def<'a>( .introduced_variables .union(&type_annotation.introduced_variables); - pattern_to_vars_by_symbol(&mut vars_by_symbol, &loc_can_pattern.value, expr_var); - - // First, make sure we are actually assigning an identifier instead of (for example) a tag. - // - // If we're assigning (UserId userId) = ... then this is certainly not a closure declaration, - // which also implies it's not a self tail call! - // - // Only defs of the form (foo = ...) can be closure declarations or self tail calls. - match (&loc_can_pattern.value, &loc_expr.value) { - ( - Pattern::Identifier(symbol) - | Pattern::AbilityMemberSpecialization { ident: symbol, .. }, - ast::Expr::Closure(arguments, body), - ) => { - // bookkeeping for tail-call detection. If we're assigning to an - // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called. - let outer_identifier = env.tailcallable_symbol; - - if let Pattern::Identifier(ref defined_symbol) = &loc_can_pattern.value { - env.tailcallable_symbol = Some(*defined_symbol); - }; - - // register the name of this closure, to make sure the closure won't capture it's own name - if let (Pattern::Identifier(ref defined_symbol), &ast::Expr::Closure(_, _)) = - (&loc_can_pattern.value, &loc_expr.value) - { - env.closure_name_symbol = Some(*defined_symbol); - }; - - let (mut closure_data, can_output) = - crate::expr::canonicalize_closure(env, var_store, scope, arguments, body); - - // reset the tailcallable_symbol - env.tailcallable_symbol = outer_identifier; - - let closure_references = can_output.references.clone(); - output.references.union_mut(&can_output.references); - - // The closure is self tail recursive iff it tail calls itself (by defined name). - let is_recursive = match can_output.tail_call { - Some(tail_symbol) if tail_symbol == *symbol => Recursive::TailRecursive, - _ => Recursive::NotRecursive, - }; - - closure_data.recursive = is_recursive; - closure_data.name = *symbol; - - let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data)); - - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - Some(Loc::at(loc_ann.region, type_annotation)), - vars_by_symbol.clone(), - ); - - output.union(can_output); - - DefOutput { - output, - references: DefReferences::Function(closure_references), - def, - } - } - - _ => { - let (loc_can_expr, can_output) = - canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); - - output.references.union_mut(&can_output.references); - - let refs = can_output.references.clone(); - - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - Some(Loc::at(loc_ann.region, type_annotation)), - vars_by_symbol.clone(), - ); - - output.union(can_output); - - DefOutput { - output, - references: DefReferences::Value(refs), - def, - } - } - } + canonicalize_pending_body( + env, + output, + scope, + var_store, + loc_can_pattern, + loc_expr, + Some(Loc::at(loc_ann.region, type_annotation)), + ) } - // If we have a pattern, then the def has a body (that is, it's not a - // standalone annotation), so we need to canonicalize the pattern and expr. - Body(loc_pattern, loc_can_pattern, loc_expr) => { + Body(_loc_pattern, loc_can_pattern, loc_expr) => { + // + canonicalize_pending_body( + env, + output, + scope, + var_store, + loc_can_pattern, + loc_expr, + None, + ) + } + } +} + +// TODO trim down these arguments! +#[allow(clippy::too_many_arguments)] +#[allow(clippy::cognitive_complexity)] +fn canonicalize_pending_body<'a>( + env: &mut Env<'a>, + mut output: Output, + scope: &mut Scope, + var_store: &mut VarStore, + + loc_can_pattern: Loc, + loc_expr: &'a Loc, + + opt_loc_annotation: Option>, +) -> DefOutput { + // Make types for the body expr, even if we won't end up having a body. + let expr_var = var_store.fresh(); + let mut vars_by_symbol = SendMap::default(); + + pattern_to_vars_by_symbol(&mut vars_by_symbol, &loc_can_pattern.value, expr_var); + + // First, make sure we are actually assigning an identifier instead of (for example) a tag. + // + // If we're assigning (UserId userId) = ... then this is certainly not a closure declaration, + // which also implies it's not a self tail call! + // + // Only defs of the form (foo = ...) can be closure declarations or self tail calls. + match (&loc_can_pattern.value, &loc_expr.value) { + ( + Pattern::Identifier(defined_symbol) + | Pattern::AbilityMemberSpecialization { + ident: defined_symbol, + .. + }, + ast::Expr::Closure(arguments, body), + ) => { // bookkeeping for tail-call detection. If we're assigning to an // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called. let outer_identifier = env.tailcallable_symbol; - - if let (&ast::Pattern::Identifier(_name), &Pattern::Identifier(ref defined_symbol)) = - (&loc_pattern.value, &loc_can_pattern.value) - { - env.tailcallable_symbol = Some(*defined_symbol); - - // TODO isn't types_by_symbol enough? Do we need vars_by_symbol too? - vars_by_symbol.insert(*defined_symbol, expr_var); - }; + env.tailcallable_symbol = Some(*defined_symbol); // register the name of this closure, to make sure the closure won't capture it's own name - if let (Pattern::Identifier(ref defined_symbol), &ast::Expr::Closure(_, _)) = - (&loc_can_pattern.value, &loc_expr.value) - { - env.closure_name_symbol = Some(*defined_symbol); - }; + env.closure_name_symbol = Some(*defined_symbol); - let (mut loc_can_expr, can_output) = - canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); + let (mut closure_data, can_output) = + crate::expr::canonicalize_closure(env, var_store, scope, arguments, body); // reset the tailcallable_symbol env.tailcallable_symbol = outer_identifier; - // First, make sure we are actually assigning an identifier instead of (for example) a tag. - // - // If we're assigning (UserId userId) = ... then this is certainly not a closure declaration, - // which also implies it's not a self tail call! - // - // Only defs of the form (foo = ...) can be closure declarations or self tail calls. - match (&loc_can_pattern.value, &loc_can_expr.value) { - ( - Pattern::Identifier(symbol), - Closure(ClosureData { - function_type, - closure_type, - closure_ext_var, - return_type, - name: closure_name, - arguments, - loc_body: body, - captured_symbols, - .. - }), - ) => { - // Since everywhere in the code it'll be referred to by its defined name, - // remove its generated name from the closure map. (We'll re-insert it later.) - let closure_references = env.closures.remove(closure_name).unwrap_or_else(|| { - panic!( - "Tried to remove symbol {:?} from procedures, but it was not found: {:?}", - closure_name, env.closures - ) - }); + let closure_references = can_output.references.clone(); + output.references.union_mut(&can_output.references); - // The closure is self tail recursive iff it tail calls itself (by defined name). - let is_recursive = match can_output.tail_call { - Some(tail_symbol) if tail_symbol == *symbol => Recursive::TailRecursive, - _ => Recursive::NotRecursive, - }; + // The closure is self tail recursive iff it tail calls itself (by defined name). + let is_recursive = match can_output.tail_call { + Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive, + _ => Recursive::NotRecursive, + }; - loc_can_expr.value = Closure(ClosureData { - function_type: *function_type, - closure_type: *closure_type, - closure_ext_var: *closure_ext_var, - return_type: *return_type, - name: *symbol, - captured_symbols: captured_symbols.clone(), - recursive: is_recursive, - arguments: arguments.clone(), - loc_body: body.clone(), - }); + closure_data.recursive = is_recursive; + closure_data.name = *defined_symbol; - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - None, - vars_by_symbol.clone(), - ); + let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data)); - output.union(can_output); + let def = single_can_def( + loc_can_pattern, + loc_can_expr, + expr_var, + opt_loc_annotation, + vars_by_symbol, + ); - DefOutput { - output, - references: DefReferences::Function(closure_references), - def, - } - } - _ => { - let refs = can_output.references.clone(); + output.union(can_output); - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - None, - vars_by_symbol.clone(), - ); + DefOutput { + output, + references: DefReferences::Function(closure_references), + def, + } + } - output.union(can_output); + _ => { + let (loc_can_expr, can_output) = + canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); - DefOutput { - output, - references: DefReferences::Value(refs), - def, - } - } + let def = single_can_def( + loc_can_pattern, + loc_can_expr, + expr_var, + opt_loc_annotation, + vars_by_symbol, + ); + + let refs = can_output.references.clone(); + output.union(can_output); + + DefOutput { + output, + references: DefReferences::Value(refs), + def, } } } From 2973af5f79df608e5823bbfa5c34b9ae2cf90cbb Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 17:11:33 +0200 Subject: [PATCH 07/13] get rid of env.closure_name_symbol --- compiler/can/src/def.rs | 17 +++++++++-------- compiler/can/src/env.rs | 4 ---- compiler/can/src/expr.rs | 10 +++------- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 17bdc0487e..9d2b013a1f 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1211,18 +1211,18 @@ fn canonicalize_pending_body<'a>( let outer_identifier = env.tailcallable_symbol; env.tailcallable_symbol = Some(*defined_symbol); - // register the name of this closure, to make sure the closure won't capture it's own name - env.closure_name_symbol = Some(*defined_symbol); - - let (mut closure_data, can_output) = - crate::expr::canonicalize_closure(env, var_store, scope, arguments, body); + let (mut closure_data, can_output) = crate::expr::canonicalize_closure( + env, + var_store, + scope, + arguments, + body, + Some(*defined_symbol), + ); // reset the tailcallable_symbol env.tailcallable_symbol = outer_identifier; - let closure_references = can_output.references.clone(); - output.references.union_mut(&can_output.references); - // The closure is self tail recursive iff it tail calls itself (by defined name). let is_recursive = match can_output.tail_call { Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive, @@ -1242,6 +1242,7 @@ fn canonicalize_pending_body<'a>( vars_by_symbol, ); + let closure_references = can_output.references.clone(); output.union(can_output); DefOutput { diff --git a/compiler/can/src/env.rs b/compiler/can/src/env.rs index 40523b2fb7..e5dfd2e6de 100644 --- a/compiler/can/src/env.rs +++ b/compiler/can/src/env.rs @@ -24,9 +24,6 @@ pub struct Env<'a> { /// current tail-callable symbol pub tailcallable_symbol: Option, - /// current closure name (if any) - pub closure_name_symbol: Option, - /// Symbols of values/functions which were referenced by qualified lookups. pub qualified_value_lookups: VecSet, @@ -57,7 +54,6 @@ impl<'a> Env<'a> { qualified_value_lookups: VecSet::default(), qualified_type_lookups: VecSet::default(), tailcallable_symbol: None, - closure_name_symbol: None, top_level_symbols: VecSet::default(), } } diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 5a0c404ed0..f04548d147 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -669,7 +669,7 @@ pub fn canonicalize_expr<'a>( } ast::Expr::Closure(loc_arg_patterns, loc_body_expr) => { let (closure_data, output) = - canonicalize_closure(env, var_store, scope, loc_arg_patterns, loc_body_expr); + canonicalize_closure(env, var_store, scope, loc_arg_patterns, loc_body_expr, None); (Closure(closure_data), output) } @@ -949,15 +949,11 @@ pub fn canonicalize_closure<'a>( scope: &mut Scope, loc_arg_patterns: &'a [Loc>], loc_body_expr: &'a Loc>, + opt_def_name: Option, ) -> (ClosureData, Output) { // The globally unique symbol that will refer to this closure once it gets converted // into a top-level procedure for code gen. - // - // In the Foo module, this will look something like Foo.$1 or Foo.$2. - let symbol = env - .closure_name_symbol - .unwrap_or_else(|| env.gen_unique_symbol()); - env.closure_name_symbol = None; + let symbol = opt_def_name.unwrap_or_else(|| env.gen_unique_symbol()); // The body expression gets a new scope for canonicalization. // Shadow `scope` to make sure we don't accidentally use the original one for the From 984ef53e75b7d1ae265fc7a2b76d314a8e65e5d7 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 17:29:32 +0200 Subject: [PATCH 08/13] shaving off a couple more lines --- compiler/can/src/def.rs | 152 ++++++++++++++++++---------------------- 1 file changed, 70 insertions(+), 82 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 9d2b013a1f..a0929c6111 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1185,94 +1185,82 @@ fn canonicalize_pending_body<'a>( opt_loc_annotation: Option>, ) -> DefOutput { - // Make types for the body expr, even if we won't end up having a body. + // We treat closure definitions `foo = \a, b -> ...` differntly from other body expressions, + // because they need more bookkeeping (for tail calls, closure captures, etc.) + // + // Only defs of the form `foo = ...` can be closure declarations or self tail calls. + let (loc_can_expr, def_references) = { + match (&loc_can_pattern.value, &loc_expr.value) { + ( + Pattern::Identifier(defined_symbol) + | Pattern::AbilityMemberSpecialization { + ident: defined_symbol, + .. + }, + ast::Expr::Closure(arguments, body), + ) => { + // bookkeeping for tail-call detection. + let outer_tailcallable = env.tailcallable_symbol; + env.tailcallable_symbol = Some(*defined_symbol); + + let (mut closure_data, can_output) = crate::expr::canonicalize_closure( + env, + var_store, + scope, + arguments, + body, + Some(*defined_symbol), + ); + + // reset the tailcallable_symbol + env.tailcallable_symbol = outer_tailcallable; + + // The closure is self tail recursive iff it tail calls itself (by defined name). + let is_recursive = match can_output.tail_call { + Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive, + _ => Recursive::NotRecursive, + }; + + closure_data.recursive = is_recursive; + closure_data.name = *defined_symbol; + + let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data)); + + let def_references = DefReferences::Function(can_output.references.clone()); + output.union(can_output); + + (loc_can_expr, def_references) + } + + _ => { + let (loc_can_expr, can_output) = + canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); + + let def_references = DefReferences::Value(can_output.references.clone()); + output.union(can_output); + + (loc_can_expr, def_references) + } + } + }; + let expr_var = var_store.fresh(); let mut vars_by_symbol = SendMap::default(); pattern_to_vars_by_symbol(&mut vars_by_symbol, &loc_can_pattern.value, expr_var); - // First, make sure we are actually assigning an identifier instead of (for example) a tag. - // - // If we're assigning (UserId userId) = ... then this is certainly not a closure declaration, - // which also implies it's not a self tail call! - // - // Only defs of the form (foo = ...) can be closure declarations or self tail calls. - match (&loc_can_pattern.value, &loc_expr.value) { - ( - Pattern::Identifier(defined_symbol) - | Pattern::AbilityMemberSpecialization { - ident: defined_symbol, - .. - }, - ast::Expr::Closure(arguments, body), - ) => { - // bookkeeping for tail-call detection. If we're assigning to an - // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called. - let outer_identifier = env.tailcallable_symbol; - env.tailcallable_symbol = Some(*defined_symbol); + let def = single_can_def( + loc_can_pattern, + loc_can_expr, + expr_var, + opt_loc_annotation, + vars_by_symbol, + ); - let (mut closure_data, can_output) = crate::expr::canonicalize_closure( - env, - var_store, - scope, - arguments, - body, - Some(*defined_symbol), - ); - - // reset the tailcallable_symbol - env.tailcallable_symbol = outer_identifier; - - // The closure is self tail recursive iff it tail calls itself (by defined name). - let is_recursive = match can_output.tail_call { - Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive, - _ => Recursive::NotRecursive, - }; - - closure_data.recursive = is_recursive; - closure_data.name = *defined_symbol; - - let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data)); - - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - opt_loc_annotation, - vars_by_symbol, - ); - - let closure_references = can_output.references.clone(); - output.union(can_output); - - DefOutput { - output, - references: DefReferences::Function(closure_references), - def, - } - } - - _ => { - let (loc_can_expr, can_output) = - canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value); - - let def = single_can_def( - loc_can_pattern, - loc_can_expr, - expr_var, - opt_loc_annotation, - vars_by_symbol, - ); - - let refs = can_output.references.clone(); - output.union(can_output); - - DefOutput { - output, - references: DefReferences::Value(refs), - def, - } - } + DefOutput { + output, + references: def_references, + def, } } From 2d0a9c8531f5b35d3562e0d662806833abb34844 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 17:57:42 +0200 Subject: [PATCH 09/13] stop scope diffing in when canonicalization --- compiler/can/src/expr.rs | 22 ++++++++-------------- compiler/can/src/pattern.rs | 15 +++++++++------ compiler/can/src/scope.rs | 4 ---- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index f04548d147..2b78a814d0 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1099,23 +1099,17 @@ fn canonicalize_when_branch<'a>( } }; - // Now that we've collected all the references for this branch, check to see if - // any of the new idents it defined were unused. If any were, report it. - for (symbol, region) in scope.symbols() { - let symbol = *symbol; - - if !output.references.has_type_or_value_lookup(symbol) - && !branch_output.references.has_type_or_value_lookup(symbol) - && !original_scope.contains_symbol(symbol) - && !scope.abilities_store.is_specialization_name(symbol) - { - env.problem(Problem::UnusedDef(symbol, *region)); - } - } - let references = branch_output.references.clone(); output.union(branch_output); + // Now that we've collected all the references for this branch, check to see if + // any of the new idents it defined were unused. If any were, report it. + for (symbol, region) in bindings_from_patterns(patterns.iter()) { + if !output.references.has_value_lookup(symbol) { + env.problem(Problem::UnusedDef(symbol, region)); + } + } + ( WhenBranch { patterns, diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index 52ee8eaaed..c83db68fac 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -687,12 +687,15 @@ fn add_bindings_from_patterns( add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer); } RecordDestructure { destructs, .. } => { - for Loc { - region, - value: RecordDestruct { symbol, .. }, - } in destructs - { - answer.push((*symbol, *region)); + for loc_destruct in destructs { + match loc_destruct.value.typ { + DestructType::Required | DestructType::Optional(_, _) => { + answer.push((loc_destruct.value.symbol, loc_destruct.region)); + } + DestructType::Guard(_, _) => { + // a guard does not introduce the symbol + } + } } } NumLiteral(..) diff --git a/compiler/can/src/scope.rs b/compiler/can/src/scope.rs index b45b7eea6c..af3518067b 100644 --- a/compiler/can/src/scope.rs +++ b/compiler/can/src/scope.rs @@ -104,10 +104,6 @@ impl Scope { self.idents.contains_key(ident) } - pub fn contains_symbol(&self, symbol: Symbol) -> bool { - self.symbols.contains_key(&symbol) - } - pub fn num_idents(&self) -> usize { self.idents.len() } From 454aa17586bdeef7c1d8a827fcf88b23fa871093 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 18:01:22 +0200 Subject: [PATCH 10/13] change where scope is cloned --- compiler/can/src/expr.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 2b78a814d0..5faf11f645 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -685,8 +685,14 @@ pub fn canonicalize_expr<'a>( let mut can_branches = Vec::with_capacity(branches.len()); for branch in branches.iter() { - let (can_when_branch, branch_references) = - canonicalize_when_branch(env, var_store, scope, region, *branch, &mut output); + let (can_when_branch, branch_references) = canonicalize_when_branch( + env, + var_store, + scope.clone(), + region, + *branch, + &mut output, + ); output.references.union_mut(&branch_references); @@ -1055,16 +1061,13 @@ pub fn canonicalize_closure<'a>( fn canonicalize_when_branch<'a>( env: &mut Env<'a>, var_store: &mut VarStore, - scope: &mut Scope, + mut scope: Scope, _region: Region, branch: &'a ast::WhenBranch<'a>, output: &mut Output, ) -> (WhenBranch, References) { let mut patterns = Vec::with_capacity(branch.patterns.len()); - let original_scope = scope; - let mut scope = original_scope.clone(); - // TODO report symbols not bound in all patterns for loc_pattern in branch.patterns.iter() { let can_pattern = canonicalize_pattern( From b2656635153070d2ad395efeb588f5d84d626485 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 18:44:06 +0200 Subject: [PATCH 11/13] reduce allocations in pattern iteration --- compiler/can/src/def.rs | 4 +- compiler/can/src/expr.rs | 7 +- compiler/can/src/pattern.rs | 129 +++++++++++++++++++++--------------- 3 files changed, 82 insertions(+), 58 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index a0929c6111..a621d50f23 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -5,7 +5,7 @@ use crate::env::Env; use crate::expr::ClosureData; use crate::expr::Expr::{self, *}; use crate::expr::{canonicalize_expr, Output, Recursive}; -use crate::pattern::{bindings_from_patterns, canonicalize_def_header_pattern, Pattern}; +use crate::pattern::{canonicalize_def_header_pattern, BindingsFromPattern, Pattern}; use crate::procedure::References; use crate::reference_matrix::ReferenceMatrix; use crate::scope::create_alias; @@ -478,7 +478,7 @@ pub(crate) fn canonicalize_defs<'a>( let mut symbol_to_index: Vec<(IdentId, u32)> = Vec::with_capacity(pending_value_defs.len()); for (def_index, pending_def) in pending_value_defs.iter().enumerate() { - for (s, r) in bindings_from_patterns(std::iter::once(pending_def.loc_pattern())) { + for (s, r) in BindingsFromPattern::new(pending_def.loc_pattern()) { // store the top-level defs, used to ensure that closures won't capture them if let PatternType::TopLevelDef = pattern_type { env.top_level_symbols.insert(s); diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 5faf11f645..caf5d32e26 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -6,7 +6,7 @@ use crate::num::{ finish_parsing_base, finish_parsing_float, finish_parsing_num, float_expr_from_result, int_expr_from_result, num_expr_from_result, FloatBound, IntBound, NumericBound, }; -use crate::pattern::{bindings_from_patterns, canonicalize_pattern, Pattern}; +use crate::pattern::{canonicalize_pattern, BindingsFromPattern, Pattern}; use crate::procedure::References; use crate::scope::Scope; use roc_collections::{SendMap, VecMap, VecSet}; @@ -984,7 +984,8 @@ pub fn canonicalize_closure<'a>( can_args.push((var_store.fresh(), can_argument_pattern)); } - let bound_by_argument_patterns = bindings_from_patterns(can_args.iter().map(|x| &x.1)); + let bound_by_argument_patterns: Vec<_> = + BindingsFromPattern::new_many(can_args.iter().map(|x| &x.1)).collect(); let (loc_body_expr, new_output) = canonicalize_expr( env, @@ -1107,7 +1108,7 @@ fn canonicalize_when_branch<'a>( // Now that we've collected all the references for this branch, check to see if // any of the new idents it defined were unused. If any were, report it. - for (symbol, region) in bindings_from_patterns(patterns.iter()) { + for (symbol, region) in BindingsFromPattern::new_many(patterns.iter()) { if !output.references.has_value_lookup(symbol) { env.problem(Problem::UnusedDef(symbol, region)); } diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index c83db68fac..9d35c77b18 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -644,69 +644,92 @@ fn malformed_pattern(env: &mut Env, problem: MalformedPatternProblem, region: Re Pattern::MalformedPattern(problem, region) } -pub fn bindings_from_patterns<'a, I>(loc_patterns: I) -> Vec<(Symbol, Region)> -where - I: Iterator>, -{ - let mut answer = Vec::new(); - - for loc_pattern in loc_patterns { - add_bindings_from_patterns(&loc_pattern.region, &loc_pattern.value, &mut answer); - } - - answer +enum BindingsFromPatternWork<'a> { + Pattern(&'a Loc), + Destruct(&'a Loc), } -/// helper function for idents_from_patterns -fn add_bindings_from_patterns( - region: &Region, - pattern: &Pattern, - answer: &mut Vec<(Symbol, Region)>, -) { - use Pattern::*; +pub struct BindingsFromPattern<'a> { + stack: Vec>, +} - match pattern { - Identifier(symbol) - | Shadowed(_, _, symbol) - | AbilityMemberSpecialization { - ident: symbol, - specializes: _, - } => { - answer.push((*symbol, *region)); +impl<'a> BindingsFromPattern<'a> { + pub fn new(initial: &'a Loc) -> Self { + Self { + stack: vec![BindingsFromPatternWork::Pattern(initial)], } - AppliedTag { - arguments: loc_args, - .. - } => { - for (_, loc_arg) in loc_args { - add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer); - } + } + + pub fn new_many(it: I) -> Self + where + I: Iterator>, + { + Self { + stack: it.map(BindingsFromPatternWork::Pattern).collect(), } - UnwrappedOpaque { argument, .. } => { - let (_, loc_arg) = &**argument; - add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer); - } - RecordDestructure { destructs, .. } => { - for loc_destruct in destructs { - match loc_destruct.value.typ { - DestructType::Required | DestructType::Optional(_, _) => { - answer.push((loc_destruct.value.symbol, loc_destruct.region)); + } +} + +impl<'a> Iterator for BindingsFromPattern<'a> { + type Item = (Symbol, Region); + + fn next(&mut self) -> Option { + use Pattern::*; + + while let Some(work) = self.stack.pop() { + match work { + BindingsFromPatternWork::Pattern(loc_pattern) => { + use BindingsFromPatternWork::*; + + match &loc_pattern.value { + Identifier(symbol) + | Shadowed(_, _, symbol) + | AbilityMemberSpecialization { + ident: symbol, + specializes: _, + } => { + return Some((*symbol, loc_pattern.region)); + } + AppliedTag { + arguments: loc_args, + .. + } => { + let it = loc_args.iter().rev().map(|(_, p)| Pattern(p)); + self.stack.extend(it); + } + UnwrappedOpaque { argument, .. } => { + let (_, loc_arg) = &**argument; + self.stack.push(Pattern(loc_arg)); + } + RecordDestructure { destructs, .. } => { + let it = destructs.iter().rev().map(Destruct); + self.stack.extend(it); + } + NumLiteral(..) + | IntLiteral(..) + | FloatLiteral(..) + | StrLiteral(_) + | SingleQuote(_) + | Underscore + | MalformedPattern(_, _) + | UnsupportedPattern(_) + | OpaqueNotInScope(..) => (), } - DestructType::Guard(_, _) => { - // a guard does not introduce the symbol + } + BindingsFromPatternWork::Destruct(loc_destruct) => { + match loc_destruct.value.typ { + DestructType::Required | DestructType::Optional(_, _) => { + return Some((loc_destruct.value.symbol, loc_destruct.region)); + } + DestructType::Guard(_, _) => { + // a guard does not introduce the symbol + } } } } } - NumLiteral(..) - | IntLiteral(..) - | FloatLiteral(..) - | StrLiteral(_) - | SingleQuote(_) - | Underscore - | MalformedPattern(_, _) - | UnsupportedPattern(_) - | OpaqueNotInScope(..) => (), + + None } } From c487506ab420a4a11b833d6a22d537b06a449ed6 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 19:11:25 +0200 Subject: [PATCH 12/13] reduce allocations further --- compiler/can/src/pattern.rs | 70 ++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index 9d35c77b18..7487b76651 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -644,39 +644,40 @@ fn malformed_pattern(env: &mut Env, problem: MalformedPatternProblem, region: Re Pattern::MalformedPattern(problem, region) } -enum BindingsFromPatternWork<'a> { +/// An iterator over the bindings made by a pattern. +/// +/// We attempt to make no allocations when we can. +pub enum BindingsFromPattern<'a> { + Empty, + One(&'a Loc), + Many(Vec>), +} + +pub enum BindingsFromPatternWork<'a> { Pattern(&'a Loc), Destruct(&'a Loc), } -pub struct BindingsFromPattern<'a> { - stack: Vec>, -} - impl<'a> BindingsFromPattern<'a> { pub fn new(initial: &'a Loc) -> Self { - Self { - stack: vec![BindingsFromPatternWork::Pattern(initial)], - } + Self::One(initial) } - pub fn new_many(it: I) -> Self + pub fn new_many(mut it: I) -> Self where I: Iterator>, { - Self { - stack: it.map(BindingsFromPatternWork::Pattern).collect(), + if let (1, Some(1)) = it.size_hint() { + Self::new(it.next().unwrap()) + } else { + Self::Many(it.map(BindingsFromPatternWork::Pattern).collect()) } } -} -impl<'a> Iterator for BindingsFromPattern<'a> { - type Item = (Symbol, Region); - - fn next(&mut self) -> Option { + fn next_many(stack: &mut Vec>) -> Option<(Symbol, Region)> { use Pattern::*; - while let Some(work) = self.stack.pop() { + while let Some(work) = stack.pop() { match work { BindingsFromPatternWork::Pattern(loc_pattern) => { use BindingsFromPatternWork::*; @@ -695,15 +696,15 @@ impl<'a> Iterator for BindingsFromPattern<'a> { .. } => { let it = loc_args.iter().rev().map(|(_, p)| Pattern(p)); - self.stack.extend(it); + stack.extend(it); } UnwrappedOpaque { argument, .. } => { let (_, loc_arg) = &**argument; - self.stack.push(Pattern(loc_arg)); + stack.push(Pattern(loc_arg)); } RecordDestructure { destructs, .. } => { let it = destructs.iter().rev().map(Destruct); - self.stack.extend(it); + stack.extend(it); } NumLiteral(..) | IntLiteral(..) @@ -733,6 +734,35 @@ impl<'a> Iterator for BindingsFromPattern<'a> { } } +impl<'a> Iterator for BindingsFromPattern<'a> { + type Item = (Symbol, Region); + + fn next(&mut self) -> Option { + use Pattern::*; + + match self { + BindingsFromPattern::Empty => None, + BindingsFromPattern::One(loc_pattern) => match &loc_pattern.value { + Identifier(symbol) + | Shadowed(_, _, symbol) + | AbilityMemberSpecialization { + ident: symbol, + specializes: _, + } => { + let region = loc_pattern.region; + *self = Self::Empty; + Some((*symbol, region)) + } + _ => { + *self = Self::Many(vec![BindingsFromPatternWork::Pattern(loc_pattern)]); + self.next() + } + }, + BindingsFromPattern::Many(stack) => Self::next_many(stack), + } + } +} + fn flatten_str_literal(literal: &StrLiteral<'_>) -> Pattern { use ast::StrLiteral::*; From 1de3148cf1c2383a445e27d164ff786ba9d50280 Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 27 Apr 2022 19:18:26 +0200 Subject: [PATCH 13/13] fix problem with record guards --- compiler/can/src/pattern.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index 7487b76651..e43f9c3622 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -718,12 +718,13 @@ impl<'a> BindingsFromPattern<'a> { } } BindingsFromPatternWork::Destruct(loc_destruct) => { - match loc_destruct.value.typ { + match &loc_destruct.value.typ { DestructType::Required | DestructType::Optional(_, _) => { return Some((loc_destruct.value.symbol, loc_destruct.region)); } - DestructType::Guard(_, _) => { + DestructType::Guard(_, inner) => { // a guard does not introduce the symbol + stack.push(BindingsFromPatternWork::Pattern(inner)) } } }