Always fail on unknown attributes

Fixes: #11000
RELNOTES[INC]: Fails on unknown attributes (even when set to None)
PiperOrigin-RevId: 562519157
Change-Id: If5e430c73485c8ae9661f4231692384a057f37d5

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -91,6 +91,7 @@
 import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.Type;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkSubruleApi;
@@ -830,6 +831,9 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
             pkgContext.getBuilder(),
             ruleClass,
             attributeValues,
+            thread
+                .getSemantics()
+                .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
             pkgContext.getEventHandler(),
             thread.getCallStack());
       } catch (InvalidRuleException | NameConflictException e) {

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java

@@ -77,7 +77,7 @@ public static Rule createRule(
     try {
       rule =
           RuleFactory.createAndAddRule(
-              packageBuilder, ruleClass, attributeValues, eventHandler, callStack);
+              packageBuilder, ruleClass, attributeValues, true, eventHandler, callStack);
     } catch (NameConflictException e) {
       // This literally cannot happen -- we just created the package!
       throw new IllegalStateException(e);

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

@@ -1982,11 +1982,13 @@ <T> Rule createRule(
       Package.Builder pkgBuilder,
       Label ruleLabel,
       AttributeValues<T> attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       List<StarlarkThread.CallStackEntry> callstack)
       throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
     Rule rule = pkgBuilder.createRule(ruleLabel, this, callstack);
-    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler);
+    populateRuleAttributeValues(
+        rule, pkgBuilder, attributeValues, failOnUnknownAttributes, eventHandler);
     checkAspectAllowedValues(rule, eventHandler);
     rule.populateOutputFiles(eventHandler, pkgBuilder);
     checkForDuplicateLabels(rule, eventHandler);
@@ -2009,7 +2011,7 @@ <T> Rule createRuleUnchecked(
       ImplicitOutputsFunction implicitOutputsFunction)
       throws InterruptedException, CannotPrecomputeDefaultsException {
     Rule rule = pkgBuilder.createRule(ruleLabel, this, callstack.toLocation(), callstack.next());
-    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, NullEventHandler.INSTANCE);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, true, NullEventHandler.INSTANCE);
     rule.populateOutputFilesUnchecked(pkgBuilder, implicitOutputsFunction);
     return rule;
   }
@@ -2025,6 +2027,7 @@ private <T> void populateRuleAttributeValues(
       Rule rule,
       Package.Builder pkgBuilder,
       AttributeValues<T> attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler)
       throws InterruptedException, CannotPrecomputeDefaultsException {
 
@@ -2033,6 +2036,7 @@ private <T> void populateRuleAttributeValues(
             rule,
             pkgBuilder.getLabelConverter(),
             attributeValues,
+            failOnUnknownAttributes,
             pkgBuilder.getListInterner(),
             eventHandler);
     populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler);
@@ -2055,14 +2059,15 @@ private <T> BitSet populateDefinedRuleAttributeValues(
       Rule rule,
       LabelConverter labelConverter,
       AttributeValues<T> attributeValues,
+      boolean failOnUnknownAttributes,
       Interner<ImmutableList<?>> listInterner,
       EventHandler eventHandler) {
     BitSet definedAttrIndices = new BitSet();
     for (T attributeAccessor : attributeValues.getAttributeAccessors()) {
       String attributeName = attributeValues.getName(attributeAccessor);
       Object attributeValue = attributeValues.getValue(attributeAccessor);
       // Ignore all None values.
-      if (attributeValue == Starlark.NONE) {
+      if (attributeValue == Starlark.NONE && !failOnUnknownAttributes) {
         continue;
       }
 
@@ -2084,10 +2089,15 @@ private <T> BitSet populateDefinedRuleAttributeValues(
             eventHandler);
         continue;
       }
+      // Ignore all None values (after reporting an error)
+      if (attributeValue == Starlark.NONE) {
+        continue;
+      }
+
       Attribute attr = getAttribute(attrIndex);
 
       if (attributeName.equals("licenses") && ignoreLicenses) {
-        rule.setAttributeValue(attr, License.NO_LICENSE, /*explicit=*/ false);
+        rule.setAttributeValue(attr, License.NO_LICENSE, /* explicit= */ false);
         definedAttrIndices.set(attrIndex);
         continue;
       }

src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.Map;
 import java.util.Set;
@@ -55,6 +56,7 @@ public static Rule createRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, InterruptedException {
@@ -92,7 +94,8 @@ public static Rule createRule(
     callstack = callstack.subList(0, callstack.size() - 1); // pop
 
     try {
-      return ruleClass.createRule(pkgBuilder, label, attributes, eventHandler, callstack);
+      return ruleClass.createRule(
+          pkgBuilder, label, attributes, failOnUnknownAttributes, eventHandler, callstack);
     } catch (LabelSyntaxException | CannotPrecomputeDefaultsException e) {
       throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage());
     }
@@ -122,10 +125,18 @@ public static Rule createAndAddRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, NameConflictException, InterruptedException {
-    Rule rule = createRule(pkgBuilder, ruleClass, attributeValues, eventHandler, callstack);
+    Rule rule =
+        createRule(
+            pkgBuilder,
+            ruleClass,
+            attributeValues,
+            failOnUnknownAttributes,
+            eventHandler,
+            callstack);
     pkgBuilder.addRule(rule);
     return rule;
   }
@@ -282,6 +293,9 @@ public NoneType call(StarlarkThread thread, Tuple args, Dict<String, Object> kwa
             context.pkgBuilder,
             ruleClass,
             new BuildLangTypedAttributeValuesMap(kwargs),
+            thread
+                .getSemantics()
+                .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
             context.eventHandler,
             thread.getCallStack());
       } catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java

@@ -49,7 +49,8 @@ public static Rule createAndAddRepositoryRule(
           InterruptedException {
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
-    Rule rule = RuleFactory.createRule(pkg, ruleClass, attributeValues, eventHandler, callstack);
+    Rule rule =
+        RuleFactory.createRule(pkg, ruleClass, attributeValues, true, eventHandler, callstack);
     pkg.addEvents(eventHandler.getEvents());
     pkg.addPosts(eventHandler.getPosts());
     overwriteRule(pkg, rule);
@@ -164,7 +165,8 @@ static void addBindRule(
     StoredEventHandler handler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);
-    Rule rule = RuleFactory.createRule(pkg, bindRuleClass, attributeValues, handler, callstack);
+    Rule rule =
+        RuleFactory.createRule(pkg, bindRuleClass, attributeValues, true, handler, callstack);
     overwriteRule(pkg, rule);
   }
 

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -712,6 +712,16 @@ public final class BuildLanguageOptions extends OptionsBase {
               + "from the top level target instead")
   public boolean incompatibleDisableObjcLibraryTransition;
 
+  // cleanup, flip, remove after Bazel LTS in Nov 2023
+  @Option(
+      name = "incompatible_fail_on_unknown_attributes",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help = "If enabled, targets that have unknown attributes set to None fail.")
+  public boolean incompatibleFailOnUnknownAttributes;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -812,6 +822,7 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION,
                 incompatibleDisableObjcLibraryTransition)
+            .setBool(INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES, incompatibleFailOnUnknownAttributes)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -903,6 +914,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-incompatible_objc_provider_remove_linking_info";
   public static final String INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION =
       "-incompatible_disable_objc_library_transition";
+  public static final String INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES =
+      "-incompatible_fail_on_unknown_attributes";
 
   // non-booleans
   public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =

src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java

@@ -454,4 +454,62 @@ public void findArtifactByOutputLabel_twoOutputsWithSameBasename() throws Except
                 .getPathString())
         .isEqualTo("foo/out");
   }
+
+  @Test
+  public void testNativeRuleAttrSetToNoneFails() throws Exception {
+    setBuildLanguageOptions("--incompatible_fail_on_unknown_attributes");
+    scratch.file(
+        "p/BUILD", //
+        "genrule(name = 'genrule', srcs = ['a.java'], outs = ['b'], cmd = '', bat = None)");
+
+    reporter.removeHandler(failFastHandler);
+    getTarget("//p:genrule");
+
+    assertContainsEvent("no such attribute 'bat' in 'genrule' rule");
+  }
+
+  @Test
+  public void testNativeRuleAttrSetToNoneDoesntFails() throws Exception {
+    setBuildLanguageOptions("--noincompatible_fail_on_unknown_attributes");
+    scratch.file(
+        "p/BUILD", //
+        "genrule(name = 'genrule', srcs = ['a.java'], outs = ['b'], cmd = '', bat = None)");
+
+    getTarget("//p:genrule");
+  }
+
+  @Test
+  public void testStarlarkRuleAttrSetToNoneFails() throws Exception {
+    setBuildLanguageOptions("--incompatible_fail_on_unknown_attributes");
+    scratch.file(
+        "p/rule.bzl", //
+        "def _impl(ctx):",
+        "  pass",
+        "my_rule = rule(_impl)");
+    scratch.file(
+        "p/BUILD", //
+        "load(':rule.bzl', 'my_rule')",
+        "my_rule(name = 'my_target',  bat = None)");
+
+    reporter.removeHandler(failFastHandler);
+    getTarget("//p:my_target");
+
+    assertContainsEvent("no such attribute 'bat' in 'my_rule' rule");
+  }
+
+  @Test
+  public void testStarlarkRuleAttrSetToNoneDoesntFail() throws Exception {
+    setBuildLanguageOptions("--noincompatible_fail_on_unknown_attributes");
+    scratch.file(
+        "p/rule.bzl", //
+        "def _impl(ctx):",
+        "  pass",
+        "my_rule = rule(_impl)");
+    scratch.file(
+        "p/BUILD", //
+        "load(':rule.bzl', 'my_rule')",
+        "my_rule(name = 'my_target',  bat = None)");
+
+    getTarget("//p:my_target");
+  }
 }

src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java

@@ -933,6 +933,7 @@ private Rule createRule(RuleClass ruleClass, String name, Map<String, Object> at
         pkgBuilder,
         ruleLabel,
         new BuildLangTypedAttributeValuesMap(attributeValues),
+        true,
         reporter,
         ImmutableList.of(
             StarlarkThread.callStackEntry(StarlarkThread.TOP_LEVEL, testRuleLocation)));

src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java

@@ -90,6 +90,7 @@ public void testCreateRule(@TestParameter boolean explicitlySetGeneratorAttrs) t
             pkgBuilder,
             ruleClass,
             new BuildLangTypedAttributeValuesMap(attributeValues),
+            true,
             new Reporter(new EventBus()),
             DUMMY_STACK);
 
@@ -149,6 +150,7 @@ public void testCreateWorkspaceRule() throws Exception {
             pkgBuilder,
             ruleClass,
             new BuildLangTypedAttributeValuesMap(attributeValues),
+            true,
             new Reporter(new EventBus()),
             DUMMY_STACK);
     assertThat(rule.containsErrors()).isFalse();
@@ -172,6 +174,7 @@ public void testWorkspaceRuleFailsInBuildFile() {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     DUMMY_STACK));
     assertThat(e).hasMessageThat().contains("must be in the WORKSPACE file");
@@ -195,6 +198,7 @@ public void testBuildRuleFailsInWorkspaceFile() {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     DUMMY_STACK));
     assertThat(e).hasMessageThat().contains("cannot be in the WORKSPACE file");
@@ -230,6 +234,7 @@ public void testOutputFileNotEqualDot() {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     DUMMY_STACK));
     assertWithMessage(e.getMessage())