Support += in Groovy build scripts #32745

mlopatkin · 2025-03-17T19:41:18Z

This builds on top of the #32287 (which cannot be reopened), changes to it are in the last two commits.

Summary of changes:

extract base support classes into a separate module groovy-support
support Groovy static compilation with type check
move Groovy support into separate files where possible

For the static types to work we can no longer get away with just DefaultXxx implementing some interface, all types has to be publicly available.

Fixes #23637

asodja

LGTM

asodja · 2025-03-19T09:49:31Z

...c/testFixtures/java/org/gradle/api/internal/groovy/support/TransformCompoundAssignments.java

 @Target({ElementType.METHOD, ElementType.TYPE})
-@GroovyASTTransformationClass("org.gradle.api.internal.provider.TestCompoundAssignmentTransform")
+@GroovyASTTransformationClass("org.gradle.api.internal.groovy.support.CompoundAssignmentTransformInTest")
 public @interface TransformCompoundAssignments {}


💅 Could we also att Test prefix to this name, so it's easier to know it comes from a test fixture? E.g.:
TransformCompoundAssignments -> TestTransformCompoundAssignments

alllex · 2025-03-20T10:51:49Z

...ration/groovy-support/src/main/java/org/gradle/api/internal/groovy/support/package-info.java

+ *         that accepts a <b>public</b> type of your class (e.g. {@code MapProperty} for {@code DefaultMapProperty} type) and returns a stand-in.
+ *     </li>
+ * </ol>
+ * Note that the extension method and the stand-in type and its methods become <b>public APIs</b> and must follow binary compatibility policy,


❓ Are changes to these types and method automatically tracked by our binary compatibility checks?

Are changes to these types and method automatically tracked by our binary compatibility checks?

Not really, but I've added some reflection-based unit tests to cover that. A cross-version test will be another option after we release a stable with this feature, but so far it is as much we can do. Kotlin DSL does something similar.

I haven't look deeper into the Kotlin thing, but if there is a similarity between this "hidden" API and the Kotlin one, it might be useful in the long-term to introduce a concept for this. As it stands, without prior knowledge it's very hard or impossible to discover this hidden API technically being part of the public one.

In effect, this is part of the public API surface, which is not intended to be used by users directly, but which must observe the same binary compatibility guarantees as the normal API. Also, it should not be included in the generated docs, etc.

I'd consider this tech debt, so it'd be nice to have something to track this, even if we'd only be able to get to the clean up much later.

alllex · 2025-03-20T11:00:23Z

...pport/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentExtensions.java

+/**
+ * This class provides catch-all support for {@code +=} overloading.
+ * <p>
+ * <b>This is a hidden public API</b>. Compiling Groovy code that depends on Gradle API may end up emitting references to methods of this class.


❓Should we add it to the Public API definition then?
build-logic-commons/basics/src/main/kotlin/gradlebuild/basics/PublicApi.kt

Should we add it to the Public API definition then?

Unfortunately this is tied to exposing it in documentation, and we'll have to move it out of internal package.

alllex · 2025-03-20T11:02:16Z

...roovy-support/src/main/resources/META-INF/groovy/org.codehaus.groovy.runtime.ExtensionModule

@@ -0,0 +1,19 @@
+#
+# Copyright 2018 the original author or authors.


alllex · 2025-03-20T11:02:57Z

...pport/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentExtensions.java

+     *
+     * @param lhs the original argument
+     * @param <T> the type of the argument, used to please the static type checker
+     * @return the give argument


alllex · 2025-03-20T11:04:44Z

...pport/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentExtensions.java

+import org.jspecify.annotations.Nullable;
+
+/**
+ * This class provides catch-all support for {@code +=} overloading.


🤔 Should it be OP= here as well, same as for the Result type? This class seems general, not bound to +=

alllex · 2025-03-20T11:14:14Z

...port/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentTransformer.java

+        }
+
+        /**
+         * Rewrites {@code foo <OP>= bar} into {@code (foo = foo.forCompoundAssignment() <OP> (bar)).unwrap()}.


✍️ We replaced "unwrap" with "toAssignementResult"

alllex · 2025-03-20T11:17:22Z

...port/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentTransformer.java

+        }
+
+        private Expression applyForCompoundAssignment(Expression expr) {
+            return callSupportMethodOn("forCompoundAssignment", expr);


💭 Since it's just a string literal, I'd add a mention of CompoundAssignmentExtensions in a comment or javadoc

alllex · 2025-03-20T11:18:17Z

...port/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentTransformer.java

+            return callSupportMethodOn("toAssignmentResult", expr);
+        }
+
+        private Expression callSupportMethodOn(String methodName, Expression argument) {


💅 Call-site reads better, if expr comes as the first argument

alllex · 2025-03-20T11:24:57Z

subprojects/core/src/main/java/org/gradle/groovy/scripts/internal/BuildScriptTransformer.java

        new StatementLabelsScriptTransformer().register(compilationUnit);
        new ModelBlockTransformer(scriptSource.getDisplayName(), scriptSource.getResource().getLocation().getURI()).register(compilationUnit);
        imperativeStatementDetectingTransformer.register(compilationUnit);
+        new CompoundAssignmentTransformer().register(compilationUnit);


❔ This means that if a user writes a Gradle plugin in Groovy, the += on properties and collections would differently. What would be the difference in behavior? Should we forbid += in non-build script source code for these types to avoid silent misconfiguration?

This means that if a user writes a Gradle plugin in Groovy, the += on properties and collections would differently. What would be the difference in behavior? Should we forbid += in non-build script source code for these types to avoid silent misconfiguration?

With the current implementation it wouldn't work outside of build scripts at all, with the usual missing plus() method. It is solvable, but there are objections to have build script sugar in the "normal" code context.

alllex · 2025-03-20T11:27:52Z

...ections/src/main/java/org/gradle/api/internal/file/collections/CompoundAssignmentResult.java

+ * A helper class to implement an intermediate result of a compound assignment operation, like "+=".
+ * It is then assigned to the left-hand side operand. When the LHS is a ConfigurableFileCollection-typed property of some Gradle-enhanced object, then the assignment action is invoked.
+ */
+class CompoundAssignmentResult extends UnionFileCollection implements org.gradle.api.internal.groovy.support.CompoundAssignmentResult {


💭 I generally don't like to see same-named classes in gradle/gradle codebase. It trips up quick searches, when only the class name appears in search results, file names and such.

Is there anything preventing us to give this a more explicit name like FileCollectionCompoundAssignmentResult?

alllex · 2025-03-20T11:50:40Z

...tion/model-core/src/main/java/org/gradle/api/internal/provider/CompoundAssignmentResult.java

+    public void assignmentComplete() {
+        // When the expression involves a variable on the left side as opposed to a field, then this provider becomes its value.
+        // It must lose all "magical" properties towards its owner, because it may be used to set its value outside the original expression.
+        assignToOwnerAction = null;


❓Why don't we dispose of the owner here?
We do it in the org.gradle.api.internal.file.collections.CompoundAssignmentResult.assignmentComplete

mlopatkin

@alllex PTAL

mlopatkin · 2025-03-28T18:29:25Z

...pport/src/main/java/org/gradle/api/internal/groovy/support/CompoundAssignmentExtensions.java

+/**
+ * This class provides catch-all support for {@code +=} overloading.
+ * <p>
+ * <b>This is a hidden public API</b>. Compiling Groovy code that depends on Gradle API may end up emitting references to methods of this class.


Should we add it to the Public API definition then?

Unfortunately this is tied to exposing it in documentation, and we'll have to move it out of internal package.

mlopatkin · 2025-03-28T18:31:27Z

...ration/groovy-support/src/main/java/org/gradle/api/internal/groovy/support/package-info.java

+ *         that accepts a <b>public</b> type of your class (e.g. {@code MapProperty} for {@code DefaultMapProperty} type) and returns a stand-in.
+ *     </li>
+ * </ol>
+ * Note that the extension method and the stand-in type and its methods become <b>public APIs</b> and must follow binary compatibility policy,


Are changes to these types and method automatically tracked by our binary compatibility checks?

Not really, but I've added some reflection-based unit tests to cover that. A cross-version test will be another option after we release a stable with this feature, but so far it is as much we can do. Kotlin DSL does something similar.

alllex · 2025-03-28T20:17:16Z

.../test/groovy/org/gradle/api/internal/file/collections/FileCollectionExtensionsApiTest.groovy

+    // This class verifies binary compatibility of non-public API used by Groovy static compilation.
+    // String are used intentionally to catch unexpected change to class name that breaks binary compatibility.


alllex · 2025-03-28T20:19:48Z

...ration/groovy-support/src/main/java/org/gradle/api/internal/groovy/support/package-info.java

🎉 Incredible level of documentation!

alllex · 2025-03-28T20:26:09Z

...ration/groovy-support/src/main/java/org/gradle/api/internal/groovy/support/package-info.java

+ *         that accepts a <b>public</b> type of your class (e.g. {@code MapProperty} for {@code DefaultMapProperty} type) and returns a stand-in.
+ *     </li>
+ * </ol>
+ * Note that the extension method and the stand-in type and its methods become <b>public APIs</b> and must follow binary compatibility policy,


I haven't look deeper into the Kotlin thing, but if there is a similarity between this "hidden" API and the Kotlin one, it might be useful in the long-term to introduce a concept for this. As it stands, without prior knowledge it's very hard or impossible to discover this hidden API technically being part of the public one.

In effect, this is part of the public API surface, which is not intended to be used by users directly, but which must observe the same binary compatibility guarantees as the normal API. Also, it should not be included in the generated docs, etc.

I'd consider this tech debt, so it'd be nice to have something to track this, even if we'd only be able to get to the clean up much later.

alllex

LGTM!

Amazing job, Mike! It's one of those rare exemplary pull requests 🌟

bot-gradle · 2025-03-28T21:30:21Z

The following builds have failed:

Pull Request Feedback (Trigger) (Check)
- Build Scan

This only supports `+=` and not `+`. Unlike Kotlin, this is much more involved because of the Groovy overloading restrictions. Groovy only supports overloading of +, and uses it to implement +=, by basically rewriting `a += b` into `a = a + b`. This doesn't play well with Properties, because a sensible implementation of plus operator (e.g. `a.zip(b) { l1, l2 -> l1 + l2 }`) will likely cause a self-referencing chain to happen. While this can also be solved upon assignment, we don't really want users to have "action-at-distance" when you can set `c = a + b`, and, a thousand lines after, happily assign `a = c` and get some strange results. The latter should cause circular evaluation failure. In order to support `+=`, and not arbitrary self-referencing `+` expressions, we employ an AST transformation. The transformation allows properties to provide a stand-in, which will be concatenated with the right-hand operand. This works for fields of Property types, where setting the property value to the stand-in simply invokes appropriate mutation operation, like `add` or `addAll`. This also works for variable, where the stand-in becomes the new value of the property. The stand-in acts as a `zip`-ped provider. Again, unlike Kotlin, the += is an expression in Groovy. To make both DSLs consistent, the value of += expression involving Properties is `null`. While it is still possible to write `foo = (a += b)` in Groovy, it is now rather pointless thing to do. All my attempts to give this expression some sensible value (e.g. return lhs operand or a shallow copy) faced different complicated corner cases. Implementation for ConfigurableFileCollection mostly follows the implementation in the collection properties. One difference is that the value of `+=` expression is already set in Groovy, at least when variables are involved. Therefore, we cannot simply replace it to null without breaking compatibility.

Sharing the same node in two branches of binary expression ( `lhs = lhs + rhs`) leads to unwanted failures when other AST transformations add node metadata. In particular, the static type checker overwrites the setter with getter's metadata, producing invalid bytecode. This was uncovered with Groovy 4, Groovy 3 probably emits the same metadata for both node instances. Add hand-rolled binary compatibility tests Our binary compatibility tests are tied to public _documented_ APIs living outside "internal" packages. We don't want to document += support methods at this point, but decoupling existing checks look involved. For now, we stick with simple reflection-based tests that will act as a safety net. We might be able to utilize cross-version tests in the future, but we need at least one release with this feature to be able to run them.

asodja · 2025-03-31T15:19:16Z

@bot-gradle test RFN

bot-gradle · 2025-03-31T16:32:58Z

The following builds have passed:

Ready for Nightly (Trigger) (Check)
- Build Scan

mlopatkin added the p:lazy-migration Issues covered by migration to an all-lazy API label Mar 17, 2025

mlopatkin added this to the 9.0 RC1 milestone Mar 17, 2025

mlopatkin requested review from alllex and asodja March 17, 2025 19:41

mlopatkin self-assigned this Mar 17, 2025

This comment has been minimized.

Sign in to view

asodja force-pushed the provider-api-migration/public-api-changes branch from 5172b4b to 4541adb Compare March 18, 2025 10:47

asodja force-pushed the ml/23637/plus-assign-groovy branch from 8b398c2 to 87c3ca1 Compare March 18, 2025 11:20

This comment has been minimized.

Sign in to view

asodja force-pushed the provider-api-migration/public-api-changes branch 2 times, most recently from 7c9a808 to 569f7b6 Compare March 18, 2025 15:19

asodja force-pushed the ml/23637/plus-assign-groovy branch from 87c3ca1 to 1471ff0 Compare March 18, 2025 15:21

This comment has been minimized.

Sign in to view

asodja approved these changes Mar 19, 2025

View reviewed changes

alllex reviewed Mar 20, 2025

View reviewed changes

mlopatkin force-pushed the ml/23637/plus-assign-groovy branch from 1471ff0 to 76df1c0 Compare March 28, 2025 18:26

mlopatkin commented Mar 28, 2025

View reviewed changes

mlopatkin requested a review from alllex March 28, 2025 18:32

alllex reviewed Mar 28, 2025

View reviewed changes

alllex approved these changes Mar 28, 2025

View reviewed changes

mlopatkin force-pushed the ml/23637/plus-assign-groovy branch from 76df1c0 to 664b9e8 Compare March 28, 2025 20:35

This comment has been minimized.

Sign in to view

mlopatkin added 2 commits March 31, 2025 17:06

asodja force-pushed the ml/23637/plus-assign-groovy branch from 664b9e8 to cde96f9 Compare March 31, 2025 15:18

This comment has been minimized.

Sign in to view

asodja merged commit ec69bf6 into provider-api-migration/public-api-changes Apr 1, 2025
20 checks passed

asodja deleted the ml/23637/plus-assign-groovy branch April 1, 2025 09:12

mlopatkin linked an issue Apr 1, 2025 that may be closed by this pull request

Support += for multi-value lazy properties and ConfigurableFileCollection in Groovy and Kotlin DSLs #23637

Closed

mlopatkin modified the milestones: 9.0 RC1, 10.0 RC1 Apr 1, 2025

		@@ -0,0 +1,19 @@
		#
		# Copyright 2018 the original author or authors.

		// This class verifies binary compatibility of non-public API used by Groovy static compilation.
		// String are used intentionally to catch unexpected change to class name that breaks binary compatibility.

Support += in Groovy build scripts #32745

Support += in Groovy build scripts #32745

Uh oh!

Conversation

mlopatkin commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asodja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlopatkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alllex left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

bot-gradle commented Mar 28, 2025

Uh oh!

asodja commented Mar 31, 2025

Uh oh!

This comment has been minimized.

bot-gradle commented Mar 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

mlopatkin commented Mar 17, 2025 •

edited

Loading