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

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(

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.