From ee624453d9f5dd908d721abb4663246c70bf4509 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Wed, 13 Jan 2016 18:41:24 +0000 Subject: For native rule classes, (de)serialize only explicit attrs Native rule classes can provide default values for rules after they're deserialized, so there isn't a need to serialize those default values. This doesn't apply yet to rules with Skylark-defined rule classes, due to the non-serializablity of Skylark rule classes. -- MOS_MIGRATED_REVID=112066930 --- .../build/lib/packages/ExternalPackageBuilder.java | 10 +- .../devtools/build/lib/packages/Package.java | 17 +- .../build/lib/packages/PackageFactory.java | 4 +- .../devtools/build/lib/packages/RuleClass.java | 345 +++++++++++++-------- .../devtools/build/lib/packages/RuleFactory.java | 164 +++++++--- .../build/lib/packages/RuleSerializer.java | 87 +++--- 6 files changed, 412 insertions(+), 215 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/packages') diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java index 8570899691..b771763eb2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.events.StoredEventHandler; +import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.util.Preconditions; @@ -40,8 +41,10 @@ public class ExternalPackageBuilder { InterruptedException { StoredEventHandler eventHandler = new StoredEventHandler(); + BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs); Rule tempRule = - RuleFactory.createRule(pkg, ruleClass, kwargs, eventHandler, ast, ast.getLocation(), null); + RuleFactory.createRule( + pkg, ruleClass, attributeValues, eventHandler, ast, ast.getLocation(), /*env=*/ null); pkg.addEvents(eventHandler.getEvents()); overwriteRule(pkg, tempRule); for (Map.Entry entry : @@ -64,8 +67,11 @@ public class ExternalPackageBuilder { attributes.put("actual", actual); } StoredEventHandler handler = new StoredEventHandler(); + BuildLangTypedAttributeValuesMap attributeValues = + new BuildLangTypedAttributeValuesMap(attributes); Rule rule = - RuleFactory.createRule(pkg, bindRuleClass, attributes, handler, null, location, null); + RuleFactory.createRule( + pkg, bindRuleClass, attributeValues, handler, /*ast=*/ null, location, /*env=*/ null); overwriteRule(pkg, rule); rule.setVisibility(ConstantRuleVisibility.PUBLIC); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index ecf1df70dc..bc7804475c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -1001,17 +1001,16 @@ public class Package { } /** - * Returns a new Rule belonging to this package instance, and uses the given Label. + * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package} + * associated with this {@link Builder}. * - *

Useful for RuleClass instantiation, where the rule name is checked by trying to create a - * Label. This label can then be used again here. + *

The created {@link Rule} will have no attribute values, no output files, and therefore + * will be in an invalid state. */ - Rule newRuleWithLabel(Label label, RuleClass ruleClass, Location location) { - return newRuleWithLabelAndAttrContainer(label, ruleClass, location, - new AttributeContainer(ruleClass)); - } - - Rule newRuleWithLabelAndAttrContainer(Label label, RuleClass ruleClass, Location location, + Rule createRule( + Label label, + RuleClass ruleClass, + Location location, AttributeContainer attributeContainer) { return new Rule(pkg, label, ruleClass, location, attributeContainer); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index ba9111c375..1de065eed8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.GlobCache.BadGlobException; import com.google.devtools.build.lib.packages.License.DistributionType; import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing; +import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param; import com.google.devtools.build.lib.syntax.AssignmentStatement; @@ -923,7 +924,8 @@ public final class PackageFactory { Environment env) throws RuleFactory.InvalidRuleException, Package.NameConflictException, InterruptedException { RuleClass ruleClass = getBuiltInRuleClass(ruleClassName, ruleFactory); - RuleFactory.createAndAddRule(context, ruleClass, kwargs, ast, env); + BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs); + RuleFactory.createAndAddRule(context, ruleClass, attributeValues, ast, env); } private static RuleClass getBuiltInRuleClass(String ruleClassName, RuleFactory ruleFactory) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 1b07ae0bba..8e00db47b6 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.BuildType.SelectorList; import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; +import com.google.devtools.build.lib.packages.RuleFactory.AttributeValuesMap; import com.google.devtools.build.lib.syntax.Argument; import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.Environment; @@ -45,6 +46,7 @@ import com.google.devtools.build.lib.syntax.GlobList; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkList; import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.syntax.Type.ConversionException; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.PathFragment; @@ -1273,104 +1275,189 @@ public final class RuleClass { } /** - * Helper function for {@link RuleFactory#createAndAddRule}. + * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package} + * associated with {@code pkgBuilder}. + * + *

The created {@link Rule} will be populated with attribute values from {@code + * attributeValues} or the default attribute values associated with this {@link RuleClass} and + * {@code pkgBuilder}. + * + *

The created {@link Rule} will also be populated with output files. These output files + * will have been collected from the explicitly provided values of type {@link BuildType#OUTPUT} + * and {@link BuildType#OUTPUT_LIST} as well as from the implicit outputs determined by this + * {@link RuleClass} and the values in {@code attributeValues}. + * + *

This performs several validity checks. Invalid output file labels result in a thrown {@link + * LabelSyntaxException}. All other errors are reported on {@code eventHandler}. */ - Rule createRuleWithLabel(Package.Builder pkgBuilder, Label ruleLabel, - Map attributeValues, EventHandler eventHandler, FuncallExpression ast, - Location location) throws LabelSyntaxException, InterruptedException { - Rule rule = pkgBuilder.newRuleWithLabel(ruleLabel, this, location); - createRuleCommon(rule, pkgBuilder, attributeValues, eventHandler, ast); - return rule; - } - - private void createRuleCommon(Rule rule, Package.Builder pkgBuilder, - Map attributeValues, EventHandler eventHandler, FuncallExpression ast) + Rule createRule( + Package.Builder pkgBuilder, + Label ruleLabel, + AttributeValuesMap attributeValues, + EventHandler eventHandler, + @Nullable FuncallExpression ast, + Location location, + AttributeContainer attributeContainer) throws LabelSyntaxException, InterruptedException { - populateRuleAttributeValues( - rule, pkgBuilder, attributeValues, eventHandler, ast); + Rule rule = pkgBuilder.createRule(ruleLabel, this, location, attributeContainer); + populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler); rule.populateOutputFiles(eventHandler, pkgBuilder); + if (ast != null) { + populateAttributeLocations(rule, ast); + } + checkForDuplicateLabels(rule, eventHandler); + checkThirdPartyRuleHasLicense(rule, pkgBuilder, eventHandler); + checkForValidSizeAndTimeoutValues(rule, eventHandler); rule.checkForNullLabels(); rule.checkValidityPredicate(eventHandler); + return rule; + } + + /** + * Populates the attributes table of the new {@link Rule} with the values in the {@code + * attributeValues} map and with default values provided by this {@link RuleClass} and the {@code + * pkgBuilder}. + * + *

Errors are reported on {@code eventHandler}. + */ + private void populateRuleAttributeValues( + Rule rule, + Package.Builder pkgBuilder, + AttributeValuesMap attributeValues, + EventHandler eventHandler) { + BitSet definedAttrIndices = + populateDefinedRuleAttributeValues(rule, attributeValues, eventHandler); + populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler); + // Now that all attributes are bound to values, collect and store configurable attribute keys. + populateConfigDependenciesAttribute(rule); } /** - * Populates the attributes table of new rule "rule" from the - * "attributeValues" mapping from attribute names to values in the build - * language. Errors are reported on "reporter". "ast" is used to associate - * location information with each rule attribute. + * Populates the attributes table of the new {@link Rule} with the values in the {@code + * attributeValues} map. + * + *

Handles the special cases of the attribute named {@code "name"} and attributes with value + * {@link Runtime#NONE}. + * + *

Returns a bitset {@code b} where {@code b.get(i)} is {@code true} if this method set a + * value for the attribute with index {@code i} in this {@link RuleClass}. Errors are reported + * on {@code eventHandler}. */ - private void populateRuleAttributeValues(Rule rule, - Package.Builder pkgBuilder, - Map attributeValues, - EventHandler eventHandler, - FuncallExpression ast) - throws InterruptedException { - BitSet definedAttrs = new BitSet(); // set of attr indices - - for (Map.Entry entry : attributeValues.entrySet()) { - String attributeName = entry.getKey(); - Object attributeValue = entry.getValue(); - if (attributeValue == Runtime.NONE) { // Ignore all None values. + private BitSet populateDefinedRuleAttributeValues( + Rule rule, AttributeValuesMap attributeValues, EventHandler eventHandler) { + BitSet definedAttrIndices = new BitSet(); + for (String attributeName : attributeValues.getAttributeNames()) { + // The attribute named "name" was handled in a special way already. + if (attributeName.equals("name")) { continue; } - Integer attrIndex = setRuleAttributeValue(rule, eventHandler, attributeName, attributeValue); - if (attrIndex != null) { - definedAttrs.set(attrIndex); - checkAttrValNonEmpty(rule, eventHandler, attributeValue, attrIndex); + + Object attributeValue = attributeValues.getAttributeValue(attributeName); + // Ignore all None values. + if (attributeValue == Runtime.NONE) { + continue; + } + + // Check that the attribute's name belongs to a valid attribute for this rule class. + Integer attrIndex = getAttributeIndex(attributeName); + if (attrIndex == null) { + rule.reportError( + String.format( + "%s: no such attribute '%s' in '%s' rule", rule.getLabel(), attributeName, name), + eventHandler); + continue; + } + Attribute attr = getAttribute(attrIndex); + + // Convert the build-lang value to a native value, if necessary. + Object nativeAttributeValue; + if (attributeValues.valuesAreBuildLanguageTyped()) { + try { + nativeAttributeValue = convertFromBuildLangType(rule, attr, attributeValue); + } catch (ConversionException e) { + rule.reportError(String.format("%s: %s", rule.getLabel(), e.getMessage()), eventHandler); + continue; + } + } else { + nativeAttributeValue = attributeValue; } + + boolean explicit = attributeValues.isAttributeExplicitlySpecified(attributeName); + setRuleAttributeValue(rule, eventHandler, attr, nativeAttributeValue, explicit); + definedAttrIndices.set(attrIndex); + checkAttrValNonEmpty(rule, eventHandler, attributeValue, attr); } + return definedAttrIndices; + } - // Save the location of each non-default attribute definition: - if (ast != null) { - for (Argument.Passed arg : ast.getArguments()) { - if (arg.isKeyword()) { - String name = arg.getName(); - Integer attrIndex = getAttributeIndex(name); - if (attrIndex != null) { - rule.setAttributeLocation(attrIndex, arg.getValue().getLocation()); - } + /** Populates attribute locations for attributes defined in {@code ast}. */ + private void populateAttributeLocations(Rule rule, FuncallExpression ast) { + for (Argument.Passed arg : ast.getArguments()) { + if (arg.isKeyword()) { + String name = arg.getName(); + Integer attrIndex = getAttributeIndex(name); + if (attrIndex != null) { + rule.setAttributeLocation(attrIndex, arg.getValue().getLocation()); } } } + } + /** + * Populates the attributes table of the new {@link Rule} with default values provided by this + * {@link RuleClass} and the {@code pkgBuilder}. This will only provide values for attributes + * that haven't already been populated, using {@code definedAttrIndices} to determine whether an + * attribute was populated. + * + *

Errors are reported on {@code eventHandler}. + */ + private void populateDefaultRuleAttributeValues( + Rule rule, Package.Builder pkgBuilder, BitSet definedAttrIndices, EventHandler eventHandler) { + // Set defaults; ensure that every mandatory attribute has a value. Use the default if none + // is specified. List attrsWithComputedDefaults = new ArrayList<>(); - - // Set defaults; ensure that every mandatory attribute has a value. Use - // the default if none is specified. int numAttributes = getAttributeCount(); for (int attrIndex = 0; attrIndex < numAttributes; ++attrIndex) { - if (!definedAttrs.get(attrIndex)) { - Attribute attr = getAttribute(attrIndex); - if (attr.isMandatory()) { - rule.reportError(rule.getLabel() + ": missing value for mandatory " - + "attribute '" + attr.getName() + "' in '" - + name + "' rule", eventHandler); - } + if (definedAttrIndices.get(attrIndex)) { + continue; + } + Attribute attr = getAttribute(attrIndex); + if (attr.isMandatory()) { + rule.reportError( + String.format( + "%s: missing value for mandatory attribute '%s' in '%s' rule", + rule.getLabel(), + attr.getName(), + name), + eventHandler); + } - if (attr.hasComputedDefault()) { - attrsWithComputedDefaults.add(attr); - } else { - Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder); - checkAttrValNonEmpty(rule, eventHandler, defaultValue, attrIndex); - rule.setAttributeValue(attr, defaultValue, /*explicit=*/false); - checkAllowedValues(rule, attr, eventHandler); - } + if (attr.hasComputedDefault()) { + // Note that it is necessary to set all non-computed default values before calling + // Attribute#getDefaultValue for computed default attributes. Computed default attributes + // may have a condition predicate (i.e. the predicate returned by Attribute#getCondition) + // that depends on non-computed default attribute values, and that condition predicate is + // evaluated by the call to Attribute#getDefaultValue. + attrsWithComputedDefaults.add(attr); + } else { + Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder); + checkAttrValNonEmpty(rule, eventHandler, defaultValue, attr); + rule.setAttributeValue(attr, defaultValue, /*explicit=*/ false); + checkAllowedValues(rule, attr, eventHandler); } } - // Evaluate and set any computed defaults now that all non-computed - // TODO(bazel-team): remove this special casing. Thanks to configurable attributes refactoring, - // computed defaults don't get bound to their final values at this point, so we no longer - // have to wait until regular attributes have been initialized. + // Set computed default attribute values now that all other (i.e. non-computed) default values + // have been set. for (Attribute attr : attrsWithComputedDefaults) { - rule.setAttributeValue(attr, attr.getDefaultValue(rule), /*explicit=*/false); + // If Attribute#hasComputedDefault is true, Attribute#getDefaultValue returns the computed + // default function object. Note that we don't evaluate the computed default function here + // because it may depend on other attribute values that are configurable (i.e. they came + // from select({..}) expressions in the build language, and they require configuration data + // from the analysis phase to be resolved). We're setting the attribute value to a + // reference to the computed default function. + rule.setAttributeValue(attr, attr.getDefaultValue(rule), /*explicit=*/ false); } - - // Now that all attributes are bound to values, collect and store configurable attribute keys. - populateConfigDependenciesAttribute(rule); - checkForDuplicateLabels(rule, eventHandler); - checkThirdPartyRuleHasLicense(rule, pkgBuilder, eventHandler); - checkForValidSizeAndTimeoutValues(rule, eventHandler); } /** @@ -1398,9 +1485,7 @@ public final class RuleClass { } private void checkAttrValNonEmpty( - Rule rule, EventHandler eventHandler, Object attributeValue, Integer attrIndex) { - - Attribute attr = getAttribute(attrIndex); + Rule rule, EventHandler eventHandler, Object attributeValue, Attribute attr) { if (!attr.isNonEmpty()) { return; } @@ -1525,65 +1610,64 @@ public final class RuleClass { } /** - * Sets the value of attribute "attrName" in rule "rule", by converting the - * build-language value "attrVal" to the appropriate type for the attribute. - * Returns the attribute index iff successful, null otherwise. + * Sets the value of attribute {@code attr} in {@code rule} to the native value {@code + * nativeAttrVal}, and sets the value's explicitness to {@code explicit}. + * + *

Handles the special case of the "visibility" attribute by also setting the rule's + * visibility with {@link Rule#setVisibility}. * - *

In case of failure, error messages are reported on "handler", and "rule" - * is marked as containing errors. + *

Checks that {@code nativeAttrVal} is an allowed value via {@link #checkAllowedValues}. */ - @SuppressWarnings("unchecked") - private Integer setRuleAttributeValue(Rule rule, - EventHandler eventHandler, - String attrName, - Object attrVal) { - if (attrName.equals("name")) { - return null; // "name" is handled specially - } - - Integer attrIndex = getAttributeIndex(attrName); - if (attrIndex == null) { - rule.reportError(rule.getLabel() + ": no such attribute '" + attrName - + "' in '" + name + "' rule", eventHandler); - return null; - } - - Attribute attr = getAttribute(attrIndex); - Object converted; - try { - String what = "attribute '" + attrName + "' in '" + name + "' rule"; - converted = BuildType.selectableConvert(attr.getType(), attrVal, what, rule.getLabel()); - - if ((converted instanceof SelectorList) && !attr.isConfigurable()) { - rule.reportError(rule.getLabel() + ": attribute \"" + attr.getName() - + "\" is not configurable", eventHandler); - return null; + private static void setRuleAttributeValue( + Rule rule, + EventHandler eventHandler, + Attribute attr, + Object nativeAttrVal, + boolean explicit) { + if (attr.getName().equals("visibility")) { + @SuppressWarnings("unchecked") + List

Throws {@link ConversionException} if the conversion fails, or if {@code buildLangValue} + * is a selector expression but {@code attr.isConfigurable()} is {@code false}. + */ + private static Object convertFromBuildLangType(Rule rule, Attribute attr, Object buildLangValue) + throws ConversionException { + String what = String.format("attribute '%s' in '%s' rule", attr.getName(), rule.getRuleClass()); + Object converted = + BuildType.selectableConvert(attr.getType(), buildLangValue, what, rule.getLabel()); + + if ((converted instanceof SelectorList) && !attr.isConfigurable()) { + throw new ConversionException( + String.format("attribute \"%s\" is not configurable", attr.getName())); } - if (attrName.equals("visibility")) { - List

If the rule is configurable, all of its potential values are evaluated, and errors for each * of the invalid values are reported. */ - void checkAllowedValues(Rule rule, Attribute attribute, EventHandler eventHandler) { + private static void checkAllowedValues( + Rule rule, Attribute attribute, EventHandler eventHandler) { if (attribute.checkAllowedValues()) { PredicateWithMessage allowedValues = attribute.getAllowedValues(); Iterable values = @@ -1603,9 +1688,13 @@ public final class RuleClass { attribute.getName(), attribute.getType()); for (Object value : values) { if (!allowedValues.apply(value)) { - rule.reportError(String.format(rule.getLabel() + ": invalid value in '%s' attribute: %s", - attribute.getName(), - allowedValues.getErrorReason(value)), eventHandler); + rule.reportError( + String.format( + "%s: invalid value in '%s' attribute: %s", + rule.getLabel(), + attribute.getName(), + allowedValues.getErrorReason(value)), + eventHandler); } } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java index 7982a62386..4837b04763 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java @@ -34,12 +34,12 @@ import java.util.Set; import javax.annotation.Nullable; /** - * Given a rule class and a set of attributes, returns a Rule instance. Also - * performs a number of checks and associates the rule and the owning package + * Given a {@link RuleClass} and a set of attribute values, returns a {@link Rule} instance. Also + * performs a number of checks and associates the {@link Rule} and the owning {@link Package} * with each other. * - *

Note: the code that actually populates the RuleClass map has been moved - * to {@link RuleClassProvider}. + *

Note: the code that actually populates the RuleClass map has been moved to {@link + * RuleClassProvider}. */ public class RuleFactory { @@ -73,20 +73,20 @@ public class RuleFactory { * Creates and returns a rule instance. * *

It is the caller's responsibility to add the rule to the package (the caller may choose not - * to do so if, for example, the rule has errors).

+ * to do so if, for example, the rule has errors). */ static Rule createRule( Package.Builder pkgBuilder, RuleClass ruleClass, - Map attributeValues, + BuildLangTypedAttributeValuesMap attributeValues, EventHandler eventHandler, - FuncallExpression ast, + @Nullable FuncallExpression ast, Location location, @Nullable Environment env) throws InvalidRuleException, InterruptedException { Preconditions.checkNotNull(ruleClass); String ruleClassName = ruleClass.getName(); - Object nameObject = attributeValues.get("name"); + Object nameObject = attributeValues.getAttributeValue("name"); if (nameObject == null) { throw new InvalidRuleException(ruleClassName + " rule has no 'name' attribute"); } else if (!(nameObject instanceof String)) { @@ -113,38 +113,48 @@ public class RuleFactory { AttributesAndLocation generator = generatorAttributesForMacros(attributeValues, env, location, label); try { - return ruleClass.createRuleWithLabel( - pkgBuilder, label, generator.attributes, eventHandler, ast, generator.location); + return ruleClass.createRule( + pkgBuilder, + label, + generator.attributes, + eventHandler, + ast, + generator.location, + new AttributeContainer(ruleClass)); } catch (LabelSyntaxException e) { throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage()); } } /** - * Creates a rule instance, adds it to the package and returns it. + * Creates a {@link Rule} instance, adds it to the {@link Package.Builder} and returns it. * - * @param pkgBuilder the under-construction package to which the rule belongs - * @param ruleClass the class of the rule; this must not be null - * @param attributeValues a map of attribute names to attribute values. Each - * attribute must be defined for this class of rule, and have a value - * of the appropriate type. There must be a map entry for each - * non-optional attribute of this class of rule. + * @param pkgBuilder the under-construction {@link Package.Builder} to which the rule belongs + * @param ruleClass the {@link RuleClass} of the rule + * @param attributeValues a {@link BuildLangTypedAttributeValuesMap} mapping attribute names to + * attribute values of build-language type. Each attribute must be defined for this class of + * rule, and have a build-language-typed value which can be converted to the appropriate + * native type of the attribute (i.e. via {@link BuildType#selectableConvert}). There must + * be a map entry for each non-optional attribute of this class of rule. * @param eventHandler a eventHandler on which errors and warnings are reported during - * rule creation + * rule creation * @param ast the abstract syntax tree of the rule expression (optional) * @param location the location at which this rule was declared + * @param env the lexical environment of the function call which declared this rule (optional) * @throws InvalidRuleException if the rule could not be constructed for any - * reason (e.g. no name attribute is defined) - * @throws InvalidRuleException, NameConflictException + * reason (e.g. no {@code name} attribute is defined) + * @throws NameConflictException if the rule's name or output files conflict with others in this + * package + * @throws InterruptedException if interrupted */ static Rule createAndAddRule( Package.Builder pkgBuilder, RuleClass ruleClass, - Map attributeValues, + BuildLangTypedAttributeValuesMap attributeValues, EventHandler eventHandler, - FuncallExpression ast, + @Nullable FuncallExpression ast, Location location, - Environment env) + @Nullable Environment env) throws InvalidRuleException, NameConflictException, InterruptedException { Rule rule = createRule( pkgBuilder, ruleClass, attributeValues, eventHandler, ast, location, env); @@ -152,12 +162,31 @@ public class RuleFactory { return rule; } + /** + * Creates a {@link Rule} instance, adds it to the {@link Package.Builder} and returns it. + * + * @param context the package-building context in which this rule was declared + * @param ruleClass the {@link RuleClass} of the rule + * @param attributeValues a {@link BuildLangTypedAttributeValuesMap} mapping attribute names to + * attribute values of build-language type. Each attribute must be defined for this class + * of rule, and have a build-language-typed value which can be converted to the appropriate + * native type of the attribute (i.e. via {@link BuildType#selectableConvert}). There must + * be a map entry for each non-optional attribute of this class of rule. + * @param ast the abstract syntax tree of the rule expression (mandatory because this looks up a + * {@link Location} from the {@code ast}) + * @param env the lexical environment of the function call which declared this rule (optional) + * @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no + * {@code name} attribute is defined) + * @throws NameConflictException if the rule's name or output files conflict with others in this + * package + * @throws InterruptedException if interrupted + */ public static Rule createAndAddRule( PackageContext context, RuleClass ruleClass, - Map attributeValues, + BuildLangTypedAttributeValuesMap attributeValues, FuncallExpression ast, - Environment env) + @Nullable Environment env) throws InvalidRuleException, NameConflictException, InterruptedException { return createAndAddRule( context.pkgBuilder, @@ -170,8 +199,8 @@ public class RuleFactory { } /** - * InvalidRuleException is thrown by createRule() if the Rule could not be - * constructed. It contains an error message. + * InvalidRuleException is thrown by {@link Rule} creation methods if the {@link Rule} could + * not be constructed. It contains an error message. */ public static class InvalidRuleException extends Exception { private InvalidRuleException(String message) { @@ -179,17 +208,71 @@ public class RuleFactory { } } - /** Pair of attributes and location */ + /** A pair of attributes and location. */ private static final class AttributesAndLocation { - final Map attributes; + final BuildLangTypedAttributeValuesMap attributes; final Location location; - AttributesAndLocation(Map attributes, Location location) { + AttributesAndLocation(BuildLangTypedAttributeValuesMap attributes, Location location) { this.attributes = attributes; this.location = location; } } + /** + * A wrapper around an map of named attribute values that specifies whether the map's values + * are of "build-language" or of "native" types. + */ + public interface AttributeValuesMap { + /** + * Returns {@code true} if all the map's values are "build-language typed", i.e., resulting + * from the evaluation of an expression in the build language. Returns {@code false} if all + * the map's values are "natively typed", i.e. of a type returned by {@link + * BuildType#selectableConvert}. + */ + boolean valuesAreBuildLanguageTyped(); + + Iterable getAttributeNames(); + + Object getAttributeValue(String attributeName); + + boolean isAttributeExplicitlySpecified(String attributeName); + } + + /** A {@link AttributeValuesMap} of explicit "build-language" values. */ + public static final class BuildLangTypedAttributeValuesMap implements AttributeValuesMap { + + private final Map attributeValues; + + public BuildLangTypedAttributeValuesMap(Map attributeValues) { + this.attributeValues = attributeValues; + } + + private boolean containsAttributeNamed(String attributeName) { + return attributeValues.containsKey(attributeName); + } + + @Override + public boolean valuesAreBuildLanguageTyped() { + return true; + } + + @Override + public Iterable getAttributeNames() { + return attributeValues.keySet(); + } + + @Override + public Object getAttributeValue(String attributeName) { + return attributeValues.get(attributeName); + } + + @Override + public boolean isAttributeExplicitlySpecified(String attributeName) { + return true; + } + } + /** * If the rule was created by a macro, this method sets the appropriate values for the * attributes generator_{name, function, location} and returns all attributes. @@ -197,14 +280,17 @@ public class RuleFactory { *

Otherwise, it returns the given attributes without any changes. */ private static AttributesAndLocation generatorAttributesForMacros( - Map args, @Nullable Environment env, Location location, Label label) { + BuildLangTypedAttributeValuesMap args, + @Nullable Environment env, + Location location, + Label label) { // Returns the original arguments if a) there is only the rule itself on the stack // trace (=> no macro) or b) the attributes have already been set by Python pre-processing. if (env == null) { return new AttributesAndLocation(args, location); } - boolean hasName = args.containsKey("generator_name"); - boolean hasFunc = args.containsKey("generator_function"); + boolean hasName = args.containsAttributeNamed("generator_name"); + boolean hasFunc = args.containsAttributeNamed("generator_function"); // TODO(bazel-team): resolve cases in our code where hasName && !hasFunc, or hasFunc && !hasName if (hasName || hasFunc) { return new AttributesAndLocation(args, location); @@ -217,23 +303,25 @@ public class RuleFactory { FuncallExpression generator = topCall.first; BaseFunction function = topCall.second; String name = generator.getNameArg(); + ImmutableMap.Builder builder = ImmutableMap.builder(); - - builder.putAll(args); - builder.put("generator_name", (name == null) ? args.get("name") : name); + for (String attributeName : args.getAttributeNames()) { + builder.put(attributeName, args.getAttributeValue(attributeName)); + } + builder.put("generator_name", (name == null) ? args.getAttributeValue("name") : name); builder.put("generator_function", function.getName()); if (generator.getLocation() != null) { location = generator.getLocation(); } - String relativePath = maybeGetRelativeLocation(location, label); if (relativePath != null) { builder.put("generator_location", relativePath); } try { - return new AttributesAndLocation(builder.build(), location); + return new AttributesAndLocation( + new BuildLangTypedAttributeValuesMap(builder.build()), location); } catch (IllegalArgumentException ex) { // We just fall back to the default case and swallow any messages. return new AttributesAndLocation(args, location); diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java index 658515cb49..f7470e92e6 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java @@ -36,48 +36,34 @@ public class RuleSerializer { builder.setName(rule.getLabel().getName()); builder.setRuleClass(rule.getRuleClass()); builder.setPublicByDefault(rule.getRuleClassObject().isPublicByDefault()); + RawAttributeMapper rawAttributeMapper = RawAttributeMapper.of(rule); + boolean isSkylark = rule.getRuleClassObject().isSkylark(); + for (Attribute attr : rule.getAttributes()) { Object rawAttributeValue = rawAttributeMapper.getRawAttributeValue(rule, attr); + boolean isExplicit = rule.isAttributeValueExplicitlySpecified(attr); + + if (!isSkylark && !isExplicit) { + // If the rule class is native (i.e. not Skylark-defined), then we can skip serialization + // of implicit attribute values. The native rule class can provide the same default value + // for the attribute after deserialization. + continue; + } Object valueToSerialize; - if (rawAttributeValue instanceof ComputedDefault) { - if (rule.getRuleClassObject().isSkylark()) { - // If the rule class is Skylark-defined (i.e. rule.getRuleClassObject().isSkylark() is - // true), and the attribute has a ComputedDefault value, we must serialize it. The - // Skylark-defined ComputedDefault function won't be available after deserialization due - // to Skylark's non-serializability. Fortunately (from the perspective of rule - // serialization), Skylark doesn't support defining rule classes with ComputedDefault - // attributes, and so the only ComputedDefault attributes we need to worry about for - // Skylark-defined rule classes are those declared in those rule classes' natively - // defined base rule classes. - // - // See the comment for SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES for the locations - // of these expected attributes. - // - // The RawAttributeMapper#get method, inherited from AbstractAttributeMapper, evaluates - // the ComputedDefault function, so we use that, after verifying the attribute's name is - // expected. - Preconditions.checkState( - SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES.contains(attr.getName()), - "Unexpected ComputedDefault value for %s in %s", - attr, - rule); - valueToSerialize = rawAttributeMapper.get(attr.getName(), attr.getType()); - } else { - // If the rule class is native (i.e. not Skylark-defined), we can skip serialization of - // attributes with ComputedDefault values. The native rule class can provide the same - // ComputedDefault value for the attribute after deserialization. - // - // TODO(mschaller): While the native rule class *could* provide it, it doesn't yet. Make - // it so! For now, we fall back to flattening the set of all possible values, computed - // using AggregatingAttributeMapper. - Iterable possibleValues = - AggregatingAttributeMapper.of(rule).getPossibleAttributeValues(rule, attr); - valueToSerialize = - AggregatingAttributeMapper.flattenAttributeValues(attr.getType(), possibleValues); - } + if (isExplicit) { + valueToSerialize = rawAttributeValue; + } else if (rawAttributeValue instanceof ComputedDefault) { + // If the rule class is Skylark-defined (i.e. rule.getRuleClassObject().isSkylark() is + // true), and the attribute has a ComputedDefault value, then we must serialize what it + // evaluates to. The Skylark-defined ComputedDefault function won't be available after + // deserialization due to Skylark's non-serializability. + valueToSerialize = evaluateSkylarkComputedDefault(rule, rawAttributeMapper, attr); } else { + // If the rule class is Skylark-defined and the attribute value is implicit, then we + // must serialize it. The Skylark-defined rule class won't be available after + // deserialization due to Skylark's non-serializability. valueToSerialize = rawAttributeValue; } @@ -85,11 +71,38 @@ public class RuleSerializer { AttributeSerializer.getAttributeProto( attr, valueToSerialize, - rule.isAttributeValueExplicitlySpecified(attr), + isExplicit, /*includeGlobs=*/ true, /*encodeBooleanAndTriStateAsIntegerAndString=*/ false)); } return builder; } + + /** + * Evaluates a {@link ComputedDefault} attribute value for a {@link Rule} with a + * Skylark-defined {@link RuleClass}. + * + *

Fortunately (from the perspective of rule serialization), Skylark doesn't support defining + * rule classes with {@link ComputedDefault} attributes, and so the only {@link + * ComputedDefault} attributes we need to worry about for Skylark-defined rule classes are + * declared in those rule classes' natively-defined base rule classes. + * + *

See the comment for {@link #SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES} for the + * locations of these expected attributes. None of them have dependencies on other attributes + * which are configurable, so they can be evaluated here without loss of fidelity. + * + *

The {@link RawAttributeMapper#get} method, inherited from {@link + * AbstractAttributeMapper}, evaluates the {@link ComputedDefault} function, so we use that, + * after verifying the attribute's name is expected. + */ + private static Object evaluateSkylarkComputedDefault( + Rule rule, RawAttributeMapper rawAttributeMapper, Attribute attr) { + Preconditions.checkState( + SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES.contains(attr.getName()), + "Unexpected ComputedDefault value for %s in %s", + attr, + rule); + return rawAttributeMapper.get(attr.getName(), attr.getType()); + } } -- cgit v1.2.3