Add new exception for wrapping parser construction failures
The exception is unchecked. The reasoning is that errors during parser construction should not occur, and when they do occur it is an internal error like a failed assertion.
This allows casual uses of the options parser to stay oblivious to the possibility of failures, consistent with how DuplicateOptionDeclarationException is currently [not] handled. At the same time, the dispatcher can catch the exception to fail gracefully (by printing to stdout instead of a log file) when parser construction fails for any reason.
RELNOTES: None
PiperOrigin-RevId: 151839620
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommand.java
index 6219a64..6be7ae1 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommand.java
@@ -24,28 +24,23 @@
*/
public interface BlazeCommand {
/**
- * This method provides the imperative portion of the command. It takes
- * a {@link OptionsProvider} instance {@code options}, which provides access
- * to the options instances via {@link OptionsProvider#getOptions(Class)},
- * and access to the residue (the remainder of the command line) via
- * {@link OptionsProvider#getResidue()}. The framework parses and makes
- * available exactly the options that the command class specifies via the
- * annotation {@link Command#options()}. The command may write to standard
- * out and standard error via {@code outErr}. It indicates success / failure
- * via its return value, which becomes the Unix exit status of the Blaze
- * client process. It may indicate a shutdown request by throwing
- * {@link BlazeCommandDispatcher.ShutdownBlazeServerException}. In that case,
- * the Blaze server process (the memory resident portion of Blaze) will
- * shut down and the exit status will be 0 (in case the shutdown succeeds
- * without error).
+ * This method provides the imperative portion of the command. It takes a {@link OptionsProvider}
+ * instance {@code options}, which provides access to the options instances via {@link
+ * OptionsProvider#getOptions(Class)}, and access to the residue (the remainder of the command
+ * line) via {@link OptionsProvider#getResidue()}. The framework parses and makes available
+ * exactly the options that the command class specifies via the annotation {@link
+ * Command#options()}. The command indicates success / failure via its return value, which becomes
+ * the Unix exit status of the Blaze client process. It may indicate a shutdown request by
+ * throwing {@link BlazeCommandDispatcher.ShutdownBlazeServerException}. In that case, the Blaze
+ * server process (the memory resident portion of Blaze) will shut down and the exit status will
+ * be 0 (in case the shutdown succeeds without error).
*
* @param env The environment for the current command invocation
- * @param options A parsed options instance initialized with the values for
- * the options specified in {@link Command#options()}.
- *
+ * @param options A parsed options instance initialized with the values for the options specified
+ * in {@link Command#options()}.
* @return The Unix exit status for the Blaze client.
- * @throws BlazeCommandDispatcher.ShutdownBlazeServerException Indicates
- * that the command wants to shutdown the Blaze server.
+ * @throws BlazeCommandDispatcher.ShutdownBlazeServerException Indicates that the command wants to
+ * shutdown the Blaze server.
*/
ExitCode exec(CommandEnvironment env, OptionsProvider options)
throws BlazeCommandDispatcher.ShutdownBlazeServerException;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 0dc21dc..7d275a4 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -17,6 +17,7 @@
import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.base.Predicates;
+import com.google.common.base.Throwables;
import com.google.common.base.Verify;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
@@ -27,6 +28,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.io.Flushables;
+import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Reporter;
@@ -380,6 +382,11 @@
List<String> rcfileNotes = new ArrayList<>();
try {
optionsParser = createOptionsParser(command);
+ } catch (OptionsParser.ConstructionException e) {
+ outErr.printErrLn("Internal error while constructing options parser: " + e.getMessage());
+ return ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode();
+ }
+ try {
parseArgsAndConfigs(env, optionsParser, commandAnnotation, args, rcfileNotes, outErr);
// Merge the invocation policy that is user-supplied, from the command line, and any
@@ -690,19 +697,23 @@
}
/**
- * Creates an option parser using the common options classes and the
- * command-specific options classes.
+ * Creates an option parser using the common options classes and the command-specific options
+ * classes.
*
- * <p>An overriding method should first call this method and can then
- * override default values directly or by calling {@link
- * #parseOptionsForCommand} for command-specific options.
- *
- * @throws OptionsParsingException
+ * <p>An overriding method should first call this method and can then override default values
+ * directly or by calling {@link #parseOptionsForCommand} for command-specific options.
*/
protected OptionsParser createOptionsParser(BlazeCommand command)
- throws OptionsParsingException {
+ throws OptionsParser.ConstructionException {
+ OpaqueOptionsData optionsData = null;
+ try {
+ optionsData = optionsDataCache.getUnchecked(command);
+ } catch (UncheckedExecutionException e) {
+ Throwables.throwIfInstanceOf(e.getCause(), OptionsParser.ConstructionException.class);
+ throw new IllegalStateException(e);
+ }
Command annotation = command.getClass().getAnnotation(Command.class);
- OptionsParser parser = OptionsParser.newOptionsParser(optionsDataCache.getUnchecked(command));
+ OptionsParser parser = OptionsParser.newOptionsParser(optionsData);
parser.setAllowResidue(annotation.allowResidue());
return parser;
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index a16f0e7..1e475e5 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -63,6 +63,26 @@
public class OptionsParser implements OptionsProvider {
/**
+ * An unchecked exception thrown when there is a problem constructing a parser, e.g. an error
+ * while validating an {@link Option} field in one of its {@link OptionsBase} subclasses.
+ *
+ * <p>Although unchecked, we explicitly mark some methods as throwing it as a reminder in the API.
+ */
+ public static class ConstructionException extends RuntimeException {
+ public ConstructionException(String message) {
+ super(message);
+ }
+
+ public ConstructionException(Throwable cause) {
+ super(cause);
+ }
+
+ public ConstructionException(String message, Throwable cause) {
+ super(message, cause);
+ }
+ }
+
+ /**
* A cache for the parsed options data. Both keys and values are immutable, so
* this is always safe. Only access this field through the {@link
* #getOptionsData} method for thread-safety! The cache is very unlikely to
@@ -73,23 +93,27 @@
Maps.newHashMap();
/**
- * Returns {@link OpaqueOptionsData} suitable for passing along to
- * {@link #newOptionsParser(OpaqueOptionsData optionsData)}.
+ * Returns {@link OpaqueOptionsData} suitable for passing along to {@link
+ * #newOptionsParser(OpaqueOptionsData optionsData)}.
*
- * This is useful when you want to do the work of analyzing the given {@code optionsClasses}
+ * <p>This is useful when you want to do the work of analyzing the given {@code optionsClasses}
* exactly once, but you want to parse lots of different lists of strings (and thus need to
- * construct lots of different {@link OptionsParser} instances).
+ * construct lots of different {@link OptionsParser} instances).
*/
public static OpaqueOptionsData getOptionsData(
- ImmutableList<Class<? extends OptionsBase>> optionsClasses) {
+ ImmutableList<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
return getOptionsDataInternal(optionsClasses);
}
private static synchronized OptionsData getOptionsDataInternal(
- ImmutableList<Class<? extends OptionsBase>> optionsClasses) {
+ ImmutableList<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
OptionsData result = optionsData.get(optionsClasses);
if (result == null) {
- result = OptionsData.from(optionsClasses);
+ try {
+ result = OptionsData.from(optionsClasses);
+ } catch (Exception e) {
+ throw new ConstructionException(e.getMessage(), e);
+ }
optionsData.put(optionsClasses, result);
}
return result;
@@ -108,7 +132,8 @@
/**
* @see #newOptionsParser(Iterable)
*/
- public static OptionsParser newOptionsParser(Class<? extends OptionsBase> class1) {
+ public static OptionsParser newOptionsParser(Class<? extends OptionsBase> class1)
+ throws ConstructionException {
return newOptionsParser(ImmutableList.<Class<? extends OptionsBase>>of(class1));
}
@@ -116,15 +141,15 @@
* @see #newOptionsParser(Iterable)
*/
public static OptionsParser newOptionsParser(Class<? extends OptionsBase> class1,
- Class<? extends OptionsBase> class2) {
+ Class<? extends OptionsBase> class2)
+ throws ConstructionException {
return newOptionsParser(ImmutableList.of(class1, class2));
}
- /**
- * Create a new {@link OptionsParser}.
- */
+ /** Create a new {@link OptionsParser}. */
public static OptionsParser newOptionsParser(
- Iterable<? extends Class<? extends OptionsBase>> optionsClasses) {
+ Iterable<? extends Class<? extends OptionsBase>> optionsClasses)
+ throws ConstructionException {
return newOptionsParser(
getOptionsDataInternal(ImmutableList.<Class<? extends OptionsBase>>copyOf(optionsClasses)));
}
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
index 69b4c96..51e3416 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -50,6 +50,18 @@
@RunWith(JUnit4.class)
public class OptionsParserTest {
+ /**
+ * Asserts that the given ConstructionException wraps an expected exception type with an expected
+ * message.
+ */
+ private static void assertConstructionErrorCausedBy(
+ OptionsParser.ConstructionException e,
+ Class<? extends Throwable> expectedType,
+ String expectedMessage) {
+ assertThat(e.getCause()).isInstanceOf(expectedType);
+ assertThat(e.getCause().getMessage()).contains(expectedMessage);
+ }
+
public static class ExampleFoo extends OptionsBase {
@Option(name = "foo",
@@ -891,8 +903,8 @@
try {
OptionsParser.newOptionsParser(NullExpansionsOptions.class);
fail("Should have failed due to null expansion function result");
- } catch (NullPointerException e) {
- assertThat(e.getMessage()).contains("null value in entry");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(e, NullPointerException.class, "null value in entry");
}
}
@@ -1577,9 +1589,11 @@
try {
newOptionsParser(ExampleNameConflictOptions.class);
fail("foo should conflict with the previous flag foo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--foo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to option: --foo");
}
}
@@ -1593,9 +1607,11 @@
try {
newOptionsParser(ExampleFoo.class, ExampleBooleanFooOptions.class);
fail("foo should conflict with the previous flag foo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--foo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to option: --foo");
}
}
@@ -1612,18 +1628,23 @@
newOptionsParser(ExampleBooleanFooOptions.class, ExamplePrefixFooOptions.class);
fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ "can be written as --nofoo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--nofoo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to option --nofoo, it conflicts with a negating alias "
+ + "for boolean flag --foo");
}
try {
newOptionsParser(ExamplePrefixFooOptions.class, ExampleBooleanFooOptions.class);
fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ "can be written as --nofoo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--nofoo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to boolean option alias: --nofoo");
}
}
@@ -1640,18 +1661,23 @@
newOptionsParser(ExampleBooleanFooOptions.class, ExampleUnderscorePrefixFooOptions.class);
fail("no_foo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ "can be written as --no_foo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--no_foo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to option --no_foo, it conflicts with a negating "
+ + "alias for boolean flag --foo");
}
try {
newOptionsParser(ExampleUnderscorePrefixFooOptions.class, ExampleBooleanFooOptions.class);
fail("no_foo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ "can be written as --no_foo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--no_foo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to boolean option alias: --no_foo");
}
}
@@ -1668,9 +1694,11 @@
newOptionsParser(ExamplePrefixFooOptions.class, ExampleBarWasNamedFooOption.class);
fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ "can be written as --nofoo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--nofoo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to boolean option alias: --nofoo");
}
}
@@ -1688,9 +1716,12 @@
newOptionsParser(ExampleBooleanFooOptions.class, ExampleBarWasNamedNoFooOption.class);
fail("nofoo, the old name for bar, should conflict with the previous flag foo, since foo, "
+ "as a boolean flag, can be written as --nofoo");
- } catch (DuplicateOptionDeclarationException e) {
- // Expected, check that the error message gives useful information.
- assertThat(e.getMessage()).contains("--nofoo");
+ } catch (OptionsParser.ConstructionException e) {
+ assertConstructionErrorCausedBy(
+ e,
+ DuplicateOptionDeclarationException.class,
+ "Duplicate option name, due to old option name --nofoo, it conflicts with a negating "
+ + "alias for boolean flag --foo");
}
}
@@ -1710,8 +1741,8 @@
try {
newOptionsParser(OldNameConflictExample.class);
fail("old_name should conflict with the flag already named old_name");
- } catch (DuplicateOptionDeclarationException e) {
- // expected
+ } catch (OptionsParser.ConstructionException e) {
+ assertThat(e.getCause()).isInstanceOf(DuplicateOptionDeclarationException.class);
}
}
diff --git a/src/test/java/com/google/devtools/common/options/OptionsTest.java b/src/test/java/com/google/devtools/common/options/OptionsTest.java
index 21a8a06..bb8e5dc 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java
@@ -485,8 +485,9 @@
try {
Options.parse(K.class, NO_ARGS).getOptions();
fail();
- } catch (IllegalStateException e) {
- assertThat(e.getMessage())
+ } catch (OptionsParser.ConstructionException e) {
+ assertThat(e.getCause()).isInstanceOf(IllegalStateException.class);
+ assertThat(e.getCause().getMessage())
.isEqualTo(
"OptionsParsingException while retrieving default for "
+ "int1: 'null' is not an int");
