Option to run scope resolved without globals

Summary:
Following diff D48941531 implements unused loads removal.

To find unused loads, the easiest way is to run scope resolved.

Before this diff, scope resolver required globals (because it populates globals in AST). Now scope resolver can be run without globals, so every unknown name is resolved to some string. Which is sufficient to find unused globals.

Reviewed By: ianlevesque

Differential Revision: D48941533

fbshipit-source-id: 1657b352196196d9b2e31dd8b8a23ccf68fc3706

Author: stepancheg

starlark/src/eval/compiler/scope/mod.rs

@@ -16,6 +16,7 @@
  */
 
 pub(crate) mod payload;
+pub(crate) mod scope_resolver_globals;
 mod tests;
 
 use std::collections::HashMap;
@@ -50,7 +51,6 @@ use crate::codemap::CodeMap;
 use crate::codemap::Span;
 use crate::environment::names::MutableNames;
 use crate::environment::slots::ModuleSlotId;
-use crate::environment::Globals;
 use crate::environment::Module;
 use crate::errors::did_you_mean::did_you_mean;
 use crate::eval::compiler::def::CopySlotFromParent;
@@ -63,6 +63,7 @@ use crate::eval::compiler::scope::payload::CstPayload;
 use crate::eval::compiler::scope::payload::CstStmt;
 use crate::eval::compiler::scope::payload::CstStmtFromAst;
 use crate::eval::compiler::scope::payload::CstTypeExpr;
+use crate::eval::compiler::scope::scope_resolver_globals::ScopeResolverGlobals;
 use crate::eval::runtime::slots::LocalSlotIdCapturedOrNot;
 use crate::syntax::Dialect;
 use crate::typing::error::InternalError;
@@ -93,7 +94,7 @@ struct ModuleScopeBuilder<'a> {
     locals: Vec<ScopeId>,
     unscopes: Vec<Unscope>,
     codemap: FrozenRef<'static, CodeMap>,
-    globals: FrozenRef<'static, Globals>,
+    globals: ScopeResolverGlobals,
     errors: Vec<EvalException>,
     top_level_stmt_count: usize,
 }
@@ -265,7 +266,7 @@ impl<'f> ModuleScopeBuilder<'f> {
         frozen_heap: &'f FrozenHeap,
         loads: &HashMap<String, Interface>,
         stmt: AstStmt,
-        globals: FrozenRef<'static, Globals>,
+        globals: ScopeResolverGlobals,
         codemap: FrozenRef<'static, CodeMap>,
         dialect: &Dialect,
     ) -> (CstStmt, ModuleScopeBuilder<'f>) {
@@ -371,7 +372,7 @@ impl<'f> ModuleScopes<'f> {
         frozen_heap: &'f FrozenHeap,
         loads: &HashMap<String, Interface>,
         stmt: AstStmt,
-        globals: FrozenRef<'static, Globals>,
+        globals: ScopeResolverGlobals,
         codemap: FrozenRef<'static, CodeMap>,
         dialect: &Dialect,
     ) -> anyhow::Result<ModuleScopes<'f>> {
@@ -388,7 +389,7 @@ impl<'f> ModuleScopes<'f> {
         frozen_heap: &'f FrozenHeap,
         loads: &HashMap<String, Interface>,
         stmt: AstStmt,
-        globals: FrozenRef<'static, Globals>,
+        globals: ScopeResolverGlobals,
         codemap: FrozenRef<'static, CodeMap>,
         dialect: &Dialect,
     ) -> (Vec<EvalException>, ModuleScopes<'f>) {
@@ -635,20 +636,23 @@ impl<'f> ModuleScopeBuilder<'f> {
         );
     }
 
-    fn current_scope_all_visible_names_for_did_you_mean(&self) -> Vec<FrozenStringValue> {
+    fn current_scope_all_visible_names_for_did_you_mean(&self) -> Option<Vec<String>> {
         // It is OK to return non-unique identifiers
-        let mut r: Vec<FrozenStringValue> = Vec::new();
+        let mut r: Vec<String> = Vec::new();
         for &scope_id in self.locals.iter().rev() {
             let scope = self.scope_data.get_scope(scope_id);
-            r.extend(scope.mp.keys().copied());
+            r.extend(scope.mp.keys().map(|s| s.as_str().to_owned()));
         }
-        r.extend(self.module_bindings.keys().copied());
-        r.extend(self.globals.names());
-        r
+        r.extend(self.module_bindings.keys().map(|s| s.as_str().to_owned()));
+        r.extend(self.globals.names()?);
+        Some(r)
     }
 
+    #[cold]
     fn variable_not_found_err(&self, ident: &CstIdent) -> EvalException {
-        let variants = self.current_scope_all_visible_names_for_did_you_mean();
+        let variants = self
+            .current_scope_all_visible_names_for_did_you_mean()
+            .unwrap_or(Vec::new());
         let better = did_you_mean(
             ident.node.ident.as_str(),
             variants.iter().map(|s| s.as_str()),
@@ -672,7 +676,7 @@ impl<'f> ModuleScopeBuilder<'f> {
         let resolved = match self.get_name(self.frozen_heap.alloc_str_intern(&ident.node.ident)) {
             None => {
                 // Must be a global, since we know all variables
-                match self.globals.get_frozen(&ident.node.ident) {
+                match self.globals.get_global(&ident.node.ident) {
                     None => {
                         self.errors.push(self.variable_not_found_err(ident));
                         return;

starlark/src/eval/compiler/scope/scope_resolver_globals.rs

@@ -0,0 +1,40 @@
+/*
+ * Copyright 2019 The Starlark in Rust Authors.
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+use crate::const_frozen_string;
+use crate::environment::Globals;
+use crate::values::FrozenRef;
+use crate::values::FrozenValue;
+
+pub(crate) struct ScopeResolverGlobals {
+    /// None if unknown.
+    pub(crate) globals: Option<FrozenRef<'static, Globals>>,
+}
+
+impl ScopeResolverGlobals {
+    pub(crate) fn get_global(&self, name: &str) -> Option<FrozenValue> {
+        match self.globals {
+            Some(globals) => globals.get_frozen(name),
+            None => Some(const_frozen_string!("unknown-global").to_frozen_value()),
+        }
+    }
+
+    pub(crate) fn names(&self) -> Option<Vec<String>> {
+        self.globals
+            .map(|g| g.names().map(|s| s.as_str().to_owned()).collect())
+    }
+}

starlark/src/eval/compiler/scope/tests.rs

@@ -33,6 +33,7 @@ use crate::eval::compiler::scope::payload::CstAssignIdent;
 use crate::eval::compiler::scope::payload::CstAssignTarget;
 use crate::eval::compiler::scope::payload::CstExpr;
 use crate::eval::compiler::scope::payload::CstStmt;
+use crate::eval::compiler::scope::scope_resolver_globals::ScopeResolverGlobals;
 use crate::eval::compiler::scope::AssignCount;
 use crate::eval::compiler::scope::Captured;
 use crate::eval::compiler::scope::ModuleScopes;
@@ -54,7 +55,9 @@ fn test_with_module(program: &str, expected: &str, module: &MutableNames) {
         &frozen_heap,
         &HashMap::new(),
         ast.statement,
-        FrozenRef::new(Globals::empty()),
+        ScopeResolverGlobals {
+            globals: Some(FrozenRef::new(Globals::empty())),
+        },
         codemap,
         &Dialect::Extended,
     )

starlark/src/eval/mod.rs

@@ -46,6 +46,7 @@ use crate::collections::symbol_map::Symbol;
 use crate::docs::DocString;
 use crate::environment::Globals;
 use crate::eval::compiler::def::DefInfo;
+use crate::eval::compiler::scope::scope_resolver_globals::ScopeResolverGlobals;
 use crate::eval::compiler::scope::ModuleScopes;
 use crate::eval::compiler::scope::ScopeId;
 use crate::eval::compiler::Compiler;
@@ -91,7 +92,9 @@ impl<'v, 'a> Evaluator<'v, 'a> {
             self.module_env.frozen_heap(),
             &HashMap::new(),
             statement,
-            globals,
+            ScopeResolverGlobals {
+                globals: Some(globals),
+            },
             codemap,
             &dialect,
         )?;

starlark/src/typing/typecheck.rs

@@ -34,6 +34,7 @@ use crate::codemap::Spanned;
 use crate::environment::names::MutableNames;
 use crate::environment::Globals;
 use crate::eval::compiler::scope::payload::CstStmt;
+use crate::eval::compiler::scope::scope_resolver_globals::ScopeResolverGlobals;
 use crate::eval::compiler::scope::BindingId;
 use crate::eval::compiler::scope::BindingSource;
 use crate::eval::compiler::scope::ModuleScopes;
@@ -200,7 +201,9 @@ impl AstModuleTypecheck for AstModule {
             &frozen_heap,
             loads,
             self.statement,
-            frozen_heap.alloc_any_display_from_debug(globals.dupe()),
+            ScopeResolverGlobals {
+                globals: Some(frozen_heap.alloc_any_display_from_debug(globals.dupe())),
+            },
             frozen_heap.alloc_any_display_from_debug(self.codemap.dupe()),
             &Dialect::Extended,
         );