From 4686e2550f76189cb1cfbe0852feb3337f38139c Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 4 Sep 2023 04:03:52 -0700 Subject: [PATCH] Always fail on unknown attributes Fixes: https://github.com/bazelbuild/bazel/issues/11000 RELNOTES[INC]: Fails on unknown attributes (even when set to None) PiperOrigin-RevId: 562519157 Change-Id: If5e430c73485c8ae9661f4231692384a057f37d5 --- .../starlark/StarlarkRuleClassFunctions.java | 4 ++ .../bazel/bzlmod/BzlmodRepoRuleCreator.java | 2 +- .../build/lib/packages/RuleClass.java | 18 ++++-- .../build/lib/packages/RuleFactory.java | 18 +++++- .../lib/packages/WorkspaceFactoryHelper.java | 6 +- .../semantics/BuildLanguageOptions.java | 13 +++++ .../analysis/RuleConfiguredTargetTest.java | 58 +++++++++++++++++++ .../build/lib/packages/RuleClassTest.java | 1 + .../build/lib/packages/RuleFactoryTest.java | 5 ++ 9 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 2931ff22c5aa49..31fee62e812e4f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/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 kwarg pkgContext.getBuilder(), ruleClass, attributeValues, + thread + .getSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES), pkgContext.getEventHandler(), thread.getCallStack()); } catch (InvalidRuleException | NameConflictException e) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java index 98bc35e22a045b..64501f92c8a370 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java +++ b/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); 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 ff2aaf6349b40e..58a43ce9629b1d 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 @@ -1982,11 +1982,13 @@ Rule createRule( Package.Builder pkgBuilder, Label ruleLabel, AttributeValues attributeValues, + boolean failOnUnknownAttributes, EventHandler eventHandler, List 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 @@ 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 void populateRuleAttributeValues( Rule rule, Package.Builder pkgBuilder, AttributeValues attributeValues, + boolean failOnUnknownAttributes, EventHandler eventHandler) throws InterruptedException, CannotPrecomputeDefaultsException { @@ -2033,6 +2036,7 @@ private void populateRuleAttributeValues( rule, pkgBuilder.getLabelConverter(), attributeValues, + failOnUnknownAttributes, pkgBuilder.getListInterner(), eventHandler); populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler); @@ -2055,6 +2059,7 @@ private BitSet populateDefinedRuleAttributeValues( Rule rule, LabelConverter labelConverter, AttributeValues attributeValues, + boolean failOnUnknownAttributes, Interner> listInterner, EventHandler eventHandler) { BitSet definedAttrIndices = new BitSet(); @@ -2062,7 +2067,7 @@ private BitSet populateDefinedRuleAttributeValues( 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 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; } 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 1921bf8336fb73..70de94ebc795bd 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 @@ -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 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 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 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) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java index 9b5d7d6d045ba6..1f514a4f43cbcc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java +++ b/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); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 03cc60980469a2..0d489866b9c7e8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/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 EXPERIMENTAL_BUILTINS_BZL_PATH = diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java index 7bf921deae72b7..8d0b97abfb3519 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java +++ b/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"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index be0948ef4761a9..bdfeda891fa0d0 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java @@ -933,6 +933,7 @@ private Rule createRule(RuleClass ruleClass, String name, Map at pkgBuilder, ruleLabel, new BuildLangTypedAttributeValuesMap(attributeValues), + true, reporter, ImmutableList.of( StarlarkThread.callStackEntry(StarlarkThread.TOP_LEVEL, testRuleLocation))); diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java index 00d55bf10af804..a8173533122141 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java +++ b/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())