diff options
author | 2017-07-13 17:23:09 +0200 | |
---|---|---|
committer | 2017-07-14 10:51:40 +0200 | |
commit | e5d95fb510ae056c8650b5bb450ec5dc7c7eeb3b (patch) | |
tree | ed7e8978345c4172738a383ad614d7ad3bbb44b6 /src/main/java/com | |
parent | 528a1eac7d2638a492655cda8e18e80c4510583c (diff) |
Fix crash when unioning depsets with different orders
Also refactor FAIL_FAST_HANDLER to throw something more specific than IllegalArgumentException. This bug was masked because the test assertion that would've caught it considered IllegalArgumentException to be an expected error, the same as EvalException.
RELNOTES: None
PiperOrigin-RevId: 161809957
Diffstat (limited to 'src/main/java/com')
4 files changed, 24 insertions, 10 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java index 3eb1c062cf..f565442c22 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java @@ -496,7 +496,8 @@ public class SkylarkRuleImplementationFunctions { // Also, allow empty set for init @Param(name = "transitive_files", type = SkylarkNestedSet.class, generic1 = Artifact.class, noneable = true, defaultValue = "None", - doc = "The (transitive) set of files to be added to the runfiles."), + doc = "The (transitive) set of files to be added to the runfiles. The depset should " + + "use the `default` order (which, as the name implies, is the default)."), @Param(name = "collect_data", type = Boolean.class, defaultValue = "False", doc = "Whether to collect the data " + "runfiles from the dependencies in srcs, data and deps attributes."), diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java index e4e4b01970..459858aad4 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java @@ -273,7 +273,7 @@ public final class BinaryOperatorExpression extends Expression { } } - // TODO(bazel-team): Remove this case. Union of sets should use '|' instead of '+'. + // TODO(bazel-team): Deprecate + and | on depsets. Needs new API design. if (lval instanceof SkylarkNestedSet) { return new SkylarkNestedSet((SkylarkNestedSet) lval, rval, location); } 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 9b17cead89..38cbb7c35f 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 @@ -940,16 +940,28 @@ public final class Environment implements Freezable { } } + /** An exception thrown by {@link #FAIL_FAST_HANDLER}. */ + // TODO(bazel-team): Possibly extend RuntimeException instead of IllegalArgumentException. + public static class FailFastException extends IllegalArgumentException { + public FailFastException(String s) { + super(s); + } + } /** - * The fail fast handler, which throws an {@link IllegalArgumentException} whenever an error or - * warning occurs. + * A handler that immediately throws {@link FailFastException} whenever an error or warning + * occurs. + * + * We do not reuse an existing unchecked exception type, because callers (e.g., test assertions) + * need to be able to distinguish between organically occurring exceptions and exceptions thrown + * by this handler. */ public static final EventHandler FAIL_FAST_HANDLER = new EventHandler() { - @Override - public void handle(Event event) { - Preconditions.checkArgument( - !EventKind.ERRORS_AND_WARNINGS.contains(event.getKind()), event); + @Override + public void handle(Event event) { + if (EventKind.ERRORS_AND_WARNINGS.contains(event.getKind())) { + throw new FailFastException(event.toString()); } - }; + } + }; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java index 12a01326da..c38fe89b6a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java @@ -159,7 +159,8 @@ public final class SkylarkNestedSet implements SkylarkValue, SkylarkQueryable { for (NestedSet<?> nestedSet : transitiveItems) { builder.addTransitive(nestedSet); } - } catch (IllegalStateException e) { + } catch (IllegalArgumentException e) { + // Order mismatch between item and builder. throw new EvalException(loc, e.getMessage()); } this.set = builder.build(); |