From aefb396d9217d9ef1698f8705ebafe3d337f44b7 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 12 Jan 2024 10:10:45 -0800 Subject: [PATCH 1/4] Fix dict types in initializers PiperOrigin-RevId: 597874318 Change-Id: I7d7135be863c2123242951635af5c10d370c7ef2 --- .../devtools/build/lib/packages/Type.java | 17 ++++- .../StarlarkRuleClassFunctionsTest.java | 68 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java index 54577d36082d7e..697376891c9964 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Type.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Type.java @@ -606,9 +606,24 @@ public Map convert(Object x, Object what, LabelConverter labelConv @Override public Object copyAndLiftStarlarkValue( Object x, Object what, @Nullable LabelConverter labelConverter) throws ConversionException { - return Dict.immutableCopyOf(convert(x, what, labelConverter)); + if (!(x instanceof Map)) { + throw new ConversionException(this, x, what); + } + Map o = (Map) x; + // It's possible that #convert() calls transform non-equal keys into equal ones so we can't + // just use ImmutableMap.Builder() here (that throws on collisions). + LinkedHashMap result = new LinkedHashMap<>(); + for (Map.Entry elem : o.entrySet()) { + result.put( + keyType.copyAndLiftStarlarkValue(elem.getKey(), "dict key element", labelConverter), + valueType.copyAndLiftStarlarkValue( + elem.getValue(), "dict value element", labelConverter)); + } + return Dict.immutableCopyOf(result); } + + @Override public Map concat(Iterable> iterable) { Dict.Builder output = new Dict.Builder<>(); diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index d12add6d7f643b..06007b9b52e4fa 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -82,6 +82,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -2821,6 +2822,73 @@ public void initializer_basic() throws Exception { assertThat((List) info.getValue("deps")).containsExactly("@@//:initial", "@@//:added"); } + @Test + @SuppressWarnings("unchecked") + public void initializer_stringListDict() throws Exception { + scratch.file( + "initializer_testing/b.bzl", + "def initializer(**kwargs):", + " return {}", + "MyInfo = provider()", + "def impl(ctx): ", + " return [MyInfo(dict = ctx.attr.dict)]", + "my_rule = rule(impl,", + " initializer = initializer,", + " attrs = {", + " 'dict': attr.string_list_dict(),", + " })"); + scratch.file( + "initializer_testing/BUILD", // + "load(':b.bzl','my_rule')", + "my_rule(name = 'my_target', dict = {'k': ['val']})"); + + ConfiguredTarget myTarget = getConfiguredTarget("//initializer_testing:my_target"); + StructImpl info = + (StructImpl) + myTarget.get( + new StarlarkProvider.Key( + Label.parseCanonical("//initializer_testing:b.bzl"), "MyInfo")); + + assertThat(((Map>) info.getValue("dict")).keySet()).containsExactly("k"); + assertThat(((Map>) info.getValue("dict")).get("k")).containsExactly("val"); + } + + @Test + @SuppressWarnings("unchecked") + public void initializer_labelKeyedStringDict() throws Exception { + scratch.file( + "BUILD", // + "filegroup(name = 'key')"); + scratch.file( + "initializer_testing/b.bzl", + "def initializer(**kwargs):", + " return {}", + "MyInfo = provider()", + "def impl(ctx): ", + " return [MyInfo(dict = ctx.attr.dict)]", + "my_rule = rule(impl,", + " initializer = initializer,", + " attrs = {", + " 'dict': attr.label_keyed_string_dict(),", + " })"); + scratch.file( + "initializer_testing/BUILD", // + "load(':b.bzl','my_rule')", + "my_rule(name = 'my_target', dict = {'//:key': 'val'})"); + + ConfiguredTarget myTarget = getConfiguredTarget("//initializer_testing:my_target"); + ConfiguredTarget key = getConfiguredTarget("//:key"); + StructImpl info = + (StructImpl) + myTarget.get( + new StarlarkProvider.Key( + Label.parseCanonical("//initializer_testing:b.bzl"), "MyInfo")); + + assertThat(((Map) info.getValue("dict")).keySet()) + .containsExactly(key); + assertThat(((Map) info.getValue("dict")).get(key)).isEqualTo("val"); + } + @Test public void initializer_legacyAnyType() throws Exception { scratch.file( From 00914f8b2d296e065f640a0f6e4287875e4288ec Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 23 Jan 2024 11:55:58 -0800 Subject: [PATCH 2/4] Allow reading the name of the target in the initializer PiperOrigin-RevId: 600862478 Change-Id: Iadc38abea17f4b3ec33a8f76245c75ae3aca6d1d --- .../starlark/StarlarkRuleClassFunctions.java | 8 +- .../StarlarkRuleFunctionsApi.java | 4 +- .../StarlarkRuleClassFunctionsTest.java | 79 ++++++++++++++----- 3 files changed, 70 insertions(+), 21 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 31488ce1f47a1c..a0b517fa548a4a 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 @@ -1071,7 +1071,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg // The less magic the better. Do not give in those temptations! Dict.Builder initializerKwargs = Dict.builder(); for (var attr : currentRuleClass.getAttributes()) { - if (attr.isPublic() && attr.starlarkDefined()) { + if ((attr.isPublic() && attr.starlarkDefined()) || attr.getName().equals("name")) { if (kwargs.containsKey(attr.getName())) { Object value = kwargs.get(attr.getName()); if (value == Starlark.NONE) { @@ -1101,6 +1101,12 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg : Dict.cast(ret, String.class, Object.class, "rule's initializer return value"); for (var arg : newKwargs.keySet()) { + if (arg.equals("name")) { + if (!kwargs.get("name").equals(newKwargs.get("name"))) { + throw Starlark.errorf("Initializer can't change the name of the target"); + } + continue; + } checkAttributeName(arg); if (arg.startsWith("_")) { // allow setting private attributes from initializers in builtins diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java index 1e402377349296..89c18897a2fbbe 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java @@ -466,8 +466,8 @@ Object provider(Object doc, Object fields, Object init, StarlarkThread thread) doc = "Experimental: the Stalark function initializing the attributes of the rule. " + "

The function is called at load time for each instance of the rule. It's " - + "called with values of public attributes defined by the rule (not with " - + "generic attributes, for example name or tags). " + + "called with name and the values of public attributes defined by" + + "the rule (not with generic attributes, for example tags). " + "

It has to return a dictionary from the attribute names to the desired " + "values. The attributes that are not returned are unaffected. Returning " + "None as value results in using the default value specified in " diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index 06007b9b52e4fa..1ced7947625c11 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -2794,7 +2794,7 @@ public void initializer_basic() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " return {'deps': deps + ['//:added']}", "def impl(ctx): ", " return [MyInfo(", @@ -2822,6 +2822,49 @@ public void initializer_basic() throws Exception { assertThat((List) info.getValue("deps")).containsExactly("@@//:initial", "@@//:added"); } + @Test + public void initializer_nameUnchanged() throws Exception { + scratch.file( + "initializer_testing/b.bzl", + "def initializer(name, **kwargs):", + " if name != 'my_target':", + " fail()", + " return {'name': name} | kwargs", + "MyInfo = provider()", + "def impl(ctx): ", + " pass", + "my_rule = rule(impl, initializer = initializer)"); + scratch.file( + "initializer_testing/BUILD", // + "load(':b.bzl','my_rule')", + "my_rule(name = 'my_target')"); + + getConfiguredTarget("//initializer_testing:my_target"); + + assertNoEvents(); + } + + @Test + public void initializer_nameChanged() throws Exception { + scratch.file( + "initializer_testing/b.bzl", + "def initializer(name, **kwargs):", + " return {'name': 'my_new_name'}", + "def impl(ctx): ", + " pass", + "my_rule = rule(impl, initializer = initializer)"); + scratch.file( + "initializer_testing/BUILD", // + "load(':b.bzl','my_rule')", + "my_rule(name = 'my_target')"); + + reporter.removeHandler(failFastHandler); + reporter.addHandler(ev.getEventCollector()); + getConfiguredTarget("//initializer_testing:my_target"); + + ev.assertContainsError("Error in my_rule: Initializer can't change the name of the target"); + } + @Test @SuppressWarnings("unchecked") public void initializer_stringListDict() throws Exception { @@ -2894,7 +2937,7 @@ public void initializer_legacyAnyType() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(tristate = -1):", + "def initializer(name, tristate = -1):", " return {'tristate': int(tristate)}", "def impl(ctx): ", " return [MyInfo(tristate = ctx.attr.tristate)]", @@ -2954,7 +2997,7 @@ public void initializer_withSelect() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(srcs = []):", + "def initializer(name, srcs = []):", " return {'srcs': srcs + ['b.ml']}", "def impl(ctx): ", " return [MyInfo(", @@ -3014,7 +3057,7 @@ public void initializer_overridesAttributeDefault() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(deps = ['//:initializer_default']):", + "def initializer(name, deps = ['//:initializer_default']):", " return {'deps': deps}", "def impl(ctx): ", " return [MyInfo(", @@ -3049,7 +3092,7 @@ public void initializer_returningNoneSetsDefault() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(deps = ['//:initializer_default']):", + "def initializer(name, deps = ['//:initializer_default']):", " return {'deps': None}", "def impl(ctx): ", " return [MyInfo(", @@ -3079,7 +3122,7 @@ public void initializer_omittedValueIsNotPassed() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(srcs):", + "def initializer(name, srcs):", " return {'srcs': srcs}", "def impl(ctx): ", " pass", @@ -3106,7 +3149,7 @@ public void initializer_noneValueIsNotPassed() throws Exception { scratch.file( "initializer_testing/b.bzl", "MyInfo = provider()", - "def initializer(srcs):", + "def initializer(name, srcs):", " return {'srcs': srcs}", "def impl(ctx): ", " pass", @@ -3131,7 +3174,7 @@ public void initializer_noneValueIsNotPassed() throws Exception { public void initializer_incorrectReturnType() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = []):", + "def initializer(name, srcs = []):", " return [srcs]", "def impl(ctx): ", " pass", @@ -3156,7 +3199,7 @@ public void initializer_incorrectReturnType() throws Exception { public void initializer_incorrectReturnDicts() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = []):", + "def initializer(name, srcs = []):", " return {True: srcs}", "def impl(ctx): ", " pass", @@ -3182,7 +3225,7 @@ public void initializer_failsSettingBaseAttribute() throws Exception { // 'args' is an attribute defined for all executable rules scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " return {'srcs': srcs, 'deps': deps, 'args': ['a']}", "def impl(ctx): ", " pass", @@ -3209,7 +3252,7 @@ public void initializer_failsSettingBaseAttribute() throws Exception { public void initializer_failsSettingPrivateAttribute_outsideBuiltins() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " return {'srcs': srcs, '_tool': ':my_tool'}", "def impl(ctx): ", " pass", @@ -3240,7 +3283,7 @@ public void initializer_settingPrivateAttribute_insideBuiltins() throws Exceptio scratch.file("initializer_testing/builtins/BUILD", "filegroup(name='my_tool')"); scratch.file( "initializer_testing/builtins/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " return {'srcs': srcs, '_tool': ':my_tool'}", "MyInfo = provider()", "def impl(ctx): ", @@ -3271,7 +3314,7 @@ public void initializer_settingPrivateAttribute_insideBuiltins() throws Exceptio public void initializer_failsSettingUnknownAttr() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " return {'srcs': srcs, 'my_deps': deps}", "def impl(ctx): ", " pass", @@ -3297,7 +3340,7 @@ public void initializer_failsSettingUnknownAttr() throws Exception { public void initializer_failsCreatingAnotherRule() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " native.java_library(name = 'jl', srcs = ['a.java'])", " return {'srcs': srcs, 'deps': deps}", "def impl(ctx): ", @@ -3325,7 +3368,7 @@ public void initializer_failsCreatingAnotherRule() throws Exception { public void initializer_failsWithExistingRules() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " native.existing_rules()", " return {'srcs': srcs, 'deps': deps}", "def impl(ctx): ", @@ -3352,7 +3395,7 @@ public void initializer_failsWithExistingRules() throws Exception { public void initializer_withFails() throws Exception { scratch.file( "initializer_testing/b.bzl", - "def initializer(srcs = [], deps = []):", + "def initializer(name, srcs = [], deps = []):", " fail('Fail called in initializer')", " return {'srcs': srcs, 'deps': deps}", "def impl(ctx): ", @@ -3481,7 +3524,7 @@ public void extendRule_withInitializers() throws Exception { scratch.file( "extend_rule_testing/parent/parent.bzl", "ParentInfo = provider()", - "def _parent_initializer(srcs, deps):", // only parents attributes + "def _parent_initializer(name, srcs, deps):", // only parents attributes " return {'deps': deps + ['//extend_rule_testing:parent_dep']}", "def _impl(ctx):", " return [ParentInfo()]", @@ -3498,7 +3541,7 @@ public void extendRule_withInitializers() throws Exception { "extend_rule_testing/child.bzl", "load('//extend_rule_testing/parent:parent.bzl', 'parent_library')", "ChildInfo = provider()", - "def _child_initializer(srcs, deps, runtime_deps = []):", + "def _child_initializer(name, srcs, deps, runtime_deps = []):", " return {'deps': deps + [':child_dep'], 'runtime_deps': runtime_deps + [':runtime_dep']}", "def _impl(ctx):", " return ctx.super() + [ChildInfo(", From f9534d3a63083f3f28ba7894c9b879a8362f7537 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 7 Feb 2024 13:39:04 +0100 Subject: [PATCH 3/4] Make Google proto_library and extension of Bazel proto_library PiperOrigin-RevId: 580146223 Change-Id: I13ac76086d56cd26ae0c661d1c36c45a2ee270d1 --- .../starlark/StarlarkRuleClassFunctions.java | 4 ++- .../starlark/StarlarkRuleContext.java | 4 +-- .../proto/ProtoCompileActionBuilder.java | 25 ------------------- .../build/lib/rules/proto/ProtoConstants.java | 2 +- .../starlark/builtins_bzl/bazel/exports.bzl | 2 ++ .../starlark/builtins_bzl/common/exports.bzl | 2 -- .../common/proto/proto_library.bzl | 10 +++----- .../common/proto/proto_semantics.bzl | 10 -------- .../rules/proto/BazelProtoLibraryTest.java | 4 +-- 9 files changed, 14 insertions(+), 49 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java 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 a0b517fa548a4a..e86add34fa34d1 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 @@ -516,7 +516,9 @@ public static StarlarkRuleFunction createRule( } // Verify the child against parent's allowlist - if (parent != null && parent.getExtendableAllowlist() != null) { + if (parent != null + && parent.getExtendableAllowlist() != null + && !bzlFile.getRepository().getNameWithAt().equals("@_builtins")) { builder.addAllowlistChecker(EXTEND_RULE_ALLOWLIST_CHECKER); Attribute.Builder