Check invariant during Starlark options parsing
The particular implementation of `BuildSettingLoader` always loads targets synchronously without every returning `null`, so the return value of `parse` is expected to be `true`. Check this invariant to make these calls easier to understand.
Closes #19003.
PiperOrigin-RevId: 549942222
Change-Id: Ibf1c6f77ed16de995374dd99b9a9f11958690cbc
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index bc54730..4756e02 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -17,6 +17,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -418,9 +419,10 @@
return DetailedExitCode.success();
}
try {
- StarlarkOptionsParser.newStarlarkOptionsParser(
- new SkyframeExecutorTargetLoader(env), optionsParser)
- .parse();
+ Preconditions.checkState(
+ StarlarkOptionsParser.newStarlarkOptionsParser(
+ new SkyframeExecutorTargetLoader(env), optionsParser)
+ .parse());
} catch (OptionsParsingException e) {
String logMessage = "Error parsing Starlark options";
logger.atInfo().withCause(e).log("%s", logMessage);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index a8ba820..c86b4a3 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -34,7 +34,6 @@
import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
-import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -103,7 +102,6 @@
// OptionsParserImpl.identifyOptionAndPossibleArgument. Consider combining. This would probably
// require multiple rounds of parsing to fit starlark-defined options into native option format.
@VisibleForTesting
- @CanIgnoreReturnValue
public boolean parse() throws OptionsParsingException {
return parseGivenArgs(nativeOptionsParser.getSkippedArgs());
}
@@ -115,7 +113,6 @@
* work to retrieve build setting targets (after which it'll call this method again)
*/
@VisibleForTesting
- @CanIgnoreReturnValue
public boolean parseGivenArgs(List<String> args) throws OptionsParsingException {
// Map of <option name (label), <unparsed option value, loaded option>>.
Multimap<String, Pair<String, Target>> unparsedOptions = LinkedListMultimap.create();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
index 92aaa1b..ea67bf0 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
@@ -16,6 +16,7 @@
import static com.google.devtools.common.options.Converters.BLAZE_ALIASING_FLAG;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.events.Event;
@@ -200,7 +201,7 @@
StarlarkOptionsParser.newStarlarkOptionsParser(
new SkyframeExecutorTargetLoader(env), parser);
try {
- starlarkOptionsParser.parse();
+ Preconditions.checkState(starlarkOptionsParser.parse());
} catch (OptionsParsingException e) {
return reportAndCreateCommandFailure(
env, e.getMessage(), FailureDetails.Command.Code.STARLARK_OPTIONS_PARSE_FAILURE);
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java b/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
index b8f30db..3e3eb6a2 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.starlark.util;
+import static com.google.common.truth.Truth.assertThat;
+
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -76,7 +78,7 @@
if (!onlyStarlarkParser) {
optionsParser.parse(asList);
}
- starlarkOptionsParser.parseGivenArgs(asList);
+ assertThat(starlarkOptionsParser.parseGivenArgs(asList)).isTrue();
return starlarkOptionsParser.getNativeOptionsParserFortesting();
}
@@ -86,11 +88,13 @@
List<String> bazelrcOptionsList = Arrays.asList(bazelrcOptions.split(" "));
optionsParser.parse(PriorityCategory.COMMAND_LINE, /* source= */ null, commandLineOptionsList);
optionsParser.parse(PriorityCategory.RC_FILE, "fake.bazelrc", bazelrcOptionsList);
- starlarkOptionsParser.parseGivenArgs(
- ImmutableList.<String>builder()
- .addAll(commandLineOptionsList)
- .addAll(bazelrcOptionsList)
- .build());
+ assertThat(
+ starlarkOptionsParser.parseGivenArgs(
+ ImmutableList.<String>builder()
+ .addAll(commandLineOptionsList)
+ .addAll(bazelrcOptionsList)
+ .build()))
+ .isTrue();
return starlarkOptionsParser.getNativeOptionsParserFortesting();
}
