From cc9873eb6d95e9aa29045d3be181a05e7794ec98 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 22:41:32 +0100 Subject: [PATCH 1/8] exploration --- compiler/can/src/def.rs | 143 ++++++++++++++++++++++++++++++--------- compiler/can/src/expr.rs | 108 ++++++++++++++++++++++++++++- 2 files changed, 218 insertions(+), 33 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index a12e31f527..c5c5dd3b15 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1,6 +1,8 @@ use crate::annotation::canonicalize_annotation; use crate::annotation::IntroducedVariables; use crate::env::Env; +use crate::expr::references_from_call_better; +use crate::expr::references_from_local_better; use crate::expr::ClosureData; use crate::expr::Expr::{self, *}; use crate::expr::{ @@ -14,6 +16,7 @@ use crate::scope::Scope; use roc_collections::all::{default_hasher, ImEntry, ImMap, ImSet, MutMap, MutSet, SendMap}; use roc_error_macros::todo_abilities; use roc_module::ident::Lowercase; +use roc_module::symbol::ModuleId; use roc_module::symbol::Symbol; use roc_parse::ast; use roc_parse::ast::TypeHeader; @@ -424,6 +427,97 @@ pub fn canonicalize_defs<'a>( ) } +fn crawl_for_references( + home: ModuleId, + input_references: References, + refs_by_def: &MutMap, + closures: &MutMap, +) -> References { + // Determine the full set of references by traversing the graph. + let mut visited_symbols = MutSet::default(); + let returned_lookups = ImSet::clone(&input_references.value_lookups); + + let mut output_references = input_references; + + // Start with the return expression's referenced locals. They're the only ones that count! + // + // If I have two defs which reference each other, but neither of them is referenced + // in the return expression, I don't want either of them (or their references) to end up + // in the final output.references. They were unused, and so were their references! + // + // The reason we need a graph here is so we don't overlook transitive dependencies. + // For example, if I have `a = b + 1` and the def returns `a + 1`, then the + // def as a whole references both `a` *and* `b`, even though it doesn't + // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, + // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. + for symbol in returned_lookups.into_iter() { + // We only care about local symbols in this analysis. + if symbol.module_id() == home { + // Traverse the graph and look up *all* the references for this local symbol. + let refs = references_from_local(symbol, &mut visited_symbols, refs_by_def, closures); + + output_references = output_references.union(refs); + } + } + + for symbol in ImSet::clone(&output_references.calls).into_iter() { + // Traverse the graph and look up *all* the references for this call. + // Reuse the same visited_symbols as before; if we already visited it, + // we won't learn anything new from visiting it again! + let refs = references_from_call(symbol, &mut visited_symbols, refs_by_def, closures); + + output_references = output_references.union(refs); + } + + output_references +} + +fn find_used_defs( + home: ModuleId, + value_lookups: &ImSet, + calls: &ImSet, + refs_by_def: &MutMap, + closures: &MutMap, +) -> References { + // Determine the full set of references by traversing the graph. + let mut visited_symbols = MutSet::default(); + + let mut output_references = References::default(); + + // Start with the return expression's referenced locals. They're the only ones that count! + // + // If I have two defs which reference each other, but neither of them is referenced + // in the return expression, I don't want either of them (or their references) to end up + // in the final output.references. They were unused, and so were their references! + // + // The reason we need a graph here is so we don't overlook transitive dependencies. + // For example, if I have `a = b + 1` and the def returns `a + 1`, then the + // def as a whole references both `a` *and* `b`, even though it doesn't + // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, + // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. + for symbol in value_lookups.iter().copied() { + // We only care about local symbols in this analysis. + if symbol.module_id() == home { + // Traverse the graph and look up *all* the references for this local symbol. + let refs = + references_from_local_better(symbol, &mut visited_symbols, refs_by_def, closures); + + output_references = output_references.union(refs); + } + } + + for symbol in calls.iter().copied() { + // Traverse the graph and look up *all* the references for this call. + // Reuse the same visited_symbols as before; if we already visited it, + // we won't learn anything new from visiting it again! + let refs = references_from_call_better(symbol, &mut visited_symbols, refs_by_def, closures); + + output_references = output_references.union(refs); + } + + output_references +} + #[inline(always)] pub fn sort_can_defs( env: &mut Env<'_>, @@ -440,41 +534,28 @@ pub fn sort_can_defs( output.aliases.insert(symbol, alias); } - // Determine the full set of references by traversing the graph. - let mut visited_symbols = MutSet::default(); - let returned_lookups = ImSet::clone(&output.references.value_lookups); + let initial = output.references.clone(); + let initial1 = output.references.clone(); + let initial2 = output.references.clone(); - // Start with the return expression's referenced locals. They're the only ones that count! - // - // If I have two defs which reference each other, but neither of them is referenced - // in the return expression, I don't want either of them (or their references) to end up - // in the final output.references. They were unused, and so were their references! - // - // The reason we need a graph here is so we don't overlook transitive dependencies. - // For example, if I have `a = b + 1` and the def returns `a + 1`, then the - // def as a whole references both `a` *and* `b`, even though it doesn't - // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, - // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. - for symbol in returned_lookups.into_iter() { - // We only care about local symbols in this analysis. - if symbol.module_id() == env.home { - // Traverse the graph and look up *all* the references for this local symbol. - let refs = - references_from_local(symbol, &mut visited_symbols, &refs_by_symbol, &env.closures); + let b = initial2.union(find_used_defs( + env.home, + &output.references.value_lookups, + &output.references.calls, + &refs_by_symbol, + &env.closures, + )); - output.references = output.references.union(refs); - } - } + let a = output.references.union(crawl_for_references( + env.home, + initial, + &refs_by_symbol, + &env.closures, + )); - for symbol in ImSet::clone(&output.references.calls).into_iter() { - // Traverse the graph and look up *all* the references for this call. - // Reuse the same visited_symbols as before; if we already visited it, - // we won't learn anything new from visiting it again! - let refs = - references_from_call(symbol, &mut visited_symbols, &refs_by_symbol, &env.closures); + assert!(a == b); - output.references = output.references.union(refs); - } + output.references = a; let mut defined_symbols: Vec = Vec::new(); let mut defined_symbols_set: ImSet = ImSet::default(); diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index a579f5094d..39f49d2483 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1147,7 +1147,111 @@ fn call_successors(call_symbol: Symbol, closures: &MutMap) - answer } -pub fn references_from_local<'a, T>( +#[derive(Debug)] +enum ReferencesFrom { + Local(Symbol), + Call(Symbol), +} + +pub(crate) fn references_from_local_better<'a, T>( + initial: Symbol, + visited: &'a mut MutSet, + refs_by_def: &'a MutMap, + closures: &'a MutMap, +) -> References +where + T: Debug, +{ + references_from_help( + ReferencesFrom::Local(initial), + visited, + refs_by_def, + closures, + ) +} + +pub(crate) fn references_from_call_better<'a, T>( + initial: Symbol, + visited: &'a mut MutSet, + refs_by_def: &'a MutMap, + closures: &'a MutMap, +) -> References +where + T: Debug, +{ + references_from_help( + ReferencesFrom::Call(initial), + visited, + refs_by_def, + closures, + ) +} + +fn references_from_help<'a, T>( + initial: ReferencesFrom, + visited: &'a mut MutSet, + refs_by_def: &'a MutMap, + closures: &'a MutMap, +) -> References +where + T: Debug, +{ + let mut stack: Vec = vec![initial]; + let mut result = References::default(); + + while let Some(job) = stack.pop() { + match job { + ReferencesFrom::Local(defined_symbol) => { + if let Some((_, refs)) = refs_by_def.get(&defined_symbol) { + if visited.contains(&defined_symbol) { + continue; + } + + visited.insert(defined_symbol); + + for local in refs.value_lookups.iter() { + stack.push(ReferencesFrom::Local(*local)); + + result.value_lookups.insert(*local); + } + + for call in refs.calls.iter() { + stack.push(ReferencesFrom::Call(*call)); + + result.calls.insert(*call); + } + } + } + ReferencesFrom::Call(call_symbol) => { + if let Some(references) = closures.get(&call_symbol) { + if visited.contains(&call_symbol) { + continue; + } + + visited.insert(call_symbol); + + result = result.union(references.clone()); + + for closed_over_local in references.value_lookups.iter() { + stack.push(ReferencesFrom::Local(*closed_over_local)); + + result.value_lookups.insert(*closed_over_local); + } + + for call in references.calls.iter() { + stack.push(ReferencesFrom::Call(*call)); + + result.calls.insert(*call); + } + } + } + } + } + + result +} + +pub(crate) fn references_from_local<'a, T>( defined_symbol: Symbol, visited: &'a mut MutSet, refs_by_def: &'a MutMap, @@ -1189,7 +1293,7 @@ where } } -pub fn references_from_call<'a, T>( +pub(crate) fn references_from_call<'a, T>( call_symbol: Symbol, visited: &'a mut MutSet, refs_by_def: &'a MutMap, From a170f461e0402a100491123bf2555fe005b1fbcd Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 22:44:14 +0100 Subject: [PATCH 2/8] cleanup --- compiler/can/src/def.rs | 60 +-------------------------- compiler/can/src/expr.rs | 88 ---------------------------------------- 2 files changed, 2 insertions(+), 146 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index c5c5dd3b15..5e5430eabc 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -427,51 +427,6 @@ pub fn canonicalize_defs<'a>( ) } -fn crawl_for_references( - home: ModuleId, - input_references: References, - refs_by_def: &MutMap, - closures: &MutMap, -) -> References { - // Determine the full set of references by traversing the graph. - let mut visited_symbols = MutSet::default(); - let returned_lookups = ImSet::clone(&input_references.value_lookups); - - let mut output_references = input_references; - - // Start with the return expression's referenced locals. They're the only ones that count! - // - // If I have two defs which reference each other, but neither of them is referenced - // in the return expression, I don't want either of them (or their references) to end up - // in the final output.references. They were unused, and so were their references! - // - // The reason we need a graph here is so we don't overlook transitive dependencies. - // For example, if I have `a = b + 1` and the def returns `a + 1`, then the - // def as a whole references both `a` *and* `b`, even though it doesn't - // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, - // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. - for symbol in returned_lookups.into_iter() { - // We only care about local symbols in this analysis. - if symbol.module_id() == home { - // Traverse the graph and look up *all* the references for this local symbol. - let refs = references_from_local(symbol, &mut visited_symbols, refs_by_def, closures); - - output_references = output_references.union(refs); - } - } - - for symbol in ImSet::clone(&output_references.calls).into_iter() { - // Traverse the graph and look up *all* the references for this call. - // Reuse the same visited_symbols as before; if we already visited it, - // we won't learn anything new from visiting it again! - let refs = references_from_call(symbol, &mut visited_symbols, refs_by_def, closures); - - output_references = output_references.union(refs); - } - - output_references -} - fn find_used_defs( home: ModuleId, value_lookups: &ImSet, @@ -535,10 +490,8 @@ pub fn sort_can_defs( } let initial = output.references.clone(); - let initial1 = output.references.clone(); - let initial2 = output.references.clone(); - let b = initial2.union(find_used_defs( + let b = initial.union(find_used_defs( env.home, &output.references.value_lookups, &output.references.calls, @@ -546,16 +499,7 @@ pub fn sort_can_defs( &env.closures, )); - let a = output.references.union(crawl_for_references( - env.home, - initial, - &refs_by_symbol, - &env.closures, - )); - - assert!(a == b); - - output.references = a; + output.references = b; let mut defined_symbols: Vec = Vec::new(); let mut defined_symbols_set: ImSet = ImSet::default(); diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 39f49d2483..65668696d0 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1251,94 +1251,6 @@ where result } -pub(crate) fn references_from_local<'a, T>( - defined_symbol: Symbol, - visited: &'a mut MutSet, - refs_by_def: &'a MutMap, - closures: &'a MutMap, -) -> References -where - T: Debug, -{ - let mut answer: References = References::new(); - - match refs_by_def.get(&defined_symbol) { - Some((_, refs)) => { - visited.insert(defined_symbol); - - for local in refs.value_lookups.iter() { - if !visited.contains(local) { - let other_refs: References = - references_from_local(*local, visited, refs_by_def, closures); - - answer = answer.union(other_refs); - } - - answer.value_lookups.insert(*local); - } - - for call in refs.calls.iter() { - if !visited.contains(call) { - let other_refs = references_from_call(*call, visited, refs_by_def, closures); - - answer = answer.union(other_refs); - } - - answer.calls.insert(*call); - } - - answer - } - None => answer, - } -} - -pub(crate) fn references_from_call<'a, T>( - call_symbol: Symbol, - visited: &'a mut MutSet, - refs_by_def: &'a MutMap, - closures: &'a MutMap, -) -> References -where - T: Debug, -{ - match closures.get(&call_symbol) { - Some(references) => { - let mut answer = references.clone(); - - visited.insert(call_symbol); - - for closed_over_local in references.value_lookups.iter() { - if !visited.contains(closed_over_local) { - let other_refs = - references_from_local(*closed_over_local, visited, refs_by_def, closures); - - answer = answer.union(other_refs); - } - - answer.value_lookups.insert(*closed_over_local); - } - - for call in references.calls.iter() { - if !visited.contains(call) { - let other_refs = references_from_call(*call, visited, refs_by_def, closures); - - answer = answer.union(other_refs); - } - - answer.calls.insert(*call); - } - - answer - } - None => { - // If the call symbol was not in the closure map, that means we're calling a non-function and - // will get a type mismatch later. For now, assume no references as a result of the "call." - References::new() - } - } -} - enum CanonicalizeRecordProblem { InvalidOptionalValue { field_name: Lowercase, From 1b1a7b038567fa1a458e456b14b065b4f4c959d1 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 22:54:24 +0100 Subject: [PATCH 3/8] more cleaned up --- compiler/can/src/def.rs | 39 ++++++++----------------------------- compiler/can/src/expr.rs | 42 ++++++++++++---------------------------- 2 files changed, 20 insertions(+), 61 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 5e5430eabc..ed3d9fb2a8 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1,14 +1,10 @@ use crate::annotation::canonicalize_annotation; use crate::annotation::IntroducedVariables; use crate::env::Env; -use crate::expr::references_from_call_better; -use crate::expr::references_from_local_better; +use crate::expr::references_from; use crate::expr::ClosureData; use crate::expr::Expr::{self, *}; -use crate::expr::{ - canonicalize_expr, local_successors, references_from_call, references_from_local, Output, - Recursive, -}; +use crate::expr::{canonicalize_expr, local_successors, Output, Recursive}; use crate::pattern::{bindings_from_patterns, canonicalize_pattern, Pattern}; use crate::procedure::References; use crate::scope::create_alias; @@ -434,11 +430,6 @@ fn find_used_defs( refs_by_def: &MutMap, closures: &MutMap, ) -> References { - // Determine the full set of references by traversing the graph. - let mut visited_symbols = MutSet::default(); - - let mut output_references = References::default(); - // Start with the return expression's referenced locals. They're the only ones that count! // // If I have two defs which reference each other, but neither of them is referenced @@ -450,27 +441,13 @@ fn find_used_defs( // def as a whole references both `a` *and* `b`, even though it doesn't // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. - for symbol in value_lookups.iter().copied() { - // We only care about local symbols in this analysis. - if symbol.module_id() == home { - // Traverse the graph and look up *all* the references for this local symbol. - let refs = - references_from_local_better(symbol, &mut visited_symbols, refs_by_def, closures); + let locals = value_lookups + .iter() + .copied() + .filter(|symbol| symbol.module_id() == home); + let calls = calls.iter().copied(); - output_references = output_references.union(refs); - } - } - - for symbol in calls.iter().copied() { - // Traverse the graph and look up *all* the references for this call. - // Reuse the same visited_symbols as before; if we already visited it, - // we won't learn anything new from visiting it again! - let refs = references_from_call_better(symbol, &mut visited_symbols, refs_by_def, closures); - - output_references = output_references.union(refs); - } - - output_references + references_from(locals, calls, refs_by_def, closures) } #[inline(always)] diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 65668696d0..400b39bcda 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1153,50 +1153,32 @@ enum ReferencesFrom { Call(Symbol), } -pub(crate) fn references_from_local_better<'a, T>( - initial: Symbol, - visited: &'a mut MutSet, +pub(crate) fn references_from<'a, T>( + locals: impl IntoIterator, + calls: impl IntoIterator, refs_by_def: &'a MutMap, closures: &'a MutMap, ) -> References where T: Debug, { - references_from_help( - ReferencesFrom::Local(initial), - visited, - refs_by_def, - closures, - ) -} + let mut stack = Vec::new(); -pub(crate) fn references_from_call_better<'a, T>( - initial: Symbol, - visited: &'a mut MutSet, - refs_by_def: &'a MutMap, - closures: &'a MutMap, -) -> References -where - T: Debug, -{ - references_from_help( - ReferencesFrom::Call(initial), - visited, - refs_by_def, - closures, - ) + stack.extend(locals.into_iter().map(ReferencesFrom::Local)); + stack.extend(calls.into_iter().map(ReferencesFrom::Call)); + + references_from_help(stack, refs_by_def, closures) } fn references_from_help<'a, T>( - initial: ReferencesFrom, - visited: &'a mut MutSet, + mut stack: Vec, refs_by_def: &'a MutMap, closures: &'a MutMap, ) -> References where T: Debug, { - let mut stack: Vec = vec![initial]; + let mut visited = Vec::new(); let mut result = References::default(); while let Some(job) = stack.pop() { @@ -1207,7 +1189,7 @@ where continue; } - visited.insert(defined_symbol); + visited.push(defined_symbol); for local in refs.value_lookups.iter() { stack.push(ReferencesFrom::Local(*local)); @@ -1228,7 +1210,7 @@ where continue; } - visited.insert(call_symbol); + visited.push(call_symbol); result = result.union(references.clone()); From cee1a787c9aca836f267a208347297427911b93f Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 23:07:27 +0100 Subject: [PATCH 4/8] and remove everything because it has no effect --- compiler/can/src/def.rs | 41 ------------------- compiler/can/src/expr.rs | 86 ---------------------------------------- 2 files changed, 127 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index ed3d9fb2a8..6a13576b78 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -1,7 +1,6 @@ use crate::annotation::canonicalize_annotation; use crate::annotation::IntroducedVariables; use crate::env::Env; -use crate::expr::references_from; use crate::expr::ClosureData; use crate::expr::Expr::{self, *}; use crate::expr::{canonicalize_expr, local_successors, Output, Recursive}; @@ -12,7 +11,6 @@ use crate::scope::Scope; use roc_collections::all::{default_hasher, ImEntry, ImMap, ImSet, MutMap, MutSet, SendMap}; use roc_error_macros::todo_abilities; use roc_module::ident::Lowercase; -use roc_module::symbol::ModuleId; use roc_module::symbol::Symbol; use roc_parse::ast; use roc_parse::ast::TypeHeader; @@ -423,33 +421,6 @@ pub fn canonicalize_defs<'a>( ) } -fn find_used_defs( - home: ModuleId, - value_lookups: &ImSet, - calls: &ImSet, - refs_by_def: &MutMap, - closures: &MutMap, -) -> References { - // Start with the return expression's referenced locals. They're the only ones that count! - // - // If I have two defs which reference each other, but neither of them is referenced - // in the return expression, I don't want either of them (or their references) to end up - // in the final output.references. They were unused, and so were their references! - // - // The reason we need a graph here is so we don't overlook transitive dependencies. - // For example, if I have `a = b + 1` and the def returns `a + 1`, then the - // def as a whole references both `a` *and* `b`, even though it doesn't - // directly mention `b` - because `a` depends on `b`. If we didn't traverse a graph here, - // we'd erroneously give a warning that `b` was unused since it wasn't directly referenced. - let locals = value_lookups - .iter() - .copied() - .filter(|symbol| symbol.module_id() == home); - let calls = calls.iter().copied(); - - references_from(locals, calls, refs_by_def, closures) -} - #[inline(always)] pub fn sort_can_defs( env: &mut Env<'_>, @@ -466,18 +437,6 @@ pub fn sort_can_defs( output.aliases.insert(symbol, alias); } - let initial = output.references.clone(); - - let b = initial.union(find_used_defs( - env.home, - &output.references.value_lookups, - &output.references.calls, - &refs_by_symbol, - &env.closures, - )); - - output.references = b; - let mut defined_symbols: Vec = Vec::new(); let mut defined_symbols_set: ImSet = ImSet::default(); diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 400b39bcda..e67af8e763 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1147,92 +1147,6 @@ fn call_successors(call_symbol: Symbol, closures: &MutMap) - answer } -#[derive(Debug)] -enum ReferencesFrom { - Local(Symbol), - Call(Symbol), -} - -pub(crate) fn references_from<'a, T>( - locals: impl IntoIterator, - calls: impl IntoIterator, - refs_by_def: &'a MutMap, - closures: &'a MutMap, -) -> References -where - T: Debug, -{ - let mut stack = Vec::new(); - - stack.extend(locals.into_iter().map(ReferencesFrom::Local)); - stack.extend(calls.into_iter().map(ReferencesFrom::Call)); - - references_from_help(stack, refs_by_def, closures) -} - -fn references_from_help<'a, T>( - mut stack: Vec, - refs_by_def: &'a MutMap, - closures: &'a MutMap, -) -> References -where - T: Debug, -{ - let mut visited = Vec::new(); - let mut result = References::default(); - - while let Some(job) = stack.pop() { - match job { - ReferencesFrom::Local(defined_symbol) => { - if let Some((_, refs)) = refs_by_def.get(&defined_symbol) { - if visited.contains(&defined_symbol) { - continue; - } - - visited.push(defined_symbol); - - for local in refs.value_lookups.iter() { - stack.push(ReferencesFrom::Local(*local)); - - result.value_lookups.insert(*local); - } - - for call in refs.calls.iter() { - stack.push(ReferencesFrom::Call(*call)); - - result.calls.insert(*call); - } - } - } - ReferencesFrom::Call(call_symbol) => { - if let Some(references) = closures.get(&call_symbol) { - if visited.contains(&call_symbol) { - continue; - } - - visited.push(call_symbol); - - result = result.union(references.clone()); - - for closed_over_local in references.value_lookups.iter() { - stack.push(ReferencesFrom::Local(*closed_over_local)); - - result.value_lookups.insert(*closed_over_local); - } - - for call in references.calls.iter() { - stack.push(ReferencesFrom::Call(*call)); - - result.calls.insert(*call); - } - } - } - } - } - - result -} - enum CanonicalizeRecordProblem { InvalidOptionalValue { field_name: Lowercase, From 14b53c0ccfc1baa49912172815c788945a346374 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 23:16:07 +0100 Subject: [PATCH 5/8] simplify local_successors --- compiler/can/src/def.rs | 8 +++----- compiler/can/src/expr.rs | 23 +++++++---------------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 6a13576b78..215aec3724 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -438,11 +438,9 @@ pub fn sort_can_defs( } let mut defined_symbols: Vec = Vec::new(); - let mut defined_symbols_set: ImSet = ImSet::default(); for symbol in can_defs_by_symbol.keys() { defined_symbols.push(*symbol); - defined_symbols_set.insert(*symbol); } // Use topological sort to reorder the defs based on their dependencies to one another. @@ -490,7 +488,7 @@ pub fn sort_can_defs( } // remove anything that is not defined in the current block - loc_succ.retain(|key| defined_symbols_set.contains(key)); + loc_succ.retain(|key| defined_symbols.contains(key)); loc_succ } @@ -533,7 +531,7 @@ pub fn sort_can_defs( } // remove anything that is not defined in the current block - loc_succ.retain(|key| defined_symbols_set.contains(key)); + loc_succ.retain(|key| defined_symbols.contains(key)); loc_succ } @@ -552,7 +550,7 @@ pub fn sort_can_defs( // NOTE: if the symbol is a closure we DONT look into its body // remove anything that is not defined in the current block - loc_succ.retain(|key| defined_symbols_set.contains(key)); + loc_succ.retain(|key| defined_symbols.contains(key)); // NOTE: direct recursion does matter here: `x = x` is invalid recursion! diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index e67af8e763..bbfa1d14df 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1117,34 +1117,25 @@ pub fn local_successors<'a>( references: &'a References, closures: &'a MutMap, ) -> ImSet { - let mut answer = references.value_lookups.clone(); + let mut answer: Vec<_> = references.value_lookups.iter().copied().collect(); - for call_symbol in references.calls.iter() { - answer = answer.union(call_successors(*call_symbol, closures)); - } + let mut stack: Vec<_> = references.calls.iter().copied().collect(); + let mut seen = Vec::new(); - answer -} - -fn call_successors(call_symbol: Symbol, closures: &MutMap) -> ImSet { - let mut answer = ImSet::default(); - let mut seen = MutSet::default(); - let mut queue = vec![call_symbol]; - - while let Some(symbol) = queue.pop() { + while let Some(symbol) = stack.pop() { if seen.contains(&symbol) { continue; } if let Some(references) = closures.get(&symbol) { answer.extend(references.value_lookups.iter().copied()); - queue.extend(references.calls.iter().copied()); + stack.extend(references.calls.iter().copied()); - seen.insert(symbol); + seen.push(symbol); } } - answer + answer.into_iter().collect() } enum CanonicalizeRecordProblem { From 725eb217d85a87dd79dcb54216475e492ff0b816 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 23:20:57 +0100 Subject: [PATCH 6/8] more vecs, less sets --- compiler/can/src/def.rs | 39 ++++++++++++++++++++++++--------------- compiler/can/src/expr.rs | 9 ++++++--- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index 215aec3724..c9a002d4b6 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -3,12 +3,12 @@ use crate::annotation::IntroducedVariables; use crate::env::Env; use crate::expr::ClosureData; use crate::expr::Expr::{self, *}; -use crate::expr::{canonicalize_expr, local_successors, Output, Recursive}; +use crate::expr::{canonicalize_expr, local_successors_with_duplicates, Output, Recursive}; use crate::pattern::{bindings_from_patterns, canonicalize_pattern, Pattern}; use crate::procedure::References; use crate::scope::create_alias; use crate::scope::Scope; -use roc_collections::all::{default_hasher, ImEntry, ImMap, ImSet, MutMap, MutSet, SendMap}; +use roc_collections::all::{default_hasher, ImEntry, ImMap, MutMap, MutSet, SendMap}; use roc_error_macros::todo_abilities; use roc_module::ident::Lowercase; use roc_module::symbol::Symbol; @@ -450,7 +450,7 @@ pub fn sort_can_defs( // recursive definitions. // All successors that occur in the body of a symbol. - let all_successors_without_self = |symbol: &Symbol| -> ImSet { + let all_successors_without_self = |symbol: &Symbol| -> Vec { // This may not be in refs_by_symbol. For example, the `f` in `f x` here: // // f = \z -> z @@ -471,7 +471,7 @@ pub fn sort_can_defs( // // In the above example, `f` cannot reference `a`, and in the closure // a call to `f` cannot cycle back to `a`. - let mut loc_succ = local_successors(references, &env.closures); + let mut loc_succ = local_successors_with_duplicates(references, &env.closures); // if the current symbol is a closure, peek into its body if let Some(References { value_lookups, .. }) = env.closures.get(symbol) { @@ -482,7 +482,7 @@ pub fn sort_can_defs( // DO NOT register a self-call behind a lambda! // // We allow `boom = \_ -> boom {}`, but not `x = x` - loc_succ.insert(*lookup); + loc_succ.push(*lookup); } } } @@ -490,9 +490,12 @@ pub fn sort_can_defs( // remove anything that is not defined in the current block loc_succ.retain(|key| defined_symbols.contains(key)); + loc_succ.sort(); + loc_succ.dedup(); + loc_succ } - None => ImSet::default(), + None => vec![], } }; @@ -500,7 +503,7 @@ pub fn sort_can_defs( // This is required to determine whether a symbol is recursive. Recursive symbols // (that are not faulty) always need a DeclareRec, even if there is just one symbol in the // group - let mut all_successors_with_self = |symbol: &Symbol| -> ImSet { + let mut all_successors_with_self = |symbol: &Symbol| -> Vec { // This may not be in refs_by_symbol. For example, the `f` in `f x` here: // // f = \z -> z @@ -521,31 +524,34 @@ pub fn sort_can_defs( // // In the above example, `f` cannot reference `a`, and in the closure // a call to `f` cannot cycle back to `a`. - let mut loc_succ = local_successors(references, &env.closures); + let mut loc_succ = local_successors_with_duplicates(references, &env.closures); // if the current symbol is a closure, peek into its body if let Some(References { value_lookups, .. }) = env.closures.get(symbol) { for lookup in value_lookups { - loc_succ.insert(*lookup); + loc_succ.push(*lookup); } } // remove anything that is not defined in the current block loc_succ.retain(|key| defined_symbols.contains(key)); + loc_succ.sort(); + loc_succ.dedup(); + loc_succ } - None => ImSet::default(), + None => vec![], } }; // If a symbol is a direct successor of itself, there is an invalid cycle. // The difference with the function above is that this one does not look behind lambdas, // but does consider direct self-recursion. - let direct_successors = |symbol: &Symbol| -> ImSet { + let direct_successors = |symbol: &Symbol| -> Vec { match refs_by_symbol.get(symbol) { Some((_, references)) => { - let mut loc_succ = local_successors(references, &env.closures); + let mut loc_succ = local_successors_with_duplicates(references, &env.closures); // NOTE: if the symbol is a closure we DONT look into its body @@ -554,9 +560,12 @@ pub fn sort_can_defs( // NOTE: direct recursion does matter here: `x = x` is invalid recursion! + loc_succ.sort(); + loc_succ.dedup(); + loc_succ } - None => ImSet::default(), + None => vec![], } }; @@ -730,14 +739,14 @@ pub fn sort_can_defs( fn group_to_declaration( group: &[Symbol], closures: &MutMap, - successors: &mut dyn FnMut(&Symbol) -> ImSet, + successors: &mut dyn FnMut(&Symbol) -> Vec, can_defs_by_symbol: &mut MutMap, declarations: &mut Vec, ) { use Declaration::*; // We want only successors in the current group, otherwise definitions get duplicated - let filtered_successors = |symbol: &Symbol| -> ImSet { + let filtered_successors = |symbol: &Symbol| -> Vec { let mut result = successors(symbol); result.retain(|key| group.contains(key)); diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index bbfa1d14df..2d94b8069e 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -1113,10 +1113,10 @@ fn canonicalize_when_branch<'a>( ) } -pub fn local_successors<'a>( +pub fn local_successors_with_duplicates<'a>( references: &'a References, closures: &'a MutMap, -) -> ImSet { +) -> Vec { let mut answer: Vec<_> = references.value_lookups.iter().copied().collect(); let mut stack: Vec<_> = references.calls.iter().copied().collect(); @@ -1135,7 +1135,10 @@ pub fn local_successors<'a>( } } - answer.into_iter().collect() + answer.sort(); + answer.dedup(); + + answer } enum CanonicalizeRecordProblem { From 7029717cd09bc51cf67a18bce369d2044871830c Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 23:46:25 +0100 Subject: [PATCH 7/8] fix missing import --- compiler/can/src/def.rs | 1 + compiler/can/src/expr.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index c9a002d4b6..cd4edd8e03 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -8,6 +8,7 @@ use crate::pattern::{bindings_from_patterns, canonicalize_pattern, Pattern}; use crate::procedure::References; use crate::scope::create_alias; use crate::scope::Scope; +use roc_collections::all::ImSet; use roc_collections::all::{default_hasher, ImEntry, ImMap, MutMap, MutSet, SendMap}; use roc_error_macros::todo_abilities; use roc_module::ident::Lowercase; diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 2d94b8069e..f427fb7125 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -9,7 +9,7 @@ use crate::num::{ use crate::pattern::{canonicalize_pattern, Pattern}; use crate::procedure::References; use crate::scope::Scope; -use roc_collections::all::{ImSet, MutMap, MutSet, SendMap}; +use roc_collections::all::{MutMap, MutSet, SendMap}; use roc_module::called_via::CalledVia; use roc_module::ident::{ForeignSymbol, Lowercase, TagName}; use roc_module::low_level::LowLevel; From 7b86a4b29e6f96b6066e399e0126188cfd478476 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 21 Mar 2022 20:18:18 +0100 Subject: [PATCH 8/8] optimize variable substitution in instantiate_rigids --- compiler/constrain/src/expr.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/constrain/src/expr.rs b/compiler/constrain/src/expr.rs index f467f6fae5..f093113971 100644 --- a/compiler/constrain/src/expr.rs +++ b/compiler/constrain/src/expr.rs @@ -10,7 +10,7 @@ use roc_can::expected::PExpected; use roc_can::expr::Expr::{self, *}; use roc_can::expr::{AccessorData, ClosureData, Field, WhenBranch}; use roc_can::pattern::Pattern; -use roc_collections::all::{HumanIndex, ImMap, MutMap, SendMap}; +use roc_collections::all::{HumanIndex, MutMap, SendMap}; use roc_module::ident::{Lowercase, TagName}; use roc_module::symbol::{ModuleId, Symbol}; use roc_region::all::{Loc, Region}; @@ -1670,14 +1670,14 @@ fn instantiate_rigids( let mut annotation = annotation.clone(); let mut new_rigid_variables: Vec = Vec::new(); - let mut rigid_substitution: ImMap = ImMap::default(); + let mut rigid_substitution: MutMap = MutMap::default(); for named in introduced_vars.named.iter() { use std::collections::hash_map::Entry::*; match ftv.entry(named.name.clone()) { Occupied(occupied) => { let existing_rigid = occupied.get(); - rigid_substitution.insert(named.variable, Type::Variable(*existing_rigid)); + rigid_substitution.insert(named.variable, *existing_rigid); } Vacant(vacant) => { // It's possible to use this rigid in nested defs @@ -1698,7 +1698,7 @@ fn instantiate_rigids( // Instantiate rigid variables if !rigid_substitution.is_empty() { - annotation.substitute(&rigid_substitution); + annotation.substitute_variables(&rigid_substitution); } // TODO investigate when we can skip this. It seems to only be required for correctness