Make deterministic the error that is stored in TransitiveTraversalValue. When there are multiple errors, we don't want non-determinism. PiperOrigin-RevId: 240208756
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java index 3d56f5b..5184e8e 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -892,9 +892,9 @@ for (Map.Entry<SkyKey, SkyValue> successfulEntry : successfulEntries) { successfulKeysBuilder.add(successfulEntry.getKey()); TransitiveTraversalValue value = (TransitiveTraversalValue) successfulEntry.getValue(); - String firstErrorMessage = value.getFirstErrorMessage(); - if (firstErrorMessage != null) { - errorMessagesBuilder.add(firstErrorMessage); + String errorMessage = value.getErrorMessage(); + if (errorMessage != null) { + errorMessagesBuilder.add(errorMessage); } } ImmutableSet<SkyKey> successfulKeys = successfulKeysBuilder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java index 3f72cf9..b7d8b43 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -21,13 +21,13 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.NoSuchThingException; -import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.FirstErrorMessageAccumulator; import com.google.devtools.build.lib.util.GroupedList; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException2; import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -35,12 +35,13 @@ /** * This class is like {@link TransitiveTargetFunction}, but the values it returns do not contain * {@link NestedSet}s. It performs the side-effects of {@link TransitiveTargetFunction} (i.e., - * ensuring that transitive targets and their packages have been loaded). It evaluates to a - * {@link TransitiveTraversalValue} that contains the first error message it encountered, and a - * set of names of providers if the target is a rule. + * ensuring that transitive targets and their packages have been loaded). It evaluates to a {@link + * TransitiveTraversalValue} that contains the first error message it encountered, and a set of + * names of providers if the target is a rule. */ public class TransitiveTraversalFunction - extends TransitiveBaseTraversalFunction<FirstErrorMessageAccumulator> { + extends TransitiveBaseTraversalFunction< + TransitiveTraversalFunction.DeterministicErrorMessageAccumulator> { @Override Label argumentFromKey(SkyKey key) { @@ -53,15 +54,16 @@ } @Override - FirstErrorMessageAccumulator processTarget(Label label, TargetAndErrorIfAny targetAndErrorIfAny) { + DeterministicErrorMessageAccumulator processTarget( + Label label, TargetAndErrorIfAny targetAndErrorIfAny) { NoSuchTargetException errorIfAny = targetAndErrorIfAny.getErrorLoadingTarget(); String errorMessageIfAny = errorIfAny == null ? null : errorIfAny.getMessage(); - return new FirstErrorMessageAccumulator(errorMessageIfAny); + return DeterministicErrorMessageAccumulator.create(errorMessageIfAny); } @Override void processDeps( - FirstErrorMessageAccumulator accumulator, + DeterministicErrorMessageAccumulator accumulator, EventHandler eventHandler, TargetAndErrorIfAny targetAndErrorIfAny, Iterable<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>> @@ -78,9 +80,9 @@ accumulator.maybeSet(e.getMessage()); continue; } - String firstErrorMessage = transitiveTraversalValue.getFirstErrorMessage(); - if (firstErrorMessage != null) { - accumulator.maybeSet(firstErrorMessage); + String errorMessage = transitiveTraversalValue.getErrorMessage(); + if (errorMessage != null) { + accumulator.maybeSet(errorMessage); } } } @@ -104,14 +106,14 @@ } @Override - SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny, - FirstErrorMessageAccumulator accumulator) { + SkyValue computeSkyValue( + TargetAndErrorIfAny targetAndErrorIfAny, DeterministicErrorMessageAccumulator accumulator) { boolean targetLoadedSuccessfully = targetAndErrorIfAny.getErrorLoadingTarget() == null; - String firstErrorMessage = accumulator.getFirstErrorMessage(); + String errorMessage = accumulator.getErrorMessage(); return targetLoadedSuccessfully - ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), firstErrorMessage) + ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), errorMessage) : TransitiveTraversalValue.unsuccessfulTransitiveTraversal( - firstErrorMessage, targetAndErrorIfAny.getTarget()); + errorMessage, targetAndErrorIfAny.getTarget()); } @Override @@ -178,28 +180,47 @@ } /** - * Keeps track of the first error message encountered while traversing itself and its - * dependencies. + * Keeps track of a deterministic error message encountered while traversing itself and its + * dependencies: either the error it was initialized with, or the shortest error it encounters, + * with ties broken alphabetically. + * + * <p>This preserves the behavior that the local target's error is the most important, and is + * cheap (constant-time) to compute the comparison between strings, unless they have the same + * length, which is unlikely. */ - static class FirstErrorMessageAccumulator { - - @Nullable private String firstErrorMessage; - - public FirstErrorMessageAccumulator(@Nullable String firstErrorMessage) { - this.firstErrorMessage = firstErrorMessage; - } - - /** Remembers {@param errorMessage} if it is the first error message. */ - void maybeSet(String errorMessage) { - Preconditions.checkNotNull(errorMessage); - if (firstErrorMessage == null) { - firstErrorMessage = errorMessage; - } - } - + interface DeterministicErrorMessageAccumulator { @Nullable - String getFirstErrorMessage() { - return firstErrorMessage; + String getErrorMessage(); + + default void maybeSet(String errorMessage) {} + + static DeterministicErrorMessageAccumulator create(@Nullable String errorMessage) { + if (errorMessage != null) { + return () -> errorMessage; + } + return new UpdateableErrorMessageAccumulator(); + } + + class UpdateableErrorMessageAccumulator implements DeterministicErrorMessageAccumulator { + private static final Comparator<String> LENGTH_THEN_ALPHABETICAL = + Comparator.nullsLast( + Comparator.comparingInt(String::length).thenComparing(Comparator.naturalOrder())); + + @Nullable private String errorMessage; + + @Override + public void maybeSet(String errorMessage) { + Preconditions.checkNotNull(errorMessage); + if (LENGTH_THEN_ALPHABETICAL.compare(this.errorMessage, errorMessage) > 0) { + this.errorMessage = errorMessage; + } + } + + @Nullable + @Override + public String getErrorMessage() { + return errorMessage; + } } } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java index 655cadd..be9c828 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
@@ -62,20 +62,18 @@ } static TransitiveTraversalValue unsuccessfulTransitiveTraversal( - String firstErrorMessage, Target target) { + String errorMessage, Target target) { return new TransitiveTraversalValueWithError( - Preconditions.checkNotNull(firstErrorMessage), target.getTargetKind()); + Preconditions.checkNotNull(errorMessage), target.getTargetKind()); } - static TransitiveTraversalValue forTarget(Target target, @Nullable String firstErrorMessage) { - if (firstErrorMessage == null) { + static TransitiveTraversalValue forTarget(Target target, @Nullable String errorMessage) { + if (errorMessage == null) { if (target instanceof Rule && ((Rule) target).getRuleClassObject().isSkylark()) { Rule rule = (Rule) target; // Do not intern values for skylark rules. return TransitiveTraversalValue.create( - rule.getRuleClassObject().getAdvertisedProviders(), - rule.getTargetKind(), - firstErrorMessage); + rule.getRuleClassObject().getAdvertisedProviders(), rule.getTargetKind(), errorMessage); } else { TransitiveTraversalValue value = VALUES_BY_TARGET_KIND.get(target.getTargetKind()); if (value != null) { @@ -95,18 +93,18 @@ return value; } } else { - return new TransitiveTraversalValueWithError(firstErrorMessage, target.getTargetKind()); + return new TransitiveTraversalValueWithError(errorMessage, target.getTargetKind()); } } @AutoCodec.Instantiator public static TransitiveTraversalValue create( - AdvertisedProviderSet providers, String kind, @Nullable String firstErrorMessage) { + AdvertisedProviderSet providers, String kind, @Nullable String errorMessage) { TransitiveTraversalValue value = - firstErrorMessage == null + errorMessage == null ? new TransitiveTraversalValueWithoutError(providers, kind) - : new TransitiveTraversalValueWithError(firstErrorMessage, kind); - if (firstErrorMessage == null) { + : new TransitiveTraversalValueWithError(errorMessage, kind); + if (errorMessage == null) { TransitiveTraversalValue oldValue = VALUE_INTERNER.getCanonical(value); return oldValue == null ? value : oldValue; } @@ -128,11 +126,11 @@ } /** - * Returns the first error message, if any, from loading the target and its transitive + * Returns a deterministic error message, if any, from loading the target and its transitive * dependencies. */ @Nullable - public abstract String getFirstErrorMessage(); + public abstract String getErrorMessage(); @Override public boolean equals(Object o) { @@ -143,14 +141,14 @@ return false; } TransitiveTraversalValue that = (TransitiveTraversalValue) o; - return Objects.equals(this.getFirstErrorMessage(), that.getFirstErrorMessage()) + return Objects.equals(this.getErrorMessage(), that.getErrorMessage()) && Objects.equals(this.getKind(), that.getKind()) && this.getProviders().equals(that.getProviders()); } @Override public int hashCode() { - return Objects.hash(getFirstErrorMessage(), getKind(), getProviders()); + return Objects.hash(getErrorMessage(), getKind(), getProviders()); } @ThreadSafe @@ -181,7 +179,7 @@ @Override @Nullable - public String getFirstErrorMessage() { + public String getErrorMessage() { return null; } @@ -196,12 +194,11 @@ /** A transitive target reference with error. */ public static final class TransitiveTraversalValueWithError extends TransitiveTraversalValue { - private final String firstErrorMessage; + private final String errorMessage; - private TransitiveTraversalValueWithError(String firstErrorMessage, String kind) { + private TransitiveTraversalValueWithError(String errorMessage, String kind) { super(kind); - this.firstErrorMessage = - StringCanonicalizer.intern(Preconditions.checkNotNull(firstErrorMessage)); + this.errorMessage = StringCanonicalizer.intern(Preconditions.checkNotNull(errorMessage)); } @Override @@ -216,14 +213,14 @@ @Override @Nullable - public String getFirstErrorMessage() { - return firstErrorMessage; + public String getErrorMessage() { + return errorMessage; } @Override public String toString() { return MoreObjects.toStringHelper(this) - .add("error", firstErrorMessage) + .add("error", errorMessage) .add("kind", getKind()) .toString(); }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java b/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java index 7c8bdc3..4351c56 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java +++ b/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java
@@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.skyframe; +import com.google.common.annotations.VisibleForTesting; import javax.annotation.Nullable; /** Wrapper for a value or the typed exception thrown when trying to compute it. */ @@ -23,7 +24,8 @@ @Nullable public abstract SkyValue get() throws E1, E2; - static <E1 extends Exception, E2 extends Exception> + @VisibleForTesting + public static <E1 extends Exception, E2 extends Exception> ValueOrException2<E1, E2> fromUntypedException( ValueOrUntypedException voe, Class<E1> exceptionClass1, Class<E2> exceptionClass2) { SkyValue value = voe.getValue();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java index 0fa3e05..9ca9b5d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
@@ -31,6 +31,8 @@ import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.ValueOrException2; +import com.google.devtools.build.skyframe.ValueOrUntypedException; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Test; import org.junit.runner.RunWith; @@ -98,6 +100,101 @@ assertThat(wasOptimizationUsed.get()).isTrue(); } + @Test + public void multipleErrorsForTransitiveTraversalFunction() throws Exception { + Label label = Label.parseAbsolute("//foo:foo", ImmutableMap.of()); + Package pkg = + scratchPackage( + "workspace", + label.getPackageIdentifier(), + "sh_library(name = '" + label.getName() + "', deps = [':bar', ':baz'])"); + TargetAndErrorIfAnyImpl targetAndErrorIfAny = + new TargetAndErrorIfAnyImpl( + /*packageLoadedSuccessfully=*/ true, + /*errorLoadingTarget=*/ null, + pkg.getTarget(label.getName())); + TransitiveTraversalFunction function = + new TransitiveTraversalFunction() { + @Override + LoadTargetResults loadTarget(Environment env, Label label) { + return targetAndErrorIfAny; + } + }; + SkyKey dep1 = function.getKey(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); + SkyKey dep2 = function.getKey(Label.parseAbsolute("//foo:baz", ImmutableMap.of())); + ImmutableMap<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> + returnedDeps = + ImmutableMap.of(dep1, makeException("bad bar"), dep2, makeException("bad baz")); + SkyFunction.Environment mockEnv = Mockito.mock(SkyFunction.Environment.class); + // Try two evaluations, with the environment reversing the order of the map it returns. + when(mockEnv.getValuesOrThrow( + Mockito.any(), + Mockito.eq(NoSuchPackageException.class), + Mockito.eq(NoSuchTargetException.class))) + .thenReturn(returnedDeps); + when(mockEnv.valuesMissing()).thenReturn(false); + + assertThat( + ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv)) + .getErrorMessage()) + .isEqualTo("bad bar"); + ImmutableMap<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> + reversedDeps = + ImmutableMap.of(dep2, makeException("bad baz"), dep1, makeException("bad bar")); + when(mockEnv.getValuesOrThrow( + Mockito.any(), + Mockito.eq(NoSuchPackageException.class), + Mockito.eq(NoSuchTargetException.class))) + .thenReturn(reversedDeps); + assertThat( + ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv)) + .getErrorMessage()) + .isEqualTo("bad bar"); + } + + @Test + public void selfErrorWins() throws Exception { + Label label = Label.parseAbsolute("//foo:foo", ImmutableMap.of()); + Package pkg = + scratchPackage( + "workspace", + label.getPackageIdentifier(), + "sh_library(name = '" + label.getName() + "', deps = [':bar', ':baz'])"); + TargetAndErrorIfAnyImpl targetAndErrorIfAny = + new TargetAndErrorIfAnyImpl( + /*packageLoadedSuccessfully=*/ true, + /*errorLoadingTarget=*/ new NoSuchTargetException("self error is long and last"), + pkg.getTarget(label.getName())); + TransitiveTraversalFunction function = + new TransitiveTraversalFunction() { + @Override + LoadTargetResults loadTarget(Environment env, Label label) { + return targetAndErrorIfAny; + } + }; + SkyKey dep = function.getKey(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); + SkyFunction.Environment mockEnv = Mockito.mock(SkyFunction.Environment.class); + when(mockEnv.getValuesOrThrow( + Mockito.any(), + Mockito.eq(NoSuchPackageException.class), + Mockito.eq(NoSuchTargetException.class))) + .thenReturn(ImmutableMap.of(dep, makeException("bad bar"))); + when(mockEnv.valuesMissing()).thenReturn(false); + + assertThat( + ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv)) + .getErrorMessage()) + .isEqualTo("self error is long and last"); + } + + private static ValueOrException2<NoSuchPackageException, NoSuchTargetException> makeException( + String errorMessage) { + ValueOrUntypedException exn = + ValueOrUntypedException.ofExn(new NoSuchTargetException(errorMessage)); + return ValueOrException2.fromUntypedException( + exn, NoSuchPackageException.class, NoSuchTargetException.class); + } + private Package scratchPackage(String workspaceName, PackageIdentifier packageId, String... lines) throws Exception { Path buildFile = scratch.file("" + packageId.getSourceRoot() + "/BUILD", lines);