diff options
11 files changed, 377 insertions, 238 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java index 9b8be1f250..a998570d8f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java @@ -104,12 +104,6 @@ public abstract class AbstractConfiguredTarget @Override public Object getValue(String name) { switch (name) { - case FILES_FIELD: - case DEFAULT_RUNFILES_FIELD: - case DATA_RUNFILES_FIELD: - case FilesToRunProvider.SKYLARK_NAME: - // Standard fields should be proxied to their default provider object - return getDefaultProvider().getValue(name); case LABEL_FIELD: return getLabel(); default: @@ -206,7 +200,19 @@ public abstract class AbstractConfiguredTarget if (OutputGroupProvider.SKYLARK_NAME.equals(providerKey)) { return get(OutputGroupProvider.SKYLARK_CONSTRUCTOR); } - return rawGetSkylarkProvider(providerKey); + switch (providerKey) { + case FILES_FIELD: + case DEFAULT_RUNFILES_FIELD: + case DATA_RUNFILES_FIELD: + case FilesToRunProvider.SKYLARK_NAME: + // Standard fields should be proxied to their default provider object + return getDefaultProvider().getValue(providerKey); + case OutputGroupProvider.SKYLARK_NAME: + return get(OutputGroupProvider.SKYLARK_CONSTRUCTOR); + default: + return rawGetSkylarkProvider(providerKey); + } + } /** Implement in subclasses to get a skylark provider for a given {@code providerKey}. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 5e38598317..cd7b72b867 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -69,19 +69,18 @@ import com.google.devtools.build.lib.packages.NativeClassObjectConstructor; import com.google.devtools.build.lib.packages.OutputFile; import com.google.devtools.build.lib.packages.PackageSpecification; import com.google.devtools.build.lib.packages.RawAttributeMapper; +import com.google.devtools.build.lib.packages.RequiredProviders; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.packages.SkylarkClassObject; -import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.rules.AliasProvider; import com.google.devtools.build.lib.rules.MakeVariableProvider; import com.google.devtools.build.lib.rules.fileset.FilesetProvider; import com.google.devtools.build.lib.shell.ShellUtils; -import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.syntax.Type.LabelClass; @@ -719,7 +718,7 @@ public final class RuleContext extends TargetContext Map<Optional<String>, ? extends List<? extends TransitiveInfoCollection>> map = getSplitPrerequisites(attributeName); return map.isEmpty() - ? ImmutableList.<TransitiveInfoCollection>of() + ? ImmutableList.of() : map.entrySet().iterator().next().getValue(); } @@ -1978,153 +1977,100 @@ public final class RuleContext extends TargetContext } } - private String getMissingMandatoryProviders(ConfiguredTarget prerequisite, Attribute attribute){ - ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> mandatoryProvidersList = - attribute.getMandatoryProvidersList(); - if (mandatoryProvidersList.isEmpty()) { - return null; - } - List<List<String>> missingProvidersList = new ArrayList<>(); - for (ImmutableSet<SkylarkProviderIdentifier> providers : mandatoryProvidersList) { - List<String> missing = new ArrayList<>(); - for (SkylarkProviderIdentifier provider : providers) { - // A rule may require a built-in provider that is always implicitly provided, e.g. "files" - ImmutableSet<String> availableKeys = - ImmutableSet.copyOf(((ClassObject) prerequisite).getKeys()); - if ((prerequisite.get(provider) == null) - && !(provider.isLegacy() && availableKeys.contains(provider.getLegacyId()))) { - missing.add(provider.toString()); - } - } - if (missing.isEmpty()) { - return null; - } else { - missingProvidersList.add(missing); - } - } - StringBuilder missingProviders = new StringBuilder(); - Joiner joinProvider = Joiner.on("', '"); - for (List<String> providers : missingProvidersList) { - if (missingProviders.length() > 0) { - missingProviders.append(" or "); - } - missingProviders.append((providers.size() > 1) ? "[" : "") - .append("'"); - joinProvider.appendTo(missingProviders, providers); - missingProviders.append("'") - .append((providers.size() > 1) ? "]" : ""); + /** + * Because some rules still have to use allowedRuleClasses to do rule dependency validation. + * A dependency is valid if it is from a rule in allowedRuledClasses, OR if all of the providers + * in requiredProviders are provided by the target. + */ + private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) { + + Set<String> unfulfilledRequirements = new LinkedHashSet<>(); + if (checkRuleDependencyClass(prerequisite, attribute, unfulfilledRequirements)) { + return; } - return missingProviders.toString(); - } - private String getMissingMandatoryNativeProviders( - ConfiguredTarget prerequisite, Attribute attribute) { - List<ImmutableList<Class<? extends TransitiveInfoProvider>>> mandatoryProvidersList = - attribute.getMandatoryNativeProvidersList(); - if (mandatoryProvidersList.isEmpty()) { - return null; + if (checkRuleDependencyClassWarnings(prerequisite, attribute)) { + return; } - List<List<String>> missingProvidersList = new ArrayList<>(); - for (ImmutableList<Class<? extends TransitiveInfoProvider>> providers - : mandatoryProvidersList) { - List<String> missing = new ArrayList<>(); - for (Class<? extends TransitiveInfoProvider> provider : providers) { - if (prerequisite.getProvider(provider) == null) { - missing.add(provider.getSimpleName()); - } - } - if (missing.isEmpty()) { - return null; - } else { - missingProvidersList.add(missing); - } + + if (checkRuleDependencyMandatoryProviders(prerequisite, attribute, + unfulfilledRequirements)) { + return; } - StringBuilder missingProviders = new StringBuilder(); - Joiner joinProvider = Joiner.on(", "); - for (List<String> providers : missingProvidersList) { - if (missingProviders.length() > 0) { - missingProviders.append(" or "); - } - missingProviders.append((providers.size() > 1) ? "[" : ""); - joinProvider.appendTo(missingProviders, providers); - missingProviders.append((providers.size() > 1) ? "]" : ""); + + // not allowed rule class and some mandatory providers missing => reject. + if (!unfulfilledRequirements.isEmpty()) { + attributeError( + attribute.getName(), StringUtil.joinEnglishList(unfulfilledRequirements, "and")); } - return missingProviders.toString(); } /** - * Because some rules still have to use allowedRuleClasses to do rule dependency validation. - * A dependency is valid if it is from a rule in allowedRuleClasses, OR if all of the providers - * in mandatoryNativeProviders AND mandatoryProvidersList are provided by the target. + * Check if prerequisite should be allowed based on its rule class. */ - private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) { - Target prerequisiteTarget = prerequisite.getTarget(); - RuleClass ruleClass = ((Rule) prerequisiteTarget).getRuleClassObject(); - String notAllowedRuleClassesMessage = null; - + private boolean checkRuleDependencyClass(ConfiguredTarget prerequisite, Attribute attribute, + Set<String> unfulfilledRequirements) { if (attribute.getAllowedRuleClassesPredicate() != Predicates.<RuleClass>alwaysTrue()) { - if (attribute.getAllowedRuleClassesPredicate().apply(ruleClass)) { + if (attribute.getAllowedRuleClassesPredicate().apply( + ((Rule) prerequisite.getTarget()).getRuleClassObject())) { // prerequisite has an allowed rule class => accept. - return; + return true; } // remember that the rule class that was not allowed; // but maybe prerequisite provides required providers? do not reject yet. - notAllowedRuleClassesMessage = + unfulfilledRequirements.add( badPrerequisiteMessage( - prerequisiteTarget.getTargetKind(), + prerequisite.getTarget().getTargetKind(), prerequisite, "expected " + attribute.getAllowedRuleClassesPredicate(), - false); + false)); } + return false; + } - if (attribute.getAllowedRuleClassesWarningPredicate().apply(ruleClass)) { + /** + * Check if prerequisite should be allowed with warning based on its rule class. + * + * If yes, also issues said warning. + */ + private boolean checkRuleDependencyClassWarnings(ConfiguredTarget prerequisite, + Attribute attribute) { + if (attribute.getAllowedRuleClassesWarningPredicate().apply( + ((Rule) prerequisite.getTarget()).getRuleClassObject())) { Predicate<RuleClass> allowedRuleClasses = attribute.getAllowedRuleClassesPredicate(); - reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite, + reportBadPrerequisite(attribute, prerequisite.getTarget().getTargetKind(), prerequisite, allowedRuleClasses == Predicates.<RuleClass>alwaysTrue() ? null : "expected " + allowedRuleClasses, true); // prerequisite has a rule class allowed with a warning => accept, emitting a warning. - return; + return true; } + return false; + } - // If we get here, we have no allowed rule class. - // If mandatory providers are specified, try them. - Set<String> unfulfilledRequirements = new LinkedHashSet<>(); - if (!attribute.getMandatoryNativeProvidersList().isEmpty() - || !attribute.getMandatoryProvidersList().isEmpty()) { - boolean hadAllMandatoryProviders = true; - - String missingNativeProviders = getMissingMandatoryNativeProviders(prerequisite, attribute); - if (missingNativeProviders != null) { - unfulfilledRequirements.add( - "'" + prerequisite.getLabel() + "' does not have mandatory providers: " - + missingNativeProviders); - hadAllMandatoryProviders = false; - } - - String missingProviders = getMissingMandatoryProviders(prerequisite, attribute); - if (missingProviders != null) { - unfulfilledRequirements.add( - "'" + prerequisite.getLabel() + "' does not have mandatory providers: " - + missingProviders); - hadAllMandatoryProviders = false; - } + /** + * Check if prerequisite should be allowed based on required providers on + * the attribute. + */ + private boolean checkRuleDependencyMandatoryProviders(ConfiguredTarget prerequisite, + Attribute attribute, Set<String> unfulfilledRequirements) { + RequiredProviders requiredProviders = attribute.getRequiredProviders(); - if (hadAllMandatoryProviders) { - // all mandatory providers are present => accept. - return; - } + if (requiredProviders.acceptsAny()) { + // If no required providers specified, we do not know if we should accept. + return false; } - // not allowed rule class and some mandatory providers missing => reject. - if (notAllowedRuleClassesMessage != null) { - unfulfilledRequirements.add(notAllowedRuleClassesMessage); + if (prerequisite.satisfies(requiredProviders)) { + return true; } - if (!unfulfilledRequirements.isEmpty()) { - attributeError( - attribute.getName(), StringUtil.joinEnglishList(unfulfilledRequirements, "and")); - } + unfulfilledRequirements.add( + String.format("'%s' does not have mandatory providers: %s", + prerequisite.getLabel(), + prerequisite.missingProviders(requiredProviders).getDescription())); + + return false; } private void validateDirectPrerequisite(Attribute attribute, ConfiguredTarget prerequisite) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java index b174a47699..23c2238d58 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.RequiredProviders; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.SkylarkIndexable; @@ -79,4 +80,29 @@ public interface TransitiveInfoCollection extends SkylarkIndexable, SkylarkProvi * <b>null</b>.</p> */ @Nullable BuildConfiguration getConfiguration(); + + /** + * Checks whether this {@link TransitiveInfoCollection} satisfies given {@link RequiredProviders}. + */ + default boolean satisfies(RequiredProviders providers) { + return providers.isSatisfiedBy( + aClass -> getProvider(aClass.asSubclass(TransitiveInfoProvider.class)) != null, + id -> this.get(id) != null + ); + } + + /** + * Returns providers that this {@link TransitiveInfoCollection} misses from + * a given {@link RequiredProviders}. + * + * If none are missing, returns {@link RequiredProviders} that accept any set + * of providers. + */ + default RequiredProviders missingProviders(RequiredProviders providers) { + return providers.getMissing( + aClass -> getProvider(aClass.asSubclass(TransitiveInfoProvider.class)) != null, + id -> this.get(id) != null + ); + } + } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java index bb2456c08e..4c9454e45f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java @@ -14,16 +14,14 @@ package com.google.devtools.build.lib.bazel.rules; -import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.PackageSpecificationProvider; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; import com.google.devtools.build.lib.packages.PackageGroup; import com.google.devtools.build.lib.packages.RawAttributeMapper; +import com.google.devtools.build.lib.packages.RequiredProviders; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.rules.AliasProvider; @@ -77,15 +75,11 @@ public class BazelPrerequisiteValidator } if (prerequisiteTarget instanceof PackageGroup) { - ImmutableList<ImmutableList<Class<? extends TransitiveInfoProvider>>> - mandatoryNativeProviders = - RawAttributeMapper.of(rule) - .getAttributeDefinition(attrName) - .getMandatoryNativeProvidersList(); + RequiredProviders requiredProviders = RawAttributeMapper.of(rule) + .getAttributeDefinition(attrName) + .getRequiredProviders(); boolean containsPackageSpecificationProvider = - mandatoryNativeProviders - .stream() - .anyMatch(list -> list.contains(PackageSpecificationProvider.class)); + requiredProviders.getDescription().contains("PackageSpecificationProvider"); // TODO(plf): Add the PackageSpecificationProvider to the 'visibility' attribute. if (!attrName.equals("visibility") && !containsPackageSpecificationProvider) { context.reportError( 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 afcb9c5947..7114040300 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 @@ -457,10 +457,8 @@ public final class Attribute implements Comparable<Attribute> { private Predicate<AttributeMap> condition; private Set<PropertyFlag> propertyFlags = EnumSet.noneOf(PropertyFlag.class); private PredicateWithMessage<Object> allowedValues = null; - private ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> mandatoryProvidersList = - ImmutableList.<ImmutableSet<SkylarkProviderIdentifier>>of(); - private ImmutableList<ImmutableList<Class<? extends TransitiveInfoProvider>>> - mandatoryNativeProvidersList = ImmutableList.of(); + private RequiredProviders.Builder requiredProvidersBuilder = + RequiredProviders.acceptAnyBuilder(); private HashMap<String, RuleAspect<?>> aspects = new LinkedHashMap<>(); /** @@ -956,11 +954,11 @@ public final class Attribute implements Comparable<Attribute> { * {@link #allowedRuleClasses}. * * <p>If the attribute contains Labels of any other rule type (other than this or those set in - * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build + * allowedRuleClasses()) and they fulfill {@link #getRequiredProviders()}}, the build * continues without error. Else the build fails during analysis. * * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that - * fulfill {@link #getMandatoryNativeProvidersList()} build without error. + * fulfill {@link #getRequiredProviders()} build without error. * * <p>This only works on a per-target basis, not on a per-file basis; with other words, it * works for 'deps' attributes, but not 'srcs' attributes. @@ -978,12 +976,10 @@ public final class Attribute implements Comparable<Attribute> { Iterable<? extends Iterable<Class<? extends TransitiveInfoProvider>>> providersList) { Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY, "must be a label-valued type"); - ImmutableList.Builder<ImmutableList<Class<? extends TransitiveInfoProvider>>> listBuilder - = ImmutableList.builder(); + for (Iterable<Class<? extends TransitiveInfoProvider>> providers : providersList) { - listBuilder.add(ImmutableList.<Class<? extends TransitiveInfoProvider>>copyOf(providers)); + this.requiredProvidersBuilder.addNativeSet(ImmutableSet.copyOf(providers)); } - this.mandatoryNativeProvidersList = listBuilder.build(); return this; } @@ -1005,12 +1001,9 @@ public final class Attribute implements Comparable<Attribute> { Iterable<? extends Iterable<SkylarkProviderIdentifier>> providersList){ Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY, "must be a label-valued type"); - ImmutableList.Builder<ImmutableSet<SkylarkProviderIdentifier>> listBuilder - = ImmutableList.builder(); for (Iterable<SkylarkProviderIdentifier> providers : providersList) { - listBuilder.add(ImmutableSet.copyOf(providers)); + this.requiredProvidersBuilder.addSkylarkSet(ImmutableSet.copyOf(providers)); } - this.mandatoryProvidersList = listBuilder.build(); return this; } @@ -1181,8 +1174,7 @@ public final class Attribute implements Comparable<Attribute> { validityPredicate, condition, allowedValues, - mandatoryProvidersList, - mandatoryNativeProvidersList, + requiredProvidersBuilder.build(), ImmutableList.copyOf(aspects.values())); } } @@ -1795,10 +1787,7 @@ public final class Attribute implements Comparable<Attribute> { private final PredicateWithMessage<Object> allowedValues; - private final ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> mandatoryProvidersList; - - private final ImmutableList<ImmutableList<Class<? extends TransitiveInfoProvider>>> - mandatoryNativeProvidersList; + private final RequiredProviders requiredProviders; private final ImmutableList<RuleAspect<?>> aspects; @@ -1831,9 +1820,7 @@ public final class Attribute implements Comparable<Attribute> { ValidityPredicate validityPredicate, Predicate<AttributeMap> condition, PredicateWithMessage<Object> allowedValues, - ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> mandatoryProvidersList, - ImmutableList<ImmutableList<Class<? extends TransitiveInfoProvider>>> - mandatoryNativeProvidersList, + RequiredProviders requiredProviders, ImmutableList<RuleAspect<?>> aspects) { Preconditions.checkNotNull(configTransition); Preconditions.checkArgument( @@ -1867,8 +1854,7 @@ public final class Attribute implements Comparable<Attribute> { this.validityPredicate = validityPredicate; this.condition = condition; this.allowedValues = allowedValues; - this.mandatoryProvidersList = mandatoryProvidersList; - this.mandatoryNativeProvidersList = mandatoryNativeProvidersList; + this.requiredProviders = requiredProviders; this.aspects = aspects; } @@ -2067,17 +2053,8 @@ public final class Attribute implements Comparable<Attribute> { return allowedRuleClassesForLabelsWarning; } - /** - * Returns the list of sets of mandatory Skylark providers. - */ - public ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> getMandatoryProvidersList() { - return mandatoryProvidersList; - } - - /** Returns the list of lists of mandatory native providers. */ - public ImmutableList<ImmutableList<Class<? extends TransitiveInfoProvider>>> - getMandatoryNativeProvidersList() { - return mandatoryNativeProvidersList; + public RequiredProviders getRequiredProviders() { + return requiredProviders; } public FileTypeSet getAllowedFileTypesPredicate() { @@ -2236,8 +2213,7 @@ public final class Attribute implements Comparable<Attribute> { builder.allowedFileTypesForLabels = allowedFileTypesForLabels; builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels; builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; - builder.mandatoryNativeProvidersList = mandatoryNativeProvidersList; - builder.mandatoryProvidersList = mandatoryProvidersList; + builder.requiredProvidersBuilder = requiredProviders.copyAsBuilder(); builder.validityPredicate = validityPredicate; builder.condition = condition; builder.configTransition = configTransition; diff --git a/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java b/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java index 586b105bd9..e9298bd08c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java @@ -13,13 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.packages; -import com.google.common.base.Predicate; -import com.google.common.base.Predicates; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.util.Preconditions; +import java.util.function.Function; +import java.util.function.Predicate; +import javax.annotation.Nullable; /** * Represents a constraint on a set of providers required by a dependency (of a rule @@ -52,6 +53,10 @@ public final class RequiredProviders { */ private final ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> skylarkProviders; + public String getDescription() { + return constraint.getDescription(this); + } + /** * Represents one of the constraints as desctibed in {@link RequiredProviders} */ @@ -59,79 +64,157 @@ public final class RequiredProviders { /** Accept any dependency */ ANY { @Override - public boolean satisfies(AdvertisedProviderSet advertisedProviderSet, - RequiredProviders requiredProviders) { + public boolean satisfies( + AdvertisedProviderSet advertisedProviderSet, + RequiredProviders requiredProviders, + Builder missing) { return true; } @Override public boolean satisfies(Predicate<Class<?>> hasNativeProvider, Predicate<SkylarkProviderIdentifier> hasSkylarkProvider, - RequiredProviders requiredProviders) { + RequiredProviders requiredProviders, + Builder missingProviders) { return true; } + + @Override + Builder copyAsBuilder(RequiredProviders providers) { + return acceptAnyBuilder(); + } + + @Override + public String getDescription(RequiredProviders providers) { + return "no providers required"; + } }, /** Accept no dependency */ NONE { @Override - public boolean satisfies(AdvertisedProviderSet advertisedProviderSet, - RequiredProviders requiredProviders) { + public boolean satisfies( + AdvertisedProviderSet advertisedProviderSet, + RequiredProviders requiredProviders, + Builder missing) { return false; } @Override public boolean satisfies(Predicate<Class<?>> hasNativeProvider, Predicate<SkylarkProviderIdentifier> hasSkylarkProvider, - RequiredProviders requiredProviders) { + RequiredProviders requiredProviders, + Builder missingProviders) { return false; } + + @Override + Builder copyAsBuilder(RequiredProviders providers) { + return acceptNoneBuilder(); + } + + @Override + public String getDescription(RequiredProviders providers) { + return "no providers accepted"; + } }, + /** Accept a dependency that has all providers from one of the sets. */ RESTRICTED { @Override + public boolean satisfies(final AdvertisedProviderSet advertisedProviderSet, + RequiredProviders requiredProviders, Builder missing) { + if (advertisedProviderSet.canHaveAnyProvider()) { + return true; + } + return satisfies( + advertisedProviderSet.getNativeProviders()::contains, + advertisedProviderSet.getSkylarkProviders()::contains, + requiredProviders, missing); + } + + @Override public boolean satisfies(Predicate<Class<?>> hasNativeProvider, Predicate<SkylarkProviderIdentifier> hasSkylarkProvider, - RequiredProviders requiredProviders) { + RequiredProviders requiredProviders, + Builder missingProviders) { for (ImmutableSet<Class<?>> nativeProviderSet : requiredProviders.nativeProviders) { - if (Iterables.all(nativeProviderSet, hasNativeProvider)) { + if (nativeProviderSet.stream().allMatch(hasNativeProvider)) { return true; } + + // Collect missing providers + if (missingProviders != null) { + missingProviders.addNativeSet( + nativeProviderSet.stream().filter(hasNativeProvider.negate()) + .collect(ImmutableSet.toImmutableSet())); + } + } for (ImmutableSet<SkylarkProviderIdentifier> skylarkProviderSet : requiredProviders.skylarkProviders) { - if (Iterables.all(skylarkProviderSet, hasSkylarkProvider)) { + if (skylarkProviderSet.stream().allMatch(hasSkylarkProvider)) { return true; } + // Collect missing providers + if (missingProviders != null) { + missingProviders.addSkylarkSet( + skylarkProviderSet.stream().filter(hasSkylarkProvider.negate()) + .collect(ImmutableSet.toImmutableSet())); + } } return false; } + + @Override + Builder copyAsBuilder(RequiredProviders providers) { + Builder result = acceptAnyBuilder(); + for (ImmutableSet<Class<?>> nativeProviderSet : providers.nativeProviders) { + result.addNativeSet(nativeProviderSet); + } + for (ImmutableSet<SkylarkProviderIdentifier> skylarkProviderSet : + providers.skylarkProviders) { + result.addSkylarkSet(skylarkProviderSet); + } + return result; + } + + @Override + public String getDescription(RequiredProviders providers) { + StringBuilder result = new StringBuilder(); + describe(result, providers.nativeProviders, Class::getSimpleName); + describe(result, providers.skylarkProviders, id -> "'" + id.toString() + "'"); + return result.toString(); + + } }; /** Checks if {@code advertisedProviderSet} satisfies these {@code RequiredProviders} */ - public boolean satisfies(final AdvertisedProviderSet advertisedProviderSet, - RequiredProviders requiredProviders) { - if (advertisedProviderSet.canHaveAnyProvider()) { - return true; - } - return satisfies( - aClass -> advertisedProviderSet.getNativeProviders().contains(aClass), - Predicates.in(advertisedProviderSet.getSkylarkProviders()), - requiredProviders); - } + public abstract boolean satisfies(AdvertisedProviderSet advertisedProviderSet, + RequiredProviders requiredProviders, Builder missing); /** * Checks if a set of providers encoded by predicates {@code hasNativeProviders} * and {@code hasSkylarkProvider} satisfies these {@code RequiredProviders} */ - abstract boolean satisfies(Predicate<Class<?>> hasNativeProvider, + abstract boolean satisfies( + Predicate<Class<?>> hasNativeProvider, Predicate<SkylarkProviderIdentifier> hasSkylarkProvider, - RequiredProviders requiredProviders); + RequiredProviders requiredProviders, + @Nullable + Builder missingProviders); + + abstract Builder copyAsBuilder(RequiredProviders providers); + + /** + * Returns a string describing the providers that can be presented to the user. + */ + abstract String getDescription(RequiredProviders providers); } /** Checks if {@code advertisedProviderSet} satisfies this {@code RequiredProviders} instance. */ public boolean isSatisfiedBy(AdvertisedProviderSet advertisedProviderSet) { - return constraint.satisfies(advertisedProviderSet, this); + return constraint.satisfies(advertisedProviderSet, this, null); } /** @@ -141,7 +224,43 @@ public final class RequiredProviders { public boolean isSatisfiedBy( Predicate<Class<?>> hasNativeProvider, Predicate<SkylarkProviderIdentifier> hasSkylarkProvider) { - return constraint.satisfies(hasNativeProvider, hasSkylarkProvider, this); + return constraint.satisfies(hasNativeProvider, hasSkylarkProvider, this, null); + } + + /** + * Returns providers that are missing. If none are missing, returns + * {@code RequiredProviders} that accept anything. + */ + public RequiredProviders getMissing( + Predicate<Class<?>> hasNativeProvider, + Predicate<SkylarkProviderIdentifier> hasSkylarkProvider) { + Builder builder = acceptAnyBuilder(); + if (constraint.satisfies(hasNativeProvider, hasSkylarkProvider, this, builder)) { + // Ignore all collected missing providers. + return acceptAnyBuilder().build(); + } + return builder.build(); + } + + /** + * Returns providers that are missing. If none are missing, returns + * {@code RequiredProviders} that accept anything. + */ + public RequiredProviders getMissing(AdvertisedProviderSet set) { + Builder builder = acceptAnyBuilder(); + if (constraint.satisfies(set, this, builder)) { + // Ignore all collected missing providers. + return acceptAnyBuilder().build(); + } + return builder.build(); + } + + + /** + * Returns true if this {@code RequiredProviders} instance accept any set of providers. + */ + public boolean acceptsAny() { + return constraint.equals(Constraint.ANY); } @@ -160,6 +279,23 @@ public final class RequiredProviders { } /** + * Helper method to describe lists of sets of things. + */ + private static <T> void describe(StringBuilder result, + ImmutableList<ImmutableSet<T>> listOfSets, Function<T, String> describeOne) { + Joiner joiner = Joiner.on(", "); + for (ImmutableSet<T> ids : listOfSets) { + if (result.length() > 0) { + result.append(" or "); + } + result.append((ids.size() > 1) ? "[" : ""); + joiner.appendTo(result, ids.stream().map(describeOne).iterator()); + result.append((ids.size() > 1) ? "]" : ""); + } + } + + + /** * A builder for {@link RequiredProviders} that accepts any dependency * unless restriction provider sets are added. */ @@ -175,6 +311,13 @@ public final class RequiredProviders { return new Builder(true); } + /** + * Returns a Builder initialized to the same value as this {@code RequiredProvider} + */ + public Builder copyAsBuilder() { + return constraint.copyAsBuilder(this); + } + /** A builder for {@link RequiredProviders} */ public static class Builder { private final ImmutableList.Builder<ImmutableSet<Class<?>>> nativeProviders; 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 2091822e95..c8b1368bbc 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 @@ -276,7 +276,11 @@ public final class SkylarkAttr implements SkylarkValue { SkylarkType.checkType(obj, SkylarkList.class, PROVIDERS_ARG); ImmutableList<ImmutableSet<SkylarkProviderIdentifier>> providersList = buildProviderPredicate( (SkylarkList<?>) obj, PROVIDERS_ARG, ast.getLocation()); - builder.mandatoryProvidersList(providersList); + + // If there is at least one empty set, there is no restriction. + if (providersList.stream().noneMatch(ImmutableSet::isEmpty)) { + builder.mandatoryProvidersList(providersList); + } } if (containsNonNoneKey(arguments, CONFIGURATION_ARG)) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 0d11365d45..4e3356d052 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; -import com.google.common.base.Predicate; import com.google.common.base.Supplier; import com.google.common.base.Verify; import com.google.common.base.VerifyException; @@ -43,7 +42,6 @@ import com.google.devtools.build.lib.analysis.MergedConfiguredTarget; import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.ToolchainContext; -import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; @@ -67,7 +65,6 @@ import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClassProvider; -import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException; @@ -987,20 +984,7 @@ public final class ConfiguredTargetFunction implements SkyFunction { if (!aspect.getDefinition().applyToFiles() && !(dep.getTarget() instanceof Rule)) { return false; } - return aspect.getDefinition().getRequiredProviders().isSatisfiedBy( - new Predicate<Class<?>>() { - @Override - public boolean apply(Class<?> provider) { - return dep.getProvider(provider.asSubclass(TransitiveInfoProvider.class)) != null; - } - }, - new Predicate<SkylarkProviderIdentifier>() { - @Override - public boolean apply(SkylarkProviderIdentifier skylarkProviderIdentifier) { - return dep.get(skylarkProviderIdentifier) != null; - } - } - ); + return dep.satisfies(aspect.getDefinition().getRequiredProviders()); } /** diff --git a/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java b/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java index 03cc956b71..05ba757976 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.packages; import static com.google.common.truth.Truth.assertThat; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; @@ -31,6 +30,9 @@ import org.junit.runners.JUnit4; */ @RunWith(JUnit4.class) public class RequiredProvidersTest { + + private static final String NO_PROVIDERS_REQUIRED = "no providers required"; + private static final class P1 {} private static final class P2 {} private static final class P3 {} @@ -60,8 +62,8 @@ public class RequiredProvidersTest { boolean result = requiredProviders.isSatisfiedBy(providers); assertThat(requiredProviders.isSatisfiedBy( - Predicates.in(providers.getNativeProviders()), - Predicates.in(providers.getSkylarkProviders()) + providers.getNativeProviders()::contains, + providers.getSkylarkProviders()::contains )).isEqualTo(result); return result; } @@ -108,7 +110,11 @@ public class RequiredProvidersTest { .addNative(P1.class) .addNative(P2.class) .build(); - assertThat(validateNative(providerSet, ImmutableSet.<Class<?>>of(P1.class, P2.class))) + assertThat( + validateNative( + providerSet, + NO_PROVIDERS_REQUIRED, + ImmutableSet.of(P1.class, P2.class))) .isTrue(); } @@ -119,6 +125,7 @@ public class RequiredProvidersTest { AdvertisedProviderSet.builder() .addNative(P1.class) .build(), + NO_PROVIDERS_REQUIRED, ImmutableSet.<Class<?>>of(P1.class), ImmutableSet.<Class<?>>of(P2.class) )).isTrue(); @@ -131,6 +138,7 @@ public class RequiredProvidersTest { AdvertisedProviderSet.builder() .addNative(P3.class) .build(), + "P1 or P2", ImmutableSet.<Class<?>>of(P1.class), ImmutableSet.<Class<?>>of(P2.class) )).isFalse(); @@ -144,6 +152,7 @@ public class RequiredProvidersTest { .addSkylark(ID_SKYLARK) .build(); assertThat(validateSkylark(providerSet, + NO_PROVIDERS_REQUIRED, ImmutableSet.of( ID_LEGACY, ID_SKYLARK, ID_NATIVE))) .isTrue(); @@ -156,6 +165,7 @@ public class RequiredProvidersTest { AdvertisedProviderSet.builder() .addSkylark(ID_LEGACY) .build(), + NO_PROVIDERS_REQUIRED, ImmutableSet.of(ID_LEGACY), ImmutableSet.of(ID_NATIVE) )).isTrue(); @@ -168,13 +178,29 @@ public class RequiredProvidersTest { AdvertisedProviderSet.builder() .addSkylark(ID_SKYLARK) .build(), + "'p_legacy' or 'p_native'", ImmutableSet.of(ID_LEGACY), ImmutableSet.of(ID_NATIVE) )).isFalse(); } + @Test + public void checkDescriptions() { + assertThat(RequiredProviders.acceptAnyBuilder().build().getDescription()) + .isEqualTo("no providers required"); + assertThat(RequiredProviders.acceptNoneBuilder().build().getDescription()) + .isEqualTo("no providers accepted"); + assertThat(RequiredProviders.acceptAnyBuilder() + .addSkylarkSet(ImmutableSet.of(ID_LEGACY, ID_SKYLARK)) + .addSkylarkSet(ImmutableSet.of(ID_SKYLARK)) + .addNativeSet(ImmutableSet.of(P1.class, P2.class)) + .build().getDescription()) + .isEqualTo("[P1, P2] or ['p_legacy', 'p_skylark'] or 'p_skylark'"); + } + @SafeVarargs private static boolean validateNative(AdvertisedProviderSet providerSet, + String missing, ImmutableSet<Class<?>>... sets) { Builder anyBuilder = RequiredProviders.acceptAnyBuilder(); Builder noneBuilder = RequiredProviders.acceptNoneBuilder(); @@ -182,14 +208,20 @@ public class RequiredProvidersTest { anyBuilder.addNativeSet(set); noneBuilder.addNativeSet(set); } - boolean result = satisfies(providerSet, anyBuilder.build()); - assertThat(satisfies(providerSet, noneBuilder.build())).isEqualTo(result); + RequiredProviders rpStartingFromAny = anyBuilder.build(); + boolean result = satisfies(providerSet, rpStartingFromAny); + assertThat(rpStartingFromAny.getMissing(providerSet).getDescription()).isEqualTo(missing); + + RequiredProviders rpStaringFromNone = noneBuilder.build(); + assertThat(satisfies(providerSet, rpStaringFromNone)).isEqualTo(result); + assertThat(rpStaringFromNone.getMissing(providerSet).getDescription()).isEqualTo(missing); return result; } @SafeVarargs private static boolean validateSkylark( AdvertisedProviderSet providerSet, + String missing, ImmutableSet<SkylarkProviderIdentifier>... sets) { Builder anyBuilder = RequiredProviders.acceptAnyBuilder(); Builder noneBuilder = RequiredProviders.acceptNoneBuilder(); @@ -197,8 +229,14 @@ public class RequiredProvidersTest { anyBuilder.addSkylarkSet(set); noneBuilder.addSkylarkSet(set); } - boolean result = satisfies(providerSet, anyBuilder.build()); - assertThat(satisfies(providerSet, noneBuilder.build())).isEqualTo(result); + + RequiredProviders rpStartingFromAny = anyBuilder.build(); + boolean result = satisfies(providerSet, rpStartingFromAny); + assertThat(rpStartingFromAny.getMissing(providerSet).getDescription()).isEqualTo(missing); + + RequiredProviders rpStaringFromNone = noneBuilder.build(); + assertThat(satisfies(providerSet, rpStaringFromNone)).isEqualTo(result); + assertThat(rpStaringFromNone.getMissing(providerSet).getDescription()).isEqualTo(missing); return result; } } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 08b9ccdf39..6db90093bd 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -711,7 +711,7 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { "main_rule = rule(implementation = rule_impl, attrs = {", " 'deps': attr.label_list(providers = [", " 'files', 'data_runfiles', 'default_runfiles',", - " 'files_to_run', 'label', 'output_groups',", + " 'files_to_run', 'output_groups',", " ])", "})"); scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index b3d614816d..9c20c6bd02 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java @@ -249,19 +249,44 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { buildAttribute("a1", "b = provider()", "attr.label_list(allow_files = True, providers = ['a', b])"); - assertThat(attr.getMandatoryProvidersList()) - .containsExactly(ImmutableSet.of(legacy("a"), declared("b"))); + assertThat(attr.getRequiredProviders().isSatisfiedBy(set(legacy("a"), declared("b")))) + .isTrue(); + assertThat(attr.getRequiredProviders().isSatisfiedBy(set(legacy("a")))) + .isFalse(); + } @Test + public void testAttrWithProvidersOneEmpty() throws Exception { + Attribute attr = + buildAttribute("a1", + "b = provider()", + "attr.label_list(allow_files = True, providers = [['a', b],[]])"); + assertThat(attr.getRequiredProviders().acceptsAny()).isTrue(); + } + + + @Test public void testAttrWithProvidersList() throws Exception { Attribute attr = buildAttribute("a1", "b = provider()", "attr.label_list(allow_files = True, providers = [['a', b], ['c']])"); - assertThat(attr.getMandatoryProvidersList()).containsExactly( - ImmutableSet.of(legacy("a"), declared("b")), - ImmutableSet.of(legacy("c"))); + assertThat(attr.getRequiredProviders().isSatisfiedBy(set(legacy("a"), declared("b")))) + .isTrue(); + assertThat(attr.getRequiredProviders().isSatisfiedBy(set(legacy("c")))) + .isTrue(); + assertThat(attr.getRequiredProviders().isSatisfiedBy(set(legacy("a")))) + .isFalse(); + + } + + private static AdvertisedProviderSet set(SkylarkProviderIdentifier ...ids) { + AdvertisedProviderSet.Builder builder = AdvertisedProviderSet.builder(); + for (SkylarkProviderIdentifier id : ids) { + builder.addSkylark(id); + } + return builder.build(); } private void checkAttributeError(String expectedMessage, String... lines) throws Exception { @@ -425,11 +450,8 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { private static final RuleClass.ConfiguredTargetFactory<Object, Object> DUMMY_CONFIGURED_TARGET_FACTORY = - new RuleClass.ConfiguredTargetFactory<Object, Object>() { - @Override - public Object create(Object ruleContext) throws InterruptedException { - throw new IllegalStateException(); - } + ruleContext -> { + throw new IllegalStateException(); }; private RuleClass ruleClass(String name) { |