Rollback of commit 0218320f1316b4d25e35abaaa80206237468824d. *** Reason for rollback *** Breaks skylark macros that call skylark rules that declare an 'args' attribute. -- MOS_MIGRATED_REVID=101118137
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 d051892..67cc935 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
@@ -751,19 +751,9 @@ /** * Adds or overrides the attribute in the rule class. Meant for Skylark usage. - * - * @throws IllegalArgumentException if the attribute overrides an existing attribute (will be - * legal in the future). */ public void addOrOverrideAttribute(Attribute attribute) { - String name = attribute.getName(); - boolean override = attributes.containsKey(name); - // Attributes may be overridden in the future, thus we keep the code for overriding right - // now. - Preconditions.checkArgument( - !override, "There is already a built-in attribute '%s' which cannot be overridden", name); - - if (override) { + if (attributes.containsKey(attribute.getName())) { overrideAttribute(attribute); } else { addAttribute(attribute);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index fc4c222..2f12950 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -160,10 +160,8 @@ .build(); /** - * Translates Skylark attribute names to native names. - * - * <p> In native code, private values start with "$".In Skylark, private values start with "_", - * because of the grammar. + * In native code, private values start with $. + * In Skylark, private values start with _, because of the grammar. */ private static String attributeToNative(String oldName, Location loc, boolean isLateBound) throws EvalException { @@ -230,76 +228,68 @@ doc = "List of names of configuration fragments that the rule requires.")}, useAst = true, useEnvironment = true) private static final BuiltinFunction rule = new BuiltinFunction("rule") { - @SuppressWarnings({"rawtypes", "unchecked"}) // castMap produces - // an Attribute.Builder instead of a Attribute.Builder<?> but it's OK. - public BaseFunction invoke(BaseFunction implementation, Boolean test, Object attrs, - Object implicitOutputs, Boolean executable, Boolean outputToGenfiles, SkylarkList fragments, - FuncallExpression ast, Environment funcallEnv) throws EvalException, ConversionException { - Preconditions.checkState(funcallEnv instanceof SkylarkEnvironment); - RuleClassType type = test ? RuleClassType.TEST : RuleClassType.NORMAL; + @SuppressWarnings({"rawtypes", "unchecked"}) // castMap produces + // an Attribute.Builder instead of a Attribute.Builder<?> but it's OK. + public BaseFunction invoke(BaseFunction implementation, Boolean test, Object attrs, + Object implicitOutputs, Boolean executable, Boolean outputToGenfiles, + SkylarkList fragments, FuncallExpression ast, Environment funcallEnv) + throws EvalException, ConversionException { - // We'll set the name later, pass the empty string for now. - RuleClass.Builder builder = - test ? new RuleClass.Builder("", type, true, testBaseRule) - : new RuleClass.Builder("", type, true, baseRule); + Preconditions.checkState(funcallEnv instanceof SkylarkEnvironment); + RuleClassType type = test ? RuleClassType.TEST : RuleClassType.NORMAL; - if (attrs != Environment.NONE) { - for (Map.Entry<String, Attribute.Builder> attr : - castMap(attrs, String.class, Attribute.Builder.class, "attrs").entrySet()) { - Attribute.Builder<?> attrBuilder = (Attribute.Builder<?>) attr.getValue(); - String attrName = - attributeToNative(attr.getKey(), ast.getLocation(), attrBuilder.hasLateBoundValue()); - addAttribute(builder, attrBuilder.build(attrName)); + // We'll set the name later, pass the empty string for now. + RuleClass.Builder builder = test + ? new RuleClass.Builder("", type, true, testBaseRule) + : new RuleClass.Builder("", type, true, baseRule); + + if (attrs != Environment.NONE) { + for (Map.Entry<String, Attribute.Builder> attr : castMap( + attrs, String.class, Attribute.Builder.class, "attrs").entrySet()) { + Attribute.Builder<?> attrBuilder = (Attribute.Builder<?>) attr.getValue(); + String attrName = attributeToNative(attr.getKey(), ast.getLocation(), + attrBuilder.hasLateBoundValue()); + builder.addOrOverrideAttribute(attrBuilder.build(attrName)); + } } - } - if (executable || test) { - addAttribute( - builder, - attr("$is_executable", BOOLEAN) - .value(true) - .nonconfigurable("Called from RunCommand.isExecutable, which takes a Target") - .build()); - builder.setOutputsDefaultExecutable(); - } - - if (implicitOutputs != Environment.NONE) { - if (implicitOutputs instanceof BaseFunction) { - BaseFunction func = (BaseFunction) implicitOutputs; - final SkylarkCallbackFunction callback = - new SkylarkCallbackFunction(func, ast, (SkylarkEnvironment) funcallEnv); - builder.setImplicitOutputsFunction( - new SkylarkImplicitOutputsFunctionWithCallback(callback, ast.getLocation())); - } else { - builder.setImplicitOutputsFunction( - new SkylarkImplicitOutputsFunctionWithMap(ImmutableMap.copyOf(castMap(implicitOutputs, - String.class, String.class, "implicit outputs of the rule class")))); + if (executable || test) { + builder.addOrOverrideAttribute( + attr("$is_executable", BOOLEAN).value(true) + .nonconfigurable("Called from RunCommand.isExecutable, which takes a Target") + .build()); + builder.setOutputsDefaultExecutable(); } - } - if (outputToGenfiles) { - builder.setOutputToGenfiles(); - } + if (implicitOutputs != Environment.NONE) { + if (implicitOutputs instanceof BaseFunction) { + BaseFunction func = (BaseFunction) implicitOutputs; + final SkylarkCallbackFunction callback = + new SkylarkCallbackFunction(func, ast, (SkylarkEnvironment) funcallEnv); + builder.setImplicitOutputsFunction( + new SkylarkImplicitOutputsFunctionWithCallback(callback, ast.getLocation())); + } else { + builder.setImplicitOutputsFunction(new SkylarkImplicitOutputsFunctionWithMap( + ImmutableMap.copyOf(castMap(implicitOutputs, String.class, String.class, + "implicit outputs of the rule class")))); + } + } - if (!fragments.isEmpty()) { - builder.requiresConfigurationFragments( - new SkylarkModuleNameResolver(), - Iterables.toArray(castList(fragments, String.class), String.class)); - } + if (outputToGenfiles) { + builder.setOutputToGenfiles(); + } - builder.setConfiguredTargetFunction(implementation); - builder.setRuleDefinitionEnvironment( - ((SkylarkEnvironment) funcallEnv).getGlobalEnvironment()); - return new RuleFunction(builder, type); - } - - private void addAttribute(RuleClass.Builder builder, Attribute attribute) throws EvalException { - try { - builder.addOrOverrideAttribute(attribute); - } catch (IllegalArgumentException ex) { - throw new EvalException(location, ex); - } - } - }; + if (!fragments.isEmpty()) { + builder.requiresConfigurationFragments( + new SkylarkModuleNameResolver(), + Iterables.toArray(castList(fragments, String.class), String.class)); + } + + builder.setConfiguredTargetFunction(implementation); + builder.setRuleDefinitionEnvironment( + ((SkylarkEnvironment) funcallEnv).getGlobalEnvironment()); + return new RuleFunction(builder, type); + } + }; // This class is needed for testing static final class RuleFunction extends BaseFunction {