bazel syntax: rationalize the type check operators
This change introduces {Dict,Sequence}.{,noneable}Cast.
These four conversion operators cast an arbitrary value
to a Sequence<T> or Dict<K,V>, replacing these 9 previous functions:
- Dict.{castSkylarkDictOrNoneToDict,getContents}
- Sequence.{castList,castSkylarkListOrNoneToList,getContents}
- SkylarkType.{cast,cast',checkType,castMap}
The functions don't allocate an unmodifiable wrapper,
as Dict and List are already unmodifiable through the java.util
interfaces, so this is just wasteful allocation and indirection.
Also, there is no a priori allocation of Formattable error messages.
A number of messes (e.g. unsound casts) were cleaned up throughout.
The new operators do not accept a Location, and report error
using Starlark.errorf. (This whole CL started as a subtask of a
subtask to eliminate the Location parameter of EvalException,
and this sub-sub-task is an experiment to see whether removing
Location parameters that don't correspond to program counter
locations is a UI regression. SRCTU.createTarget is the only
place where this appears to be a problem. For now I've added a
small kludge, but in a follow-up I will change it to report
events, not throw exceptions, so that it can report multiple
errors at arbitrary locations.
Depset might benefit from a similar {noneable,}Cast treatment,
but this too is left for a follow-up.
This is a breaking API change for copybara.
RELNOTES: N/A
PiperOrigin-RevId: 306925434
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
index ad60ded..7d1e676 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -175,10 +175,10 @@
@Test
public void testRepoMappingNotAStringStringDict() throws Exception {
helper.parse("local_repository(name='foo', path='/foo', repo_mapping=1)");
- assertThat(helper.getParserError()).contains("got 'int' for 'repo_mapping', want 'dict'");
+ assertThat(helper.getParserError()).contains("got int for 'repo_mapping', want dict");
helper.parse("local_repository(name='foo', path='/foo', repo_mapping='hello')");
- assertThat(helper.getParserError()).contains("got 'string' for 'repo_mapping', want 'dict'");
+ assertThat(helper.getParserError()).contains("got string for 'repo_mapping', want dict");
helper.parse("local_repository(name='foo', path='/foo', repo_mapping={1: 1})");
assertThat(helper.getParserError())
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
index 43efec1..58464d6 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
@@ -2376,9 +2376,7 @@
EvalException e =
assertThrows(
EvalException.class, () -> CcModule.withFeatureSetFromSkylark(withFeatureSetProvider));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'features' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for features, got struct, want sequence");
}
@Test
@@ -2392,9 +2390,7 @@
EvalException e =
assertThrows(
EvalException.class, () -> CcModule.withFeatureSetFromSkylark(withFeatureSetProvider));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'not_features' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for not_features, got struct, want sequence");
}
@Test
@@ -2410,7 +2406,7 @@
EvalException.class, () -> CcModule.withFeatureSetFromSkylark(withFeatureSetProvider));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'features' element but got type 'struct' instead");
+ .contains("at index 0 of features, got element of type struct, want string");
}
@Test
@@ -2426,8 +2422,7 @@
EvalException.class, () -> CcModule.withFeatureSetFromSkylark(withFeatureSetProvider));
assertThat(e)
.hasMessageThat()
- .contains(
- "expected type 'string' for 'not_features' element but got type 'struct' instead");
+ .contains("at index 0 of not_features, got element of type struct, want string");
}
private void createCustomWithFeatureSetRule(String pkg, String features, String notFeatures)
@@ -2588,9 +2583,7 @@
assertThat(envSetProvider).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.envSetFromSkylark(envSetProvider));
- assertThat(e)
- .hasMessageThat()
- .contains("'env_entries' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for env_entries, got struct, want sequence");
}
@Test
@@ -2624,9 +2617,7 @@
assertThat(envSetProvider).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.envSetFromSkylark(envSetProvider));
- assertThat(e)
- .hasMessageThat()
- .contains("'with_features' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for with_features, got string, want sequence");
}
@Test
@@ -3170,9 +3161,7 @@
EvalException e = assertThrows(EvalException.class, () -> CcModule.toolFromSkylark(toolStruct));
assertThat(e)
.hasMessageThat()
- .contains(
- "expected type 'string' for 'execution_requirements' "
- + "element but got type 'struct' instead");
+ .contains("at index 0 of execution_requirements, got element of type struct, want string");
}
@Test
@@ -3245,9 +3234,7 @@
SkylarkInfo toolStruct = (SkylarkInfo) getMyInfoFromTarget(t).getValue("tool");
assertThat(toolStruct).isNotNull();
EvalException e = assertThrows(EvalException.class, () -> CcModule.toolFromSkylark(toolStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'with_features' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for with_features, got struct, want sequence");
}
@Test
@@ -3280,8 +3267,7 @@
EvalException e = assertThrows(EvalException.class, () -> CcModule.toolFromSkylark(toolStruct));
assertThat(e)
.hasMessageThat()
- .contains(
- "llegal argument: 'execution_requirements' is not of expected type list or NoneType");
+ .contains("for execution_requirements, got string, want sequence");
}
@Test
@@ -3296,9 +3282,7 @@
EvalException e = assertThrows(EvalException.class, () -> CcModule.toolFromSkylark(toolStruct));
assertThat(e)
.hasMessageThat()
- .contains(
- "expected type 'string' for 'execution_requirements' "
- + "element but got type 'struct' instead");
+ .contains("at index 0 of execution_requirements, got element of type struct, want string");
}
private void createCustomToolRule(
@@ -3359,7 +3343,7 @@
() -> CcModule.flagSetFromSkylark(flagSetStruct, /* actionName= */ null));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'actions' element but got type 'struct' instead");
+ .contains("at index 0 of actions, got element of type struct, want string");
}
@Test
@@ -3499,9 +3483,7 @@
assertThrows(
EvalException.class,
() -> CcModule.flagSetFromSkylark(flagSetStruct, /* actionName */ null));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'flag_groups' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for flag_groups, got struct, want sequence");
}
@Test
@@ -3517,9 +3499,7 @@
assertThrows(
EvalException.class,
() -> CcModule.flagSetFromSkylark(flagSetStruct, /* actionName */ null));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'with_features' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for with_features, got struct, want sequence");
}
@Test
@@ -3535,9 +3515,7 @@
assertThrows(
EvalException.class,
() -> CcModule.flagSetFromSkylark(flagSetStruct, /* actionName */ null));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'actions' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for actions, got struct, want sequence");
}
private void createCustomFlagSetRule(
@@ -3669,7 +3647,7 @@
EvalException.class, () -> CcModule.actionConfigFromSkylark(actionConfigStruct));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'implies' element but got type 'struct' instead");
+ .contains("at index 0 of implies, got element of type struct, want string");
}
@Test
@@ -3691,7 +3669,7 @@
EvalException.class, () -> CcModule.actionConfigFromSkylark(actionConfigStruct));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'implies' element but got type 'struct' instead");
+ .contains("at index 0 of implies, got element of type struct, want string");
}
@Test
@@ -3843,9 +3821,7 @@
EvalException e =
assertThrows(
EvalException.class, () -> CcModule.actionConfigFromSkylark(actionConfigStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'tools' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for tools, got struct, want sequence");
}
@Test
@@ -3865,9 +3841,7 @@
EvalException e =
assertThrows(
EvalException.class, () -> CcModule.actionConfigFromSkylark(actionConfigStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'flag_sets' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for flag_sets, got bool, want sequence");
}
@Test
@@ -3887,9 +3861,7 @@
EvalException e =
assertThrows(
EvalException.class, () -> CcModule.actionConfigFromSkylark(actionConfigStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'implies' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for implies, got struct, want sequence");
}
private void createCustomActionConfigRule(
@@ -4040,7 +4012,7 @@
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'implies' element but got type 'struct' instead");
+ .contains("at index 0 of implies, got element of type struct, want string");
}
@Test
@@ -4064,7 +4036,7 @@
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
assertThat(e)
.hasMessageThat()
- .contains("expected type 'string' for 'provides' element but got type 'struct' instead");
+ .contains("at index 0 of provides, got element of type struct, want string");
}
@Test
@@ -4226,9 +4198,7 @@
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'flag_sets' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for flag_sets, got struct, want sequence");
}
@Test
@@ -4249,9 +4219,7 @@
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'env_sets' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for env_sets, got struct, want sequence");
}
@Test
@@ -4272,9 +4240,7 @@
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'requires' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for requires, got struct, want sequence");
}
@Test
@@ -4295,9 +4261,7 @@
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'implies' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for implies, got struct, want sequence");
}
@Test
@@ -4318,9 +4282,7 @@
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromSkylark(featureStruct));
- assertThat(e)
- .hasMessageThat()
- .contains("Illegal argument: 'provides' is not of expected type list or NoneType");
+ assertThat(e).hasMessageThat().contains("for provides, got struct, want sequence");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
index 2fe94e3..b77aadf 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
@@ -91,8 +91,7 @@
ThrowingRunnable access, String fieldName, String expectedType) {
assertThrowsEvalExceptionContaining(
access,
- String.format(
- "\'py' provider's '%s' field should be a %s (got a 'int')", fieldName, expectedType));
+ String.format("\'py' provider's '%s' field was int, want %s", fieldName, expectedType));
}
/** We need this because {@code NestedSet}s don't have value equality. */
@@ -154,7 +153,7 @@
public void getUsesSharedLibraries_WrongType() {
StructImpl info = makeStruct(ImmutableMap.of(PyStructUtils.USES_SHARED_LIBRARIES, 123));
assertHasWrongTypeMessage(
- () -> PyStructUtils.getUsesSharedLibraries(info), "uses_shared_libraries", "boolean");
+ () -> PyStructUtils.getUsesSharedLibraries(info), "uses_shared_libraries", "bool");
}
@Test
@@ -191,7 +190,7 @@
public void getHasPy2OnlySources_WrongType() {
StructImpl info = makeStruct(ImmutableMap.of(PyStructUtils.HAS_PY2_ONLY_SOURCES, 123));
assertHasWrongTypeMessage(
- () -> PyStructUtils.getHasPy2OnlySources(info), "has_py2_only_sources", "boolean");
+ () -> PyStructUtils.getHasPy2OnlySources(info), "has_py2_only_sources", "bool");
}
@Test
@@ -209,7 +208,7 @@
public void getHasPy3OnlySources_WrongType() {
StructImpl info = makeStruct(ImmutableMap.of(PyStructUtils.HAS_PY3_ONLY_SOURCES, 123));
assertHasWrongTypeMessage(
- () -> PyStructUtils.getHasPy3OnlySources(info), "has_py3_only_sources", "boolean");
+ () -> PyStructUtils.getHasPy3OnlySources(info), "has_py3_only_sources", "bool");
}
/** Checks values set by the builder. */
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 11ef9a4..8a59f59 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -401,7 +401,7 @@
@Test
public void testLabelListWithAspectsError() throws Exception {
checkEvalErrorContains(
- "expected type 'Aspect' for 'aspects' element but got type 'int' instead",
+ "at index 0 of aspects, got element of type int, want Aspect",
"def _impl(target, ctx):",
" pass",
"my_aspect = aspect(implementation = _impl)",
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 65637e4..77bdd6d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -401,7 +401,7 @@
public void testCreateSpawnActionBadGenericArg() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected type 'File' for 'outputs' element but got type 'string' instead",
+ "at index 0 of outputs, got element of type string, want File",
"l = ['a', 'b']",
"ruleContext.actions.run_shell(",
" outputs = l,",
@@ -841,7 +841,7 @@
public void testRunfilesBadListGenericType() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected type 'File' for 'files' element but got type 'string' instead",
+ "at index 0 of files, got element of type string, want File",
"ruleContext.runfiles(files = ['some string'])");
}
@@ -857,16 +857,16 @@
public void testRunfilesBadMapGenericType() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected type 'string' for 'symlinks' key but got type 'int' instead",
+ "got dict<int, File> for 'symlinks', want dict<string, File>",
"ruleContext.runfiles(symlinks = {123: ruleContext.files.srcs[0]})");
checkEvalErrorContains(
- "expected type 'File' for 'symlinks' value but got type 'int' instead",
+ "got dict<string, int> for 'symlinks', want dict<string, File>",
"ruleContext.runfiles(symlinks = {'some string': 123})");
checkEvalErrorContains(
- "expected type 'string' for 'root_symlinks' key but got type 'int' instead",
+ "got dict<int, File> for 'root_symlinks', want dict<string, File>",
"ruleContext.runfiles(root_symlinks = {123: ruleContext.files.srcs[0]})");
checkEvalErrorContains(
- "expected type 'File' for 'root_symlinks' value but got type 'int' instead",
+ "got dict<string, int> for 'root_symlinks', want dict<string, File>",
"ruleContext.runfiles(root_symlinks = {'some string': 123})");
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
index f1592ca..431cbee 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
@@ -234,7 +234,7 @@
public void testItemsAndTransitive() throws Exception {
new Scenario()
.testIfExactError(
- "expected type 'sequence' for items but got type 'depset' instead",
+ "for items, got depset, want sequence",
"depset(items = depset(), transitive = [depset()])");
}
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 428a7b1..78aeeac 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
@@ -716,9 +716,7 @@
@Test
public void testDepsetDirectInvalidType() throws Exception {
new Scenario()
- .testIfErrorContains(
- "expected type 'sequence' for direct but got type 'string' instead",
- "depset(direct='hello')");
+ .testIfErrorContains("for direct, got string, want sequence", "depset(direct='hello')");
}
@Test
