Merge pull request #2973 from rtfeldman/scope-touchups

Scope touchups

compiler/can/src/expr.rs

@@ -955,7 +955,7 @@ pub fn canonicalize_closure<'a>(
     opt_def_name: Option<Symbol>,
 ) -> (ClosureData, Output) {
     scope.inner_scope(|inner_scope| {
-        canonicalize_closure_inner_scope(
+        canonicalize_closure_body(
             env,
             var_store,
             inner_scope,
@@ -966,7 +966,7 @@ pub fn canonicalize_closure<'a>(
     })
 }
 
-fn canonicalize_closure_inner_scope<'a>(
+fn canonicalize_closure_body<'a>(
     env: &mut Env<'a>,
     var_store: &mut VarStore,
     scope: &mut Scope,

compiler/can/src/module.rs

@@ -47,7 +47,6 @@ pub struct ModuleOutput {
     pub exposed_imports: MutMap<Symbol, Variable>,
     pub lookups: Vec<(Symbol, Variable, Region)>,
     pub problems: Vec<Problem>,
-    pub ident_ids: IdentIds,
     pub referenced_values: VecSet<Symbol>,
     pub referenced_types: VecSet<Symbol>,
     pub scope: Scope,
@@ -170,7 +169,7 @@ pub fn canonicalize_module_defs<'a>(
     var_store: &mut VarStore,
 ) -> Result<ModuleOutput, RuntimeError> {
     let mut can_exposed_imports = MutMap::default();
-    let mut scope = Scope::new(home, var_store, exposed_ident_ids);
+    let mut scope = Scope::new(home, exposed_ident_ids);
     let mut env = Env::new(home, dep_idents, module_ids);
     let num_deps = dep_idents.len();
 
@@ -533,8 +532,6 @@ pub fn canonicalize_module_defs<'a>(
                 }
             }
 
-            let ident_ids = scope.ident_ids.clone();
-
             let output = ModuleOutput {
                 scope,
                 aliases,
@@ -545,7 +542,6 @@ pub fn canonicalize_module_defs<'a>(
                 exposed_imports: can_exposed_imports,
                 problems: env.problems,
                 lookups,
-                ident_ids,
             };
 
             Ok(output)

compiler/can/src/scope.rs

@@ -1,5 +1,4 @@
-use roc_collections::all::{MutSet, SendMap};
-use roc_collections::SmallStringInterner;
+use roc_collections::{MutSet, SmallStringInterner, VecMap};
 use roc_module::ident::{Ident, Lowercase};
 use roc_module::symbol::{IdentIds, ModuleId, Symbol};
 use roc_problem::can::RuntimeError;
@@ -14,7 +13,7 @@ pub struct Scope {
     idents: IdentStore,
 
     /// The type aliases currently in scope
-    pub aliases: SendMap<Symbol, Alias>,
+    pub aliases: VecMap<Symbol, Alias>,
 
     /// The abilities currently in scope, and their implementors.
     pub abilities_store: AbilitiesStore,
@@ -29,11 +28,11 @@ pub struct Scope {
     exposed_ident_count: usize,
 }
 
-fn add_aliases(var_store: &mut VarStore) -> SendMap<Symbol, Alias> {
+fn add_aliases(var_store: &mut VarStore) -> VecMap<Symbol, Alias> {
     use roc_types::solved_types::{BuiltinAlias, FreeVars};
 
     let solved_aliases = roc_types::builtin_aliases::aliases();
-    let mut aliases = SendMap::default();
+    let mut aliases = VecMap::default();
 
     for (symbol, builtin_alias) in solved_aliases {
         let BuiltinAlias {
@@ -70,13 +69,13 @@ fn add_aliases(var_store: &mut VarStore) -> SendMap<Symbol, Alias> {
 }
 
 impl Scope {
-    pub fn new(home: ModuleId, _var_store: &mut VarStore, initial_ident_ids: IdentIds) -> Scope {
+    pub fn new(home: ModuleId, initial_ident_ids: IdentIds) -> Scope {
         Scope {
             home,
             exposed_ident_count: initial_ident_ids.len(),
             ident_ids: initial_ident_ids,
             idents: IdentStore::new(),
-            aliases: SendMap::default(),
+            aliases: VecMap::default(),
             // TODO(abilities): default abilities in scope
             abilities_store: AbilitiesStore::default(),
         }
@@ -118,6 +117,11 @@ impl Scope {
         }
     }
 
+    #[cfg(test)]
+    fn idents_in_scope(&self) -> impl Iterator<Item = Ident> + '_ {
+        self.idents.iter_idents()
+    }
+
     pub fn lookup_alias(&self, symbol: Symbol) -> Option<&Alias> {
         self.aliases.get(&symbol)
     }
@@ -343,15 +347,20 @@ impl Scope {
     where
         F: FnOnce(&mut Scope) -> T,
     {
+        // store enough information to roll back to the original outer scope
+        //
+        // - abilities_store: ability definitions not allowed in inner scopes
+        // - ident_ids: identifiers in inner scopes should still be available in the ident_ids
+        // - idents: we have to clone for now
+        // - aliases: stored in a VecMap, we just discard anything added in an inner scope
+        // - exposed_ident_count: unchanged
         let idents = self.idents.clone();
-        let aliases = self.aliases.clone();
-        let abilities_store = self.abilities_store.clone();
+        let aliases_count = self.aliases.len();
 
         let result = f(self);
 
         self.idents = idents;
-        self.aliases = aliases;
-        self.abilities_store = abilities_store;
+        self.aliases.truncate(aliases_count);
 
         result
     }
@@ -500,3 +509,136 @@ impl IdentStore {
         self.regions.push(region);
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use roc_module::symbol::ModuleIds;
+    use roc_region::all::Position;
+
+    use pretty_assertions::{assert_eq, assert_ne};
+
+    #[test]
+    fn scope_contains_introduced() {
+        let _register_module_debug_names = ModuleIds::default();
+        let mut scope = Scope::new(ModuleId::ATTR, IdentIds::default());
+
+        let region = Region::zero();
+        let ident = Ident::from("mezolit");
+
+        assert!(scope.lookup(&ident, region).is_err());
+
+        assert!(scope.introduce(ident.clone(), region).is_ok());
+
+        assert!(scope.lookup(&ident, region).is_ok());
+    }
+
+    #[test]
+    fn second_introduce_shadows() {
+        let _register_module_debug_names = ModuleIds::default();
+        let mut scope = Scope::new(ModuleId::ATTR, IdentIds::default());
+
+        let region1 = Region::from_pos(Position { offset: 10 });
+        let region2 = Region::from_pos(Position { offset: 20 });
+        let ident = Ident::from("mezolit");
+
+        assert!(scope.lookup(&ident, Region::zero()).is_err());
+
+        let first = scope.introduce(ident.clone(), region1).unwrap();
+        let (original_region, _ident, shadow_symbol) =
+            scope.introduce(ident.clone(), region2).unwrap_err();
+
+        scope.register_debug_idents();
+
+        assert_ne!(first, shadow_symbol);
+        assert_eq!(original_region, region1);
+
+        let lookup = scope.lookup(&ident, Region::zero()).unwrap();
+
+        assert_eq!(first, lookup);
+    }
+
+    #[test]
+    fn inner_scope_does_not_influence_outer() {
+        let _register_module_debug_names = ModuleIds::default();
+        let mut scope = Scope::new(ModuleId::ATTR, IdentIds::default());
+
+        let region = Region::zero();
+        let ident = Ident::from("uránia");
+
+        assert!(scope.lookup(&ident, region).is_err());
+
+        scope.inner_scope(|inner| {
+            assert!(inner.introduce(ident.clone(), region).is_ok());
+        });
+
+        assert!(scope.lookup(&ident, region).is_err());
+    }
+
+    #[test]
+    fn idents_with_inner_scope() {
+        let _register_module_debug_names = ModuleIds::default();
+        let mut scope = Scope::new(ModuleId::ATTR, IdentIds::default());
+
+        let idents: Vec<_> = scope.idents_in_scope().collect();
+
+        assert_eq!(
+            &idents,
+            &[
+                Ident::from("Box"),
+                Ident::from("Set"),
+                Ident::from("Dict"),
+                Ident::from("Str"),
+                Ident::from("Ok"),
+                Ident::from("False"),
+                Ident::from("List"),
+                Ident::from("True"),
+                Ident::from("Err"),
+            ]
+        );
+
+        let builtin_count = idents.len();
+
+        let region = Region::zero();
+
+        let ident1 = Ident::from("uránia");
+        let ident2 = Ident::from("malmok");
+        let ident3 = Ident::from("Járnak");
+
+        scope.introduce(ident1.clone(), region).unwrap();
+        scope.introduce(ident2.clone(), region).unwrap();
+        scope.introduce(ident3.clone(), region).unwrap();
+
+        let idents: Vec<_> = scope.idents_in_scope().collect();
+
+        assert_eq!(
+            &idents[builtin_count..],
+            &[ident1.clone(), ident2.clone(), ident3.clone(),]
+        );
+
+        scope.inner_scope(|inner| {
+            let ident4 = Ident::from("Ångström");
+            let ident5 = Ident::from("Sirály");
+
+            inner.introduce(ident4.clone(), region).unwrap();
+            inner.introduce(ident5.clone(), region).unwrap();
+
+            let idents: Vec<_> = inner.idents_in_scope().collect();
+
+            assert_eq!(
+                &idents[builtin_count..],
+                &[
+                    ident1.clone(),
+                    ident2.clone(),
+                    ident3.clone(),
+                    ident4,
+                    ident5
+                ]
+            );
+        });
+
+        let idents: Vec<_> = scope.idents_in_scope().collect();
+
+        assert_eq!(&idents[builtin_count..], &[ident1, ident2, ident3,]);
+    }
+}

compiler/can/tests/helpers/mod.rs

@@ -55,7 +55,7 @@ pub fn can_expr_with(arena: &Bump, home: ModuleId, expr_str: &str) -> CanExprOut
     // rules multiple times unnecessarily.
     let loc_expr = operator::desugar_expr(arena, &loc_expr);
 
-    let mut scope = Scope::new(home, &mut var_store, IdentIds::default());
+    let mut scope = Scope::new(home, IdentIds::default());
     scope.add_alias(
         Symbol::NUM_INT,
         Region::zero(),

compiler/collections/src/vec_map.rs

@@ -109,6 +109,11 @@ impl<K: PartialEq, V> VecMap<K, V> {
         self.values.iter()
     }
 
+    pub fn truncate(&mut self, len: usize) {
+        self.keys.truncate(len);
+        self.values.truncate(len);
+    }
+
     pub fn unzip(self) -> (Vec<K>, Vec<V>) {
         (self.keys, self.values)
     }

compiler/load_internal/src/docs.rs

@@ -89,14 +89,13 @@ pub struct Tag {
 pub fn generate_module_docs<'a>(
     scope: Scope,
     module_name: ModuleName,
-    ident_ids: &'a IdentIds,
     parsed_defs: &'a [Loc<ast::Def<'a>>],
 ) -> ModuleDocumentation {
     let (entries, _) =
         parsed_defs
             .iter()
             .fold((vec![], None), |(acc, maybe_comments_after), def| {
-                generate_entry_doc(ident_ids, acc, maybe_comments_after, &def.value)
+                generate_entry_doc(&scope.ident_ids, acc, maybe_comments_after, &def.value)
             });
 
     ModuleDocumentation {

compiler/load_internal/src/file.rs

@@ -3844,7 +3844,6 @@ fn canonicalize_and_constrain<'a>(
                     let docs = crate::docs::generate_module_docs(
                         module_output.scope.clone(),
                         name.as_str().into(),
-                        &module_output.ident_ids,
                         parsed_defs,
                     );
 
@@ -3915,7 +3914,7 @@ fn canonicalize_and_constrain<'a>(
                 var_store,
                 constraints,
                 constraint,
-                ident_ids: module_output.ident_ids,
+                ident_ids: module_output.scope.ident_ids,
                 dep_idents,
                 module_timing,
             };