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..38874b8 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
