Merge branch 'refactor-closure-can' into scope-smarter-storage

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);
@@ -1024,12 +1024,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(
@@ -1150,232 +1150,121 @@ fn canonicalize_pending_value_def<'a>(
                 .introduced_variables
                 .union(&type_annotation.introduced_variables);
 
-            // bookkeeping for tail-call detection. If we're assigning to an
+            canonicalize_pending_body(
-            // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called.
+                env,
-            let outer_identifier = env.tailcallable_symbol;
+                output,
-
+                scope,
-            if let Pattern::Identifier(ref defined_symbol) = &loc_can_pattern.value {
+                var_store,
-                env.tailcallable_symbol = Some(*defined_symbol);
+                loc_can_pattern,
-            };
+                loc_expr,
-
+                Some(Loc::at(loc_ann.region, type_annotation)),
-            // 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) {
-                (
-                    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,
-                        ..
-                    }),
-                ) => {
-                    // 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
-                        )
-                    });
-
-                    // 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,
-                    };
-
-                    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(),
-                    });
-
-                    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 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,
-                    }
-                }
-            }
         }
-        // If we have a pattern, then the def has a body (that is, it's not a
+        Body(_loc_pattern, loc_can_pattern, loc_expr) => {
-        // standalone annotation), so we need to canonicalize the pattern and expr.
+            //
-        Body(loc_pattern, loc_can_pattern, loc_expr) => {
+            canonicalize_pending_body(
-            // bookkeeping for tail-call detection. If we're assigning to an
+                env,
-            // identifier (e.g. `f = \x -> ...`), then this symbol can be tail-called.
+                output,
-            let outer_identifier = env.tailcallable_symbol;
+                scope,
+                var_store,
+                loc_can_pattern,
+                loc_expr,
+                None,
+            )
+        }
+    }
+}
 
-            if let (&ast::Pattern::Identifier(_name), &Pattern::Identifier(ref defined_symbol)) =
+// TODO trim down these arguments!
-                (&loc_pattern.value, &loc_can_pattern.value)
+#[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<Pattern>,
+    loc_expr: &'a Loc<ast::Expr>,
+
+    opt_loc_annotation: Option<Loc<crate::annotation::Annotation>>,
+) -> DefOutput {
+    // 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);
 
-                // TODO isn't types_by_symbol enough? Do we need vars_by_symbol too?
+                let (mut closure_data, can_output) = crate::expr::canonicalize_closure(
-                vars_by_symbol.insert(*defined_symbol, expr_var);
+                    env,
-            };
+                    var_store,
+                    scope,
+                    arguments,
+                    body,
+                    Some(*defined_symbol),
+                );
 
-            // register the name of this closure, to make sure the closure won't capture it's own name
+                // reset the tailcallable_symbol
-            if let (Pattern::Identifier(ref defined_symbol), &ast::Expr::Closure(_, _)) =
+                env.tailcallable_symbol = outer_tailcallable;
-                (&loc_can_pattern.value, &loc_expr.value)
-            {
-                env.closure_name_symbol = Some(*defined_symbol);
-            };
 
-            let (mut loc_can_expr, can_output) =
+                // The closure is self tail recursive iff it tail calls itself (by defined name).
-                canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value);
+                let is_recursive = match can_output.tail_call {
+                    Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive,
+                    _ => Recursive::NotRecursive,
+                };
 
-            // reset the tailcallable_symbol
+                closure_data.recursive = is_recursive;
-            env.tailcallable_symbol = outer_identifier;
+                closure_data.name = *defined_symbol;
 
-            // First, make sure we are actually assigning an identifier instead of (for example) a tag.
+                let loc_can_expr = Loc::at(loc_expr.region, Expr::Closure(closure_data));
-            //
-            // 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
-                            )
-                        });
 
-                    // The closure is self tail recursive iff it tail calls itself (by defined name).
+                let def_references = DefReferences::Function(can_output.references.clone());
-                    let is_recursive = match can_output.tail_call {
+                output.union(can_output);
-                        Some(tail_symbol) if tail_symbol == *symbol => Recursive::TailRecursive,
-                        _ => Recursive::NotRecursive,
-                    };
 
-                    loc_can_expr.value = Closure(ClosureData {
+                (loc_can_expr, def_references)
-                        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(),
-                    });
 
-                    let def = single_can_def(
+            _ => {
-                        loc_can_pattern,
+                let (loc_can_expr, can_output) =
-                        loc_can_expr,
+                    canonicalize_expr(env, var_store, scope, loc_expr.region, &loc_expr.value);
-                        expr_var,
-                        None,
-                        vars_by_symbol.clone(),
-                    );
 
-                    output.union(can_output);
+                let def_references = DefReferences::Value(can_output.references.clone());
+                output.union(can_output);
 
-                    DefOutput {
+                (loc_can_expr, def_references)
-                        output,
-                        references: DefReferences::Function(closure_references),
-                        def,
-                    }
-                }
-                _ => {
-                    let refs = can_output.references.clone();
-
-                    let def = single_can_def(
-                        loc_can_pattern,
-                        loc_can_expr,
-                        expr_var,
-                        None,
-                        vars_by_symbol.clone(),
-                    );
-
-                    output.union(can_output);
-
-                    DefOutput {
-                        output,
-                        references: DefReferences::Value(refs),
-                        def,
-                    }
-                }
             }
         }
+    };
+
+    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);
+
+    let def = single_can_def(
+        loc_can_pattern,
+        loc_can_expr,
+        expr_var,
+        opt_loc_annotation,
+        vars_by_symbol,
+    );
+
+    DefOutput {
+        output,
+        references: def_references,
+        def,
     }
 }
 

compiler/can/src/env.rs

@@ -24,9 +24,6 @@ pub struct Env<'a> {
     /// current tail-callable symbol
     pub tailcallable_symbol: Option<Symbol>,
 
-    /// current closure name (if any)
-    pub closure_name_symbol: Option<Symbol>,
-
     /// Symbols of values/functions which were referenced by qualified lookups.
     pub qualified_value_lookups: VecSet<Symbol>,
 
@@ -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(),
         }
     }

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::{canonicalize_pattern, BindingsFromPattern, 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;
@@ -668,127 +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
+            let (closure_data, output) =
-            // into a top-level procedure for code gen.
+                canonicalize_closure(env, var_store, scope, loc_arg_patterns, loc_body_expr, None);
-            //
-            // 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.
+            (Closure(closure_data), output)
-            // 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: Vec<_> =
-                output.references.bound_symbols().copied().collect();
-
-            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: MutSet<Symbol> =
-                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!
-
-            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));
-                    }
-
-                    // 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());
-
-            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();
-
-            // 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,
-            )
         }
         ast::Expr::When(loc_cond, branches) => {
             // Infer the condition expression's type.
@@ -802,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) =
+                let (can_when_branch, branch_references) = canonicalize_when_branch(
-                    canonicalize_when_branch(env, var_store, scope, region, *branch, &mut output);
+                    env,
+                    var_store,
+                    scope.clone(),
+                    region,
+                    *branch,
+                    &mut output,
+                );
 
                 output.references.union_mut(&branch_references);
 
@@ -1060,20 +949,126 @@ 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<ast::Pattern<'a>>],
+    loc_body_expr: &'a Loc<ast::Expr<'a>>,
+    opt_def_name: Option<Symbol>,
+) -> (ClosureData, Output) {
+    // The globally unique symbol that will refer to this closure once it gets converted
+    // into a top-level procedure for code gen.
+    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
+    // 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: Vec<_> =
+        BindingsFromPattern::new_many(can_args.iter().map(|x| &x.1)).collect();
+
+    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>,
     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(
@@ -1108,23 +1103,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 BindingsFromPattern::new_many(patterns.iter()) {
+        if !output.references.has_value_lookup(symbol) {
+            env.problem(Problem::UnusedDef(symbol, region));
+        }
+    }
+
     (
         WhenBranch {
             patterns,

compiler/can/src/pattern.rs

@@ -644,69 +644,123 @@ 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)>
+/// An iterator over the bindings made by a pattern.
-where
+///
-    I: Iterator<Item = &'a Loc<Pattern>>,
+/// We attempt to make no allocations when we can.
-{
+pub enum BindingsFromPattern<'a> {
-    let mut answer = Vec::new();
+    Empty,
-
+    One(&'a Loc<Pattern>),
-    for loc_pattern in loc_patterns {
+    Many(Vec<BindingsFromPatternWork<'a>>),
-        add_bindings_from_patterns(&loc_pattern.region, &loc_pattern.value, &mut answer);
-    }
-
-    answer
 }
 
-/// helper function for idents_from_patterns
+pub enum BindingsFromPatternWork<'a> {
-fn add_bindings_from_patterns(
+    Pattern(&'a Loc<Pattern>),
-    region: &Region,
+    Destruct(&'a Loc<RecordDestruct>),
-    pattern: &Pattern,
+}
-    answer: &mut Vec<(Symbol, Region)>,
-) {
-    use Pattern::*;
 
-    match pattern {
+impl<'a> BindingsFromPattern<'a> {
-        Identifier(symbol)
+    pub fn new(initial: &'a Loc<Pattern>) -> Self {
-        | Shadowed(_, _, symbol)
+        Self::One(initial)
-        | AbilityMemberSpecialization {
+    }
-            ident: symbol,
+
-            specializes: _,
+    pub fn new_many<I>(mut it: I) -> Self
-        } => {
+    where
-            answer.push((*symbol, *region));
+        I: Iterator<Item = &'a Loc<Pattern>>,
+    {
+        if let (1, Some(1)) = it.size_hint() {
+            Self::new(it.next().unwrap())
+        } else {
+            Self::Many(it.map(BindingsFromPatternWork::Pattern).collect())
         }
-        AppliedTag {
+    }
-            arguments: loc_args,
+
-            ..
+    fn next_many(stack: &mut Vec<BindingsFromPatternWork<'a>>) -> Option<(Symbol, Region)> {
-        } => {
+        use Pattern::*;
-            for (_, loc_arg) in loc_args {
+
-                add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer);
+        while let Some(work) = 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));
+                            stack.extend(it);
+                        }
+                        UnwrappedOpaque { argument, .. } => {
+                            let (_, loc_arg) = &**argument;
+                            stack.push(Pattern(loc_arg));
+                        }
+                        RecordDestructure { destructs, .. } => {
+                            let it = destructs.iter().rev().map(Destruct);
+                            stack.extend(it);
+                        }
+                        NumLiteral(..)
+                        | IntLiteral(..)
+                        | FloatLiteral(..)
+                        | StrLiteral(_)
+                        | SingleQuote(_)
+                        | Underscore
+                        | MalformedPattern(_, _)
+                        | UnsupportedPattern(_)
+                        | OpaqueNotInScope(..) => (),
+                    }
+                }
+                BindingsFromPatternWork::Destruct(loc_destruct) => {
+                    match &loc_destruct.value.typ {
+                        DestructType::Required | DestructType::Optional(_, _) => {
+                            return Some((loc_destruct.value.symbol, loc_destruct.region));
+                        }
+                        DestructType::Guard(_, inner) => {
+                            // a guard does not introduce the symbol
+                            stack.push(BindingsFromPatternWork::Pattern(inner))
+                        }
+                    }
+                }
             }
         }
-        UnwrappedOpaque {
+
-            argument, opaque, ..
+        None
-        } => {
+    }
-            let (_, loc_arg) = &**argument;
+}
-            add_bindings_from_patterns(&loc_arg.region, &loc_arg.value, answer);
+
-            answer.push((*opaque, *region));
+impl<'a> Iterator for BindingsFromPattern<'a> {
+    type Item = (Symbol, Region);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        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),
         }
-        RecordDestructure { destructs, .. } => {
-            for Loc {
-                region,
-                value: RecordDestruct { symbol, .. },
-            } in destructs
-            {
-                answer.push((*symbol, *region));
-            }
-        }
-        NumLiteral(..)
-        | IntLiteral(..)
-        | FloatLiteral(..)
-        | StrLiteral(_)
-        | SingleQuote(_)
-        | Underscore
-        | MalformedPattern(_, _)
-        | UnsupportedPattern(_)
-        | OpaqueNotInScope(..) => (),
     }
 }
 

compiler/can/src/scope.rs

@@ -196,10 +196,6 @@ impl Scope {
         self.idents.get_index(ident).is_some()
     }
 
-    pub fn contains_symbol(&self, symbol: Symbol) -> bool {
-        self.idents.symbols.contains(&symbol)
-    }
-
     pub fn num_idents(&self) -> usize {
         self.idents.len()
     }