Use the original visibility of symbols in ScopeData

Summary:
When evaluating a module, we do some funkiness with scopes during compilation. We create a scope, run enter_module, then pull that data back out with `exit_module`, and feed that to the compiler.
As part of enter_module, we copy all of that module's bindings over, but ignore their visibility, and set it to Public. When we run the compiler on that incorrect data, we spit out a module with the wrong visibilities.

Correct this so that we maintain the original visibilities (also adding a quick helper on Module. Instead of fetching the names, then grabbing the visibility for each, return the names and visibilities)

Reviewed By: ndmitchell

Differential Revision: D30916481

fbshipit-source-id: f93b1964b119db830e4c3f31465a3e8c87cc9c4c

Author: Natalie Jameson

starlark/src/environment/modules.rs

@@ -322,7 +322,7 @@ impl Module {
 
     /// Set the value of a variable in the environment. Set its visibliity to
     /// "private" to ensure that it is not re-exported
-    fn set_private<'v>(&'v self, name: &str, value: Value<'v>) {
+    pub(crate) fn set_private<'v>(&'v self, name: &str, value: Value<'v>) {
         let slot = self.names.add_name_visibility(name, Visibility::Private);
         let slots = self.slots();
         slots.ensure_slot(slot);

starlark/src/environment/names.rs

@@ -102,6 +102,14 @@ impl MutableNames {
             .collect()
     }
 
+    pub(crate) fn all_names_and_visibilities(&self) -> IndexMap<String, Visibility> {
+        self.0
+            .borrow()
+            .iter()
+            .map(|(name, (_slot, vis))| (name.to_owned(), *vis))
+            .collect()
+    }
+
     pub fn freeze(self) -> FrozenNames {
         FrozenNames(self.0.into_inner())
     }

starlark/src/eval/compiler/scope.rs

@@ -173,10 +173,9 @@ impl<'a> Scope<'a> {
 
         let mut locals = IndexMap::new();
 
-        let existing_module_names = module.all_names();
-        for name in existing_module_names.keys() {
-            let (binding_id, _binding) =
-                scope_data.new_binding(Visibility::Public, AssignCount::AtMostOnce);
+        let existing_module_names_and_visibilites = module.all_names_and_visibilities();
+        for (name, vis) in existing_module_names_and_visibilites.iter() {
+            let (binding_id, _binding) = scope_data.new_binding(*vis, AssignCount::AtMostOnce);
             locals.insert(name.as_str(), binding_id);
         }
 

starlark/src/eval/tests/mod.rs

@@ -402,6 +402,49 @@ fn test_load_reexport() {
     );
 }
 
+#[test]
+fn test_module_visibility_preserved_by_evaluator() -> anyhow::Result<()> {
+    // Make sure that when we use a module in the evaluator, the entering / exiting the
+    // module with ScopeData preserves the visibility of symbols.
+
+    let globals = Globals::standard();
+
+    let import = Module::new();
+    import.set("a", Value::new_int(1));
+    import.set_private("b", Value::new_int(2));
+
+    let mut eval = Evaluator::new(&import, &globals);
+    let ast = AstModule::parse("prelude.bzl", "c = 3".to_owned(), &Dialect::Standard).unwrap();
+    // This mutates the original module named `import`
+    let _: Value = eval.eval_module(ast)?;
+    let frozen_import = import.freeze()?;
+
+    let m_uses_public = Module::new();
+    m_uses_public.import_public_symbols(&frozen_import);
+    let mut eval = Evaluator::new(&m_uses_public, &globals);
+    let ast = AstModule::parse("code.bzl", "d = a".to_owned(), &Dialect::Standard).unwrap();
+    let _: Value = eval.eval_module(ast)?;
+
+    let m_uses_private = Module::new();
+    m_uses_private.import_public_symbols(&frozen_import);
+    let mut eval = Evaluator::new(&m_uses_private, &globals);
+    let ast = AstModule::parse("code.bzl", "d = b".to_owned(), &Dialect::Standard).unwrap();
+    let err = eval
+        .eval_module(ast)
+        .expect_err("Evaluation should have failed using a private symbol");
+
+    let msg = err.to_string();
+    let expected_msg = "Variable `b` not found";
+    if !msg.contains(expected_msg) {
+        panic!(
+            "Expected `{}` to be in error message `{}`",
+            expected_msg, msg
+        );
+    }
+
+    Ok(())
+}
+
 #[test]
 fn test_load_did_you_mean() {
     let mut a = Assert::new();