From 0287d927d92c0b0cb514e97120461b3df563c37f Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 26 Jan 2024 13:12:20 -0800 Subject: [PATCH] Evaluate macro implementation functions https://github.com/bazelbuild/bazel/commit/09eef85a7348d6cf2ddaaad007c1d57ae5a12570 added the ability to declare symbolic macros in packages, but did not run their implementation functions. This CL adds that feature. For now, implementation functions run synchronously with the call that instantiates the macro. The implementation runs in its own Starlark thread, but it has the side effect of mutating the original Package.Builder, so the targets that it declares are visible to native.existing_rules() in legacy macros. In the future we may make symbolic macro evaluation lazy, in which case the implementation function would run sometime after BUILD evaluation completes, and macro-generated targets would be invisible to native.existing_rules(). MacroClass.java: - add static helper executeMacroImplementation(). This is analogous to PackageFactory#executeBuildFileImpl(), but I prefer to not weigh that file down with additional code. SymbolicMacroTest.java: - tests for signature of implementation function - test for macro-generated targets. Note that the "my_macro$my_target" naming convention isn't enforced yet. - test for macro composability - test for calling existing_rules and glob in a symbolic macro. These will be inverted in a later CL when we ban them, by supplying a different class than Package.Builder to executeMacroImplementation(). Work toward #19922. PiperOrigin-RevId: 601848482 Change-Id: Ic70fdad9e7d8323c76a3f0125c518547adeee96f --- .../starlark/StarlarkRuleClassFunctions.java | 8 + .../build/lib/packages/MacroClass.java | 56 +++++ .../devtools/build/lib/packages/Package.java | 2 - .../build/lib/packages/SymbolicMacroTest.java | 194 ++++++++++++++++++ .../StarlarkRuleClassFunctionsTest.java | 8 +- 5 files changed, 262 insertions(+), 6 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.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 5d7324634f293b..fd470d0520f78b 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 @@ -1060,6 +1060,14 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg } catch (NameConflictException e) { throw new EvalException(e); } + + // TODO: #19922 - Instead of evaluating macros synchronously with their declaration, evaluate + // them lazily as the targets they declare are requested. But this would mean that targets + // declared in a symbolic macro are invisible to native.existing_rules() calls in a legacy + // macro. Therefore, this is blocked on either changing the semantics of existing_rules() or + // deprecating it entirely. + MacroClass.executeMacroImplementation(macroInstance, pkgBuilder, thread.getSemantics()); + return Starlark.NONE; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java index f8866bf1926451..a27e645105bac5 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java @@ -15,9 +15,16 @@ package com.google.devtools.build.lib.packages; import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; import com.google.errorprone.annotations.CanIgnoreReturnValue; import javax.annotation.Nullable; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Mutability; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkFunction; +import net.starlark.java.eval.StarlarkSemantics; +import net.starlark.java.eval.StarlarkThread; /** * Represents a symbolic macro, defined in a .bzl file, that may be instantiated during Package @@ -65,4 +72,53 @@ public MacroClass build() { return new MacroClass(name, implementation); } } + + /** + * Executes a symbolic macro's implementation function, in a new Starlark thread, mutating the + * given package under construction. + */ + // TODO: #19922 - Take a new type, PackagePiece.Builder, in place of Package.Builder. PackagePiece + // would represent the collection of targets/macros instantiated by expanding a single symbolic + // macro. + public static void executeMacroImplementation( + MacroInstance macro, Package.Builder builder, StarlarkSemantics semantics) + throws InterruptedException { + try (Mutability mu = + Mutability.create("macro", builder.getPackageIdentifier(), macro.getName())) { + StarlarkThread thread = new StarlarkThread(mu, semantics); + thread.setPrintHandler(Event.makeDebugPrintHandler(builder.getLocalEventHandler())); + + new BazelStarlarkContext( + BazelStarlarkContext.Phase.LOADING, + // TODO: #19922 - Technically we should use a different key than the one in the main + // BUILD thread, but that'll be fixed when we change the builder type to + // PackagePiece.Builder. + new SymbolGenerator<>(builder.getPackageIdentifier())) + .storeInThread(thread); + + // TODO: #19922 - Have Package.Builder inherit from BazelStarlarkContext and only store one + // thread-local object. + thread.setThreadLocal(Package.Builder.class, builder); + + // TODO: #19922 - If we want to support creating analysis_test rules inside symbolic macros, + // we'd need to call `thread.setThreadLocal(RuleDefinitionEnvironment.class, + // ruleClassProvider)`. In that case we'll need to consider how to get access to the + // ConfiguredRuleClassProvider. For instance, we could put it in the builder. + + try { + Starlark.fastcall( + thread, + macro.getMacroClass().getImplementation(), + /* positional= */ new Object[] {}, + /* named= */ new Object[] {"name", macro.getName()}); + } catch (EvalException ex) { + builder + .getLocalEventHandler() + .handle( + Package.error( + /* location= */ null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR)); + builder.setContainsErrors(); + } + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 5b052c075aed87..4ee7c9a670f99a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -1660,8 +1660,6 @@ void addRule(Rule rule) throws NameConflictException { public void addMacro(MacroInstance macro) throws NameConflictException { checkForExistingName(macro); macros.put(macro.getName(), macro); - // TODO(#19922): Push to a queue of unexpanded macros, read those when expanding macros as - // part of monolithic package evaluation (but not lazy macro evaluation). } void addRegisteredExecutionPlatforms(List platforms) { diff --git a/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java new file mode 100644 index 00000000000000..cb80376cc00998 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/packages/SymbolicMacroTest.java @@ -0,0 +1,194 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import javax.annotation.Nullable; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests the execution of symbolic macro implementations. */ +@RunWith(JUnit4.class) +public final class SymbolicMacroTest extends BuildViewTestCase { + + @Before + public void setUp() throws Exception { + setBuildLanguageOptions("--experimental_enable_first_class_macros"); + } + + /** + * Returns a package by the given name (no leading "//"), or null upon {@link + * NoSuchPackageException}. + */ + @CanIgnoreReturnValue + @Nullable + private Package getPackage(String pkgName) throws InterruptedException { + try { + return getPackageManager().getPackage(reporter, PackageIdentifier.createInMainRepo(pkgName)); + } catch (NoSuchPackageException unused) { + return null; + } + } + + private void assertPackageNotInError(@Nullable Package pkg) { + assertThat(pkg).isNotNull(); + assertThat(pkg.containsErrors()).isFalse(); + } + + @Test + public void implementationIsInvokedWithNameParam() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _impl(name):", + " print('my_macro called with name = %s' % name)", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "my_macro(name='abc')"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertContainsEvent("called with name = abc"); + } + + @Test + public void implementationFailsDueToBadSignature() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _impl():", + " pass", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "my_macro(name='abc')"); + + reporter.removeHandler(failFastHandler); + Package pkg = getPackage("pkg"); + assertThat(pkg).isNotNull(); + assertThat(pkg.containsErrors()).isTrue(); + assertContainsEvent("_impl() got unexpected keyword argument: name"); + } + + @Test + public void macroCanDeclareTargets() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _impl(name):", + " native.cc_library(name = name + '$lib')", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "my_macro(name='abc')"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(pkg.getTargets()).containsKey("abc$lib"); + } + + @Test + public void macroCanDeclareSubmacros() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _inner_impl(name):", + " native.cc_library(name = name + '$lib')", + "inner_macro = macro(implementation=_inner_impl)", + "def _impl(name):", + " inner_macro(name = name + '$inner')", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "my_macro(name='abc')"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertThat(pkg.getTargets()).containsKey("abc$inner$lib"); + } + + // TODO: #19922 - Invert this, symbolic macros shouldn't be able to call glob(). + @Test + public void macroCanCallGlob() throws Exception { + scratch.file("pkg/foo.txt"); + scratch.file( + "pkg/foo.bzl", // + "def _impl(name):", + " print('Glob result: %s' % native.glob(['foo*']))", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "my_macro(name='abc')"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertContainsEvent("Glob result: [\"foo.bzl\", \"foo.txt\"]"); + } + + // TODO: #19922 - Invert this, symbolic macros shouldn't be able to call existing_rules(). + @Test + public void macroCanCallExistingRules() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _impl(name):", + " native.cc_binary(name = name + '$lib')", + " print('existing_rules() keys: %s' % native.existing_rules().keys())", + "my_macro = macro(implementation=_impl)"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro')", + "cc_library(name = 'outer_target')", + "my_macro(name='abc')"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertContainsEvent("existing_rules() keys: [\"outer_target\", \"abc$lib\"]"); + } + + // TODO: #19922 - This behavior is necessary to preserve compatibility with use cases for + // native.existing_rules(), but it's a blocker for making symbolic macro evaluation lazy. + @Test + public void macroDeclaredTargetsAreVisibleToExistingRules() throws Exception { + scratch.file( + "pkg/foo.bzl", // + "def _impl(name):", + " native.cc_binary(name = name + '$lib')", + "my_macro = macro(implementation=_impl)", + "def query():", + " print('existing_rules() keys: %s' % native.existing_rules().keys())"); + scratch.file( + "pkg/BUILD", // + "load(':foo.bzl', 'my_macro', 'query')", + "cc_library(name = 'outer_target')", + "my_macro(name='abc')", + "query()"); + + Package pkg = getPackage("pkg"); + assertPackageNotInError(pkg); + assertContainsEvent("existing_rules() keys: [\"outer_target\", \"abc$lib\"]"); + } + + // TODO: #19922 - Add more test cases for interaction between macros and environment_group, + // package_group, implicit/explicit input files, and the package() function. But all of these + // behaviors are about to change (from allowed to prohibited). +} 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 67fab2c94af72c..9e48ef4d94fb16 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 @@ -212,7 +212,10 @@ public void testImplicitArgsAttribute() throws Exception { assertThat(getRuleClass("non_exec_rule").hasAttr("args", Type.STRING_LIST)).isFalse(); } - /** Returns a package by the given name (no leading "//"), or null if it was in error. */ + /** + * Returns a package by the given name (no leading "//"), or null upon {@link + * NoSuchPackageException}. + */ @CanIgnoreReturnValue @Nullable private Package getPackage(String pkgName) throws InterruptedException { @@ -376,9 +379,6 @@ public void testSymbolicMacro_macroFunctionApi() throws Exception { assertThat(ev.eval("repr(s.unexported)")).isEqualTo(""); } - // TODO(#19922): Add assertions for calling convention and execution of macro implementation - // function. - private RuleClass getRuleClass(String name) throws Exception { return ((StarlarkRuleFunction) ev.lookup(name)).getRuleClass(); }