From f1ee74d78cf2e6dbdbe0a8bb431676f678f57060 Mon Sep 17 00:00:00 2001 From: cparsons Date: Wed, 31 Jul 2019 07:09:53 -0700 Subject: [PATCH] Disallow field syntax to access a target's providers The provider-key syntax should be used instead. This is an incompatible change attached to new flag --incompatible_disable_target_provider_fields. See https://github.com/bazelbuild/bazel/issues/9014 for details. RELNOTES: New incompatible flag --incompatible_disable_target_provider_fields removes the ability (in Starlark) to access a target's providers via the field syntax (for example, `ctx.attr.dep.my_provider`). The provider-key syntax should be used instead (for example, `ctx.attr.dep[MyProvider]`). See https://github.com/bazelbuild/bazel/issues/9014 for details. PiperOrigin-RevId: 260920114 --- .../AbstractConfiguredTarget.java | 50 ++++++++-- .../build/lib/packages/SkylarkInfo.java | 8 ++ .../packages/StarlarkSemanticsOptions.java | 18 ++++ .../build/lib/syntax/DotExpression.java | 2 +- .../build/lib/syntax/SkylarkClassObject.java | 16 ++++ .../build/lib/syntax/StarlarkSemantics.java | 5 + .../SkylarkSemanticsConsistencyTest.java | 2 + .../lib/skylark/SkylarkIntegrationTest.java | 92 +++++++++++++++++++ 8 files changed, 185 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java index 3027a99344ab8d..799b6d1f9838ba 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java @@ -16,6 +16,7 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.DefaultInfo; @@ -36,20 +37,23 @@ import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; import com.google.devtools.build.lib.skylarkinterface.StarlarkContext; -import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; +import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Printer; +import com.google.devtools.build.lib.syntax.SkylarkClassObject; +import com.google.devtools.build.lib.syntax.SkylarkType; +import com.google.devtools.build.lib.syntax.StarlarkSemantics; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import javax.annotation.Nullable; /** - * An abstract implementation of ConfiguredTarget in which all properties are - * assigned trivial default values. + * An abstract implementation of ConfiguredTarget in which all properties are assigned trivial + * default values. */ public abstract class AbstractConfiguredTarget - implements ConfiguredTarget, VisibilityProvider, ClassObject { + implements ConfiguredTarget, VisibilityProvider, SkylarkClassObject { private final Label label; private final BuildConfigurationValue.Key configurationKey; @@ -62,6 +66,18 @@ public abstract class AbstractConfiguredTarget private static final String DATA_RUNFILES_FIELD = "data_runfiles"; private static final String DEFAULT_RUNFILES_FIELD = "default_runfiles"; + // A set containing all field names which may be specially handled (and thus may not be + // attributed to normal user-specified providers). + private static final ImmutableSet SPECIAL_FIELD_NAMES = + ImmutableSet.of( + LABEL_FIELD, + FILES_FIELD, + DEFAULT_RUNFILES_FIELD, + DATA_RUNFILES_FIELD, + FilesToRunProvider.SKYLARK_NAME, + OutputGroupInfo.SKYLARK_NAME, + RuleConfiguredTarget.ACTIONS_FIELD_NAME); + public AbstractConfiguredTarget(Label label, BuildConfigurationValue.Key configurationKey) { this(label, configurationKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER)); } @@ -105,11 +121,34 @@ public

P getProvider(Class

provider) { } } + @Override + public Object getValue(Location loc, StarlarkSemantics semantics, String name) + throws EvalException { + if (semantics.incompatibleDisableTargetProviderFields() + && !SPECIAL_FIELD_NAMES.contains(name)) { + throw new EvalException( + loc, + "Accessing providers via the field syntax on structs is " + + "deprecated and will be removed soon. It may be temporarily re-enabled by setting " + + "--incompatible_disable_target_provider_fields=false. See " + + "https://github.com/bazelbuild/bazel/issues/9014 for details."); + } + return getValue(name); + } + @Override public Object getValue(String name) { switch (name) { case LABEL_FIELD: return getLabel(); + case RuleConfiguredTarget.ACTIONS_FIELD_NAME: + // Depending on subclass, the 'actions' field will either be unsupported or of type + // java.util.List, which needs to be converted to SkylarkList before being returned. + Object result = get(name); + if (result != null) { + result = SkylarkType.convertToSkylark(result, (Mutability) null); + } + return result; default: return get(name); } @@ -207,9 +246,6 @@ public String getRuleClassString() { */ @Override public final Object get(String providerKey) { - if (OutputGroupInfo.SKYLARK_NAME.equals(providerKey)) { - return get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); - } switch (providerKey) { case FILES_FIELD: return getDefaultProvider().getFiles(); diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java index 0837b1f5d83b71..c18438fdc9c544 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.SkylarkClassObject; import com.google.devtools.build.lib.syntax.SkylarkType; +import com.google.devtools.build.lib.syntax.StarlarkSemantics; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -97,6 +98,13 @@ public boolean isCompact() { return getLayout() != null; } + @Override + public Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name) + throws EvalException { + // By default, a SkylarkInfo's field values are not affected by the Starlark semantics. + return getValue(name); + } + /** * Creates a schemaless (map-based) provider instance with the given provider type and field * values. diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 65c65e47461166..f588a4e22460e0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -232,6 +232,23 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "convert to a list.") public boolean incompatibleDepsetIsNotIterable; + @Option( + name = "incompatible_disable_target_provider_fields", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If set to true, disable the ability to access providers on 'target' objects via field " + + "syntax. Use provider-key syntax instead. For example, instead of using " + + "`ctx.attr.dep.my_info` to access `my_info` from inside a rule implementation " + + "function, use `ctx.attr.dep[MyInfo]`. See " + + "https://github.com/bazelbuild/bazel/issues/9014 for details.") + public boolean incompatibleDisableTargetProviderFields; + @Option( name = "incompatible_disable_deprecated_attr_params", defaultValue = "true", @@ -702,6 +719,7 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleBzlDisallowLoadAfterStatement(incompatibleBzlDisallowLoadAfterStatement) .incompatibleDepsetIsNotIterable(incompatibleDepsetIsNotIterable) .incompatibleDepsetUnion(incompatibleDepsetUnion) + .incompatibleDisableTargetProviderFields(incompatibleDisableTargetProviderFields) .incompatibleDisableThirdPartyLicenseChecking( incompatibleDisableThirdPartyLicenseChecking) .incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java index 07c00f10e84894..64701b5ef7d1e9 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java @@ -133,7 +133,7 @@ public static Object eval(Object objValue, String name, if (objValue instanceof SkylarkClassObject) { try { - return ((SkylarkClassObject) objValue).getValue(name); + return ((SkylarkClassObject) objValue).getValue(loc, env.getSemantics(), name); } catch (IllegalArgumentException ex) { throw new EvalException(loc, ex); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java index 03c9f88208fac1..64e6fc6b04c246 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java @@ -13,6 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.syntax; +import com.google.devtools.build.lib.events.Location; +import javax.annotation.Nullable; + /** * A marker interface for a {@link ClassObject} whose {@link #getValue} always returns a * Skylark-friendly value, with no defensive conversion required. @@ -26,4 +29,17 @@ * Skylark-friendly values. */ public interface SkylarkClassObject extends ClassObject { + + /** + * Returns the value of the field with the given name, or null if the field does not exist. + * + * @param loc the location in the current Starlark evaluation context + * @param starlarkSemantics the current starlark semantics, which may affect which fields are + * available, or the semantics of the available fields + * @param name the name of the field to return the value of + * @throws EvalException if a user-visible error occurs (other than non-existent field). + */ + @Nullable + Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name) + throws EvalException; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 7527c6c9273c6e..c3e1143d0f42fd 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -148,6 +148,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleDepsetUnion(); + public abstract boolean incompatibleDisableTargetProviderFields(); + public abstract boolean incompatibleDisableThirdPartyLicenseChecking(); public abstract boolean incompatibleDisableDeprecatedAttrParams(); @@ -263,6 +265,7 @@ public static Builder builderWithDefaults() { .incompatibleBzlDisallowLoadAfterStatement(true) .incompatibleDepsetIsNotIterable(true) .incompatibleDepsetUnion(true) + .incompatibleDisableTargetProviderFields(false) .incompatibleDisableThirdPartyLicenseChecking(true) .incompatibleDisableDeprecatedAttrParams(true) .incompatibleDisableObjcProviderResources(true) @@ -331,6 +334,8 @@ public abstract static class Builder { public abstract Builder incompatibleDepsetUnion(boolean value); + public abstract Builder incompatibleDisableTargetProviderFields(boolean value); + public abstract Builder incompatibleDisableThirdPartyLicenseChecking(boolean value); public abstract Builder incompatibleDisableDeprecatedAttrParams(boolean value); diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index e77ed2d97f5cd4..a15435486ceedf 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -138,6 +138,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_depset_for_libraries_to_link_getter=" + rand.nextBoolean(), "--incompatible_depset_is_not_iterable=" + rand.nextBoolean(), "--incompatible_depset_union=" + rand.nextBoolean(), + "--incompatible_disable_target_provider_fields=" + rand.nextBoolean(), "--incompatible_disable_deprecated_attr_params=" + rand.nextBoolean(), "--incompatible_disable_objc_provider_resources=" + rand.nextBoolean(), "--incompatible_disable_third_party_license_checking=" + rand.nextBoolean(), @@ -196,6 +197,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleDepsetForLibrariesToLinkGetter(rand.nextBoolean()) .incompatibleDepsetIsNotIterable(rand.nextBoolean()) .incompatibleDepsetUnion(rand.nextBoolean()) + .incompatibleDisableTargetProviderFields(rand.nextBoolean()) .incompatibleDisableDeprecatedAttrParams(rand.nextBoolean()) .incompatibleDisableObjcProviderResources(rand.nextBoolean()) .incompatibleDisableThirdPartyLicenseChecking(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 38f7a3a89b3c0e..11b0f1021f0d55 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -2908,6 +2908,98 @@ public void testDisallowStructProviderSyntax() throws Exception { + "--incompatible_disallow_struct_provider_syntax=false"); } + @Test + public void testDisableTargetProviderFields() throws Exception { + setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=true"); + scratch.file( + "test/skylark/rule.bzl", + "MyProvider = provider()", + "", + "def _my_rule_impl(ctx): ", + " print(ctx.attr.dep.my_info)", + "def _dep_rule_impl(ctx): ", + " my_info = MyProvider(foo = 'bar')", + " return struct(my_info = my_info, providers = [my_info])", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + " 'dep': attr.label(),", + " })", + "dep_rule = rule(implementation = _dep_rule_impl)"); + scratch.file( + "test/skylark/BUILD", + "load(':rule.bzl', 'my_rule', 'dep_rule')", + "", + "my_rule(name = 'r', dep = ':d')", + "dep_rule(name = 'd')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test/skylark:r"); + assertContainsEvent( + "Accessing providers via the field syntax on structs is deprecated and will be removed " + + "soon. It may be temporarily re-enabled by setting " + + "--incompatible_disable_target_provider_fields=false. " + + "See https://github.com/bazelbuild/bazel/issues/9014 for details."); + } + + // Verifies that non-provider fields on the 'target' type are still available even with + // --incompatible_disable_target_provider_fields. + @Test + public void testDisableTargetProviderFields_actionsField() throws Exception { + setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=true"); + scratch.file( + "test/skylark/rule.bzl", + "MyProvider = provider()", + "", + "def _my_rule_impl(ctx): ", + " print(ctx.attr.dep.actions)", + "def _dep_rule_impl(ctx): ", + " my_info = MyProvider(foo = 'bar')", + " return struct(my_info = my_info, providers = [my_info])", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + " 'dep': attr.label(),", + " })", + "dep_rule = rule(implementation = _dep_rule_impl)"); + scratch.file( + "test/skylark/BUILD", + "load(':rule.bzl', 'my_rule', 'dep_rule')", + "", + "my_rule(name = 'r', dep = ':d')", + "dep_rule(name = 'd')"); + + assertThat(getConfiguredTarget("//test/skylark:r")).isNotNull(); + } + + @Test + public void testDisableTargetProviderFields_disabled() throws Exception { + setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=false"); + scratch.file( + "test/skylark/rule.bzl", + "MyProvider = provider()", + "", + "def _my_rule_impl(ctx): ", + " print(ctx.attr.dep.my_info)", + "def _dep_rule_impl(ctx): ", + " my_info = MyProvider(foo = 'bar')", + " return struct(my_info = my_info, providers = [my_info])", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + " 'dep': attr.label(),", + " })", + "dep_rule = rule(implementation = _dep_rule_impl)"); + scratch.file( + "test/skylark/BUILD", + "load(':rule.bzl', 'my_rule', 'dep_rule')", + "", + "my_rule(name = 'r', dep = ':d')", + "dep_rule(name = 'd')"); + + assertThat(getConfiguredTarget("//test/skylark:r")).isNotNull(); + } + @Test public void testNoRuleOutputsParam() throws Exception { setSkylarkSemanticsOptions("--incompatible_no_rule_outputs_param=true");