From f188dc23bff5edbe4c7b8f43cc66a8f1d07ba07b Mon Sep 17 00:00:00 2001 From: Dmitry Lomov Date: Tue, 19 Jul 2016 09:00:55 +0000 Subject: Prohibit duplicate addition of aspect to an attribute and improve diagnostics. -- MOS_MIGRATED_REVID=127808009 --- .../devtools/build/lib/packages/Attribute.java | 44 +++++++++++++++------- .../devtools/build/lib/rules/SkylarkAttr.java | 2 +- .../build/lib/skylark/SkylarkAspectsTest.java | 29 ++++++++++++++ 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index c605964870..0655f9f6ec 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -25,6 +25,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.ClassObject; @@ -37,16 +38,14 @@ import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.StringUtil; - import java.util.Arrays; import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; -import java.util.LinkedHashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; - import javax.annotation.concurrent.Immutable; /** @@ -415,7 +414,6 @@ public final class Attribute implements Comparable { * already undocumented based on its name cannot be marked as undocumented. */ public static class Builder { - private String name; private final Type type; private Transition configTransition = ConfigurationTransition.NONE; @@ -435,7 +433,7 @@ public final class Attribute implements Comparable { ImmutableList.>of(); private ImmutableList> mandatoryNativeProviders = ImmutableList.of(); - private Set aspects = new LinkedHashSet<>(); + private HashMap> aspects = new LinkedHashMap<>(); /** * Creates an attribute builder with given name and type. This attribute is optional, uses @@ -890,7 +888,12 @@ public final class Attribute implements Comparable { */ public Builder aspect( NativeAspectClass aspect, Function evaluator) { - this.aspects.add(new NativeRuleAspect(aspect, evaluator)); + NativeRuleAspect nativeRuleAspect = new NativeRuleAspect(aspect, evaluator); + RuleAspect oldAspect = this.aspects.put(nativeRuleAspect.getName(), nativeRuleAspect); + if (oldAspect != null) { + throw new AssertionError( + String.format("Aspect %s has already been added", oldAspect.getName())); + } return this; } @@ -909,13 +912,25 @@ public final class Attribute implements Comparable { return this.aspect(aspect, noParameters); } - public Builder aspect(SkylarkAspect skylarkAspect) { - this.aspects.add(new SkylarkRuleAspect(skylarkAspect)); + public Builder aspect( + SkylarkAspect skylarkAspect, Location location) throws EvalException { + SkylarkRuleAspect skylarkRuleAspect = new SkylarkRuleAspect(skylarkAspect); + RuleAspect oldAspect = this.aspects.put(skylarkAspect.getName(), skylarkRuleAspect); + if (oldAspect != null) { + throw new EvalException(location, + String.format("Aspect %s added more than once", skylarkAspect.getName())); + } return this; } public Builder aspect(final Aspect aspect) { - this.aspects.add(new PredefinedRuleAspect(aspect)); + PredefinedRuleAspect predefinedRuleAspect = new PredefinedRuleAspect(aspect); + RuleAspect oldAspect = + this.aspects.put(predefinedRuleAspect.getName(), predefinedRuleAspect); + if (oldAspect != null) { + throw new AssertionError( + String.format("Aspect %s has already been added", oldAspect.getName())); + } return this; } @@ -1002,7 +1017,7 @@ public final class Attribute implements Comparable { allowedValues, mandatoryProvidersList, mandatoryNativeProviders, - ImmutableSet.copyOf(aspects)); + ImmutableList.copyOf(aspects.values())); } } @@ -1289,7 +1304,7 @@ public final class Attribute implements Comparable { private final ImmutableList> mandatoryNativeProviders; - private final ImmutableSet aspects; + private final ImmutableList> aspects; /** * Constructs a rule attribute with the specified name, type and default @@ -1322,7 +1337,7 @@ public final class Attribute implements Comparable { PredicateWithMessage allowedValues, ImmutableList> mandatoryProvidersList, ImmutableList> mandatoryNativeProviders, - ImmutableSet aspects) { + ImmutableList> aspects) { Preconditions.checkNotNull(configTransition); Preconditions.checkArgument( (configTransition == ConfigurationTransition.NONE && configurator == null) @@ -1695,7 +1710,10 @@ public final class Attribute implements Comparable { builder.value = defaultValue; builder.valueSet = false; builder.allowedValues = allowedValues; - builder.aspects = new LinkedHashSet<>(aspects); + builder.aspects = new LinkedHashMap<>(); + for (RuleAspect aspect : aspects) { + builder.aspects.put(aspect.getName(), aspect); + } return builder; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java index 848cdcc0d7..7256393faa 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java @@ -1247,7 +1247,7 @@ public final class SkylarkAttr { throw new EvalException(definitionLocation, "All aspects applied to rule dependencies must be top-level values"); } - attributeBuilder.aspect(skylarkAspect); + attributeBuilder.aspect(skylarkAspect, definitionLocation); } exported = true; } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java index 0b61f3f277..6746341eae 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.analysis.SkylarkProviders; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.AspectDefinition; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; @@ -1071,6 +1072,34 @@ public class SkylarkAspectsTest extends AnalysisTestCase { assertThat(analysisResult.hasError()).isFalse(); } + @Test + public void multipleAspects() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _aspect_impl(target,ctx):", + " return struct()", + "my_aspect = aspect(implementation = _aspect_impl)", + "def _dummy_impl(ctx):", + " pass", + "r1 = rule(_dummy_impl, ", + " attrs = { 'deps' : attr.label_list(aspects = [my_aspect, my_aspect]) })" + ); + + scratch.file( + "test/BUILD", + "load(':aspect.bzl', 'r1')", + "r1(name = 't1')" + ); + reporter.removeHandler(failFastHandler); + try { + AnalysisResult result = update("//test:r1"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); + } catch (TargetParsingException | ViewCreationFailedException expected) { + // expected. + } + assertContainsEvent("Aspect //test:aspect.bzl%my_aspect added more than once"); + } @RunWith(JUnit4.class) public static final class WithKeepGoing extends SkylarkAspectsTest { -- cgit v1.2.3