Add 'did you mean' suggestion when accessing an undefined variable. -- PiperOrigin-RevId: 143373605 MOS_MIGRATED_REVID=143373605
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java index 11515ef..37fcdd2 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -14,7 +14,6 @@ package com.google.devtools.build.lib.syntax; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; @@ -723,11 +722,7 @@ eventHandler.handle(event); } - /** - * @return the (immutable) set of names of all variables defined in this - * Environment. Exposed for testing. - */ - @VisibleForTesting + /** @return the (immutable) set of names of all variables defined in this Environment. */ public Set<String> getVariableNames() { Set<String> vars = new HashSet<>(); if (lexicalFrame != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java index b7c2d59..30b9417 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -832,15 +832,13 @@ @Override void validate(ValidationEnvironment env) throws EvalException { - for (Argument.Passed arg : args) { - arg.getValue().validate(env); - } - if (obj != null) { obj.validate(env); - } else if (!env.hasSymbolInEnvironment(func.getName())) { - throw new EvalException(getLocation(), - String.format("function '%s' does not exist", func.getName())); + } else { + func.validate(env); + } + for (Argument.Passed arg : args) { + arg.getValue().validate(env); } }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java index f953b05..83335e2 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -17,10 +17,10 @@ import com.google.devtools.build.lib.syntax.compiler.DebugInfo; import com.google.devtools.build.lib.syntax.compiler.Variable.SkylarkVariable; import com.google.devtools.build.lib.syntax.compiler.VariableScope; - -import net.bytebuddy.implementation.bytecode.ByteCodeAppender; - +import com.google.devtools.build.lib.util.SpellChecker; +import java.util.Set; import javax.annotation.Nullable; +import net.bytebuddy.implementation.bytecode.ByteCodeAppender; // TODO(bazel-team): for extra performance: // (1) intern the strings, so we can use == to compare, and have .equals use the assumption. @@ -74,7 +74,7 @@ Object doEval(Environment env) throws EvalException { Object value = env.lookup(name); if (value == null) { - throw createInvalidIdentifierException(); + throw createInvalidIdentifierException(env.getVariableNames()); } return value; } @@ -87,14 +87,23 @@ @Override void validate(ValidationEnvironment env) throws EvalException { if (!env.hasSymbolInEnvironment(name)) { - throw createInvalidIdentifierException(); + throw createInvalidIdentifierException(env.getAllSymbols()); } } - private EvalException createInvalidIdentifierException() { - return name.equals("$error$") - ? new EvalException(getLocation(), "contains syntax error(s)", true) - : new EvalException(getLocation(), "name '" + name + "' is not defined"); + private EvalException createInvalidIdentifierException(Set<String> symbols) { + if (name.equals("$error$")) { + return new EvalException(getLocation(), "contains syntax error(s)", true); + } + + String suggestion = SpellChecker.suggest(name, symbols); + if (suggestion == null) { + suggestion = ""; + } else { + suggestion = " (did you mean '" + suggestion + "'?)"; + } + + return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java index 6aa5a7d..b0e766d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -105,6 +105,16 @@ || (parent != null && topLevel().variables.contains(varname)); } + /** Returns the set of all accessible symbols (both local and global) */ + public Set<String> getAllSymbols() { + Set<String> all = new HashSet<>(); + all.addAll(variables); + if (parent != null) { + all.addAll(parent.getAllSymbols()); + } + return all; + } + private ValidationEnvironment topLevel() { return Preconditions.checkNotNull(parent == null ? this : parent); }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java index cde97c1..ae6b8fb 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -518,7 +518,7 @@ new SkylarkTest() .testIfErrorContains( - "ERROR 1:1: function 'join' does not exist", "join(' ', [ 'a', 'b', 'c' ])"); + "ERROR 1:1: name 'join' is not defined", "join(' ', [ 'a', 'b', 'c' ])"); new BothModesTest().testStatement("' '.join([ 'a', 'b', 'c' ])", "a b c"); }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java index 8cd35f2..c527fd2 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -1204,7 +1204,7 @@ public void testLoadNotAtTopLevel() throws Exception { setFailFast(false); parseFileForSkylark("if 1: load(8)\n"); - assertContainsError("function 'load' does not exist"); + assertContainsError("name 'load' is not defined"); } @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index f6493ba..38874b8b 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -1044,6 +1044,15 @@ } @Test + public void testTypo() throws Exception { + new SkylarkTest() + .testIfErrorContains( + "name 'my_variable' is not defined (did you mean 'myVariable'?)", + "myVariable = 2", + "x = my_variable + 1"); + } + + @Test public void testNoneTrueFalseInSkylark() throws Exception { new SkylarkTest().setUp("a = None", "b = True",
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java index b420ffd..2e4ae53 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -103,7 +103,7 @@ @Test public void testFunctionDoesNotExist() { - checkError("function 'foo' does not exist", "def bar(): a = foo() + 'a'"); + checkError("name 'foo' is not defined", "def bar(): a = foo() + 'a'"); } @Test