From 6e1103dedf71fc257dd5003f90f666c351dd7d99 Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Wed, 13 Nov 2024 08:53:20 -0800
Subject: [PATCH] Disallow symbolic macros from returning a non-None value

Previously, it was a no-op for a symbolic macro implementation function to return a value. This CL makes it an evaluation error. (An explicit `return` or `return <expr that evals to None>` statement is still permitted.)

This is useful to enforce code clarity for symbolic macros. It may also help catch errors sooner when migrating legacy macros to symbolic macros. It also leaves the door open to having Bazel interpret the return values of macros in specific ways in the future.

Fixes #24312.

PiperOrigin-RevId: 696153298
Change-Id: Ib317871664c2036cc24ea31bc0033ec691fd07c8
---
 .../build/lib/packages/MacroClass.java        | 18 ++++++++++++------
 .../build/lib/analysis/SymbolicMacroTest.java | 19 +++++++++++++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

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 72d50a3bac7303..c4c66fbb713dba 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
@@ -411,12 +411,18 @@ public static void executeMacroImplementation(
       MacroFrame childMacroFrame = new MacroFrame(macro);
       @Nullable MacroFrame parentMacroFrame = builder.setCurrentMacroFrame(childMacroFrame);
       try {
-        Starlark.call(
-            thread,
-            macro.getMacroClass().getImplementation(),
-            /* args= */ ImmutableList.of(),
-            /* kwargs= */ macro.getAttrValues());
-      } catch (EvalException ex) {
+        Object returnValue =
+            Starlark.call(
+                thread,
+                macro.getMacroClass().getImplementation(),
+                /* args= */ ImmutableList.of(),
+                /* kwargs= */ macro.getAttrValues());
+        if (returnValue != Starlark.NONE) {
+          throw Starlark.errorf(
+              "macro '%s' may not return a non-None value (got %s)",
+              macro.getName(), Starlark.repr(returnValue));
+        }
+      } catch (EvalException ex) { // from either call() or non-None return
         builder
             .getLocalEventHandler()
             .handle(
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
index 917a33847de243..45bab995c98dd9 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
@@ -176,6 +176,25 @@ def _impl():
         "pkg", "_impl() got unexpected keyword arguments: name, visibility");
   }
 
+  @Test
+  public void implementationMustNotReturnAValue() throws Exception {
+    scratch.file(
+        "pkg/foo.bzl",
+        """
+        def _impl(name, visibility):
+            return True
+        my_macro = macro(implementation=_impl)
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load(":foo.bzl", "my_macro")
+        my_macro(name="abc")
+        """);
+
+    assertGetPackageFailsWithEvent("pkg", "macro 'abc' may not return a non-None value (got True)");
+  }
+
   /**
    * Writes source files for package with a given name such that there is a macro by the given name
    * declaring a target by the given name.