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.util.FileType;
@@ -799,6 +800,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.getSemantics(),
             thread.getCallStack());

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

@@ -76,7 +76,7 @@ public static Rule createRule(
     try {
       rule =
           RuleFactory.createAndAddRule(
-              packageBuilder, ruleClass, attributeValues, eventHandler, semantics, callStack);
+              packageBuilder, ruleClass, attributeValues, true, eventHandler, semantics, 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/PackageFactory.java

@@ -414,6 +414,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.getSemantics(),
             thread.getCallStack());

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

@@ -2053,14 +2053,15 @@ <T> Rule createRule(
       Package.Builder pkgBuilder,
       Label ruleLabel,
       AttributeValues<T> attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       Location location,
       List<StarlarkThread.CallStackEntry> callstack)
       throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
     Rule rule =
         pkgBuilder.createRule(
             ruleLabel, this, location, callstack, AttributeContainer.newMutableInstance(this));
-    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, failOnUnknownAttributes, eventHandler);
     checkAspectAllowedValues(rule, eventHandler);
     rule.populateOutputFiles(eventHandler, pkgBuilder);
     checkForDuplicateLabels(rule, eventHandler);
@@ -2085,7 +2086,7 @@ <T> Rule createRuleUnchecked(
       throws InterruptedException, CannotPrecomputeDefaultsException {
     Rule rule =
         pkgBuilder.createRule(ruleLabel, this, location, callstack, implicitOutputsFunction);
-    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, NullEventHandler.INSTANCE);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, true, NullEventHandler.INSTANCE);
     rule.populateOutputFilesUnchecked(pkgBuilder);
     return rule;
   }
@@ -2101,6 +2102,7 @@ private <T> void populateRuleAttributeValues(
       Rule rule,
       Package.Builder pkgBuilder,
       AttributeValues<T> attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler)
       throws InterruptedException, CannotPrecomputeDefaultsException {
 
@@ -2109,6 +2111,7 @@ private <T> void populateRuleAttributeValues(
             rule,
             pkgBuilder.getLabelConverter(),
             attributeValues,
+            failOnUnknownAttributes,
             pkgBuilder.getListInterner(),
             eventHandler);
     populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler);
@@ -2131,14 +2134,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;
       }
 
@@ -2160,10 +2164,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

@@ -69,6 +69,7 @@ public static Rule createRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       StarlarkSemantics semantics,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
@@ -111,6 +112,7 @@ public static Rule createRule(
           pkgBuilder,
           label,
           generator.attributes,
+          failOnUnknownAttributes,
           eventHandler,
           generator.location, // see b/23974287 for rationale
           callstack);
@@ -143,12 +145,13 @@ public static Rule createAndAddRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
+      boolean failOnUnknownAttributes,
       EventHandler eventHandler,
       StarlarkSemantics semantics,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, NameConflictException, InterruptedException {
     Rule rule =
-        createRule(pkgBuilder, ruleClass, attributeValues, eventHandler, semantics, callstack);
+        createRule(pkgBuilder, ruleClass, attributeValues, failOnUnknownAttributes, eventHandler, semantics, callstack);
     pkgBuilder.addRule(rule);
     return rule;
   }

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

@@ -48,7 +48,7 @@ public static Rule createAndAddRepositoryRule(
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
     Rule rule =
-        RuleFactory.createRule(pkg, ruleClass, attributeValues, eventHandler, semantics, callstack);
+        RuleFactory.createRule(pkg, ruleClass, attributeValues, true, eventHandler, semantics, callstack);
     pkg.addEvents(eventHandler.getEvents());
     pkg.addPosts(eventHandler.getPosts());
     overwriteRule(pkg, rule);
@@ -166,7 +166,7 @@ static void addBindRule(
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);
     Rule rule =
-        RuleFactory.createRule(pkg, bindRuleClass, attributeValues, handler, semantics, callstack);
+        RuleFactory.createRule(pkg, bindRuleClass, attributeValues, true, handler, semantics, callstack);
     overwriteRule(pkg, rule);
     rule.setVisibility(ConstantRuleVisibility.PUBLIC);
   }

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

@@ -652,6 +652,16 @@ public final class BuildLanguageOptions extends OptionsBase {
               + " specified through features configuration.")
   public boolean experimentalGetFixedConfiguredEnvironment;
 
+  // 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
@@ -743,6 +753,7 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV,
                 experimentalGetFixedConfiguredEnvironment)
+            .setBool(INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES, incompatibleFailOnUnknownAttributes)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -831,6 +842,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-incompatible_disable_starlark_host_transitions";
   public static final String EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV =
       "-experimental_get_fixed_configured_action_env";
+  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/BUILD

@@ -9,13 +9,13 @@ filegroup(
     name = "srcs",
     testonly = 0,
     srcs = glob(["**"]) + [
+        "//src/test/java/com/google/devtools/build/lib/analysis/allowlisting:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/extra:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/mock:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/platform:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/starlark/annotations/processor:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/testing:srcs",
         "//src/test/java/com/google/devtools/build/lib/analysis/util:srcs",
-        "//src/test/java/com/google/devtools/build/lib/analysis/allowlisting:srcs",
     ],
     visibility = ["//src:__subpackages__"],
 )
@@ -348,6 +348,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:required_config_fragments_provider",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//third_party:guava",

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

@@ -17,7 +17,9 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -435,4 +437,62 @@ public void testRulesDontProvideRequiredFragmentsByDefault() throws Exception {
     assertThat(getConfiguredTarget("//a:config").getProvider(RequiredConfigFragmentsProvider.class))
         .isNull();
   }
+
+  @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

@@ -958,6 +958,7 @@ private Rule createRule(
         pkgBuilder,
         ruleLabel,
         new BuildLangTypedAttributeValuesMap(attributeValues),
+        true,
         reporter,
         location,
         callstack);

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

@@ -84,6 +84,7 @@ public void testCreateRule() throws Exception {
             pkgBuilder,
             ruleClass,
             new BuildLangTypedAttributeValuesMap(attributeValues),
+            true,
             new Reporter(new EventBus()),
             StarlarkSemantics.DEFAULT,
             DUMMY_STACK);
@@ -144,6 +145,7 @@ public void testCreateWorkspaceRule() throws Exception {
             pkgBuilder,
             ruleClass,
             new BuildLangTypedAttributeValuesMap(attributeValues),
+            true,
             new Reporter(new EventBus()),
             StarlarkSemantics.DEFAULT,
             DUMMY_STACK);
@@ -168,6 +170,7 @@ public void testWorkspaceRuleFailsInBuildFile() throws Exception {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     StarlarkSemantics.DEFAULT,
                     DUMMY_STACK));
@@ -192,6 +195,7 @@ public void testBuildRuleFailsInWorkspaceFile() throws Exception {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     StarlarkSemantics.DEFAULT,
                     DUMMY_STACK));
@@ -228,6 +232,7 @@ public void testOutputFileNotEqualDot() throws Exception {
                     pkgBuilder,
                     ruleClass,
                     new BuildLangTypedAttributeValuesMap(attributeValues),
+                    true,
                     new Reporter(new EventBus()),
                     StarlarkSemantics.DEFAULT,
                     DUMMY_STACK));