Fix `ModifyCollectionInEnhancedForLoop` by Pankraz76 · Pull Request #34972 · gradle/gradle

Pankraz76 · 2025-09-12T20:05:00Z

Context

Contributor Checklist

Review Contribution Guidelines.
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
Provide unit tests (under <subproject>/src/test) to verify logic.
Update User Guide, DSL Reference, and Javadoc for public-facing changes.
Ensure that tests pass sanity check: ./gradlew sanityCheck.
Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

BUILD SUCCESSFUL in 10m 47s

Pankraz76 · 2025-09-13T12:41:37Z

BUILD SUCCESSFUL in 35s
543 actionable tasks: 5 executed, 71 from cache, 467 up-to-date
Configuration cache entry reused.
14:40:30: Execution finished ':antlr:test --tests "org.gradle.api.plugins.antlr.AntlrPluginTest"'.

Pankraz76 · 2025-09-13T12:46:11Z

        } finally {
            lock.unlock();
        }
-        throw new IllegalStateException("Gradle user home directory scoped services have already been released.");


Is this still needed? Feels a little off, only working in combination with the return in L:132.

Since loop is actually an stream, this might not be mandatory anymore.

Isn't it for the break condition on L:124?

i mean everything is kind of the preset for the last line. Test again demanding this having the need for ifElse as implemented in new fashion way of java featuring streaming api.

Pankraz76 · 2025-09-13T13:01:13Z

-                        break;
-                    }
-                    services.count--;
-                    if (services.count == 0 && (servicesForHomeDir.size() > 1 || System.getProperty(REUSE_USER_HOME_SERVICES, "true").equals("false"))) {


Assuming count is the SSOT, meaning it’s not shared, we don’t need to decrement it but can simply check for 1 instead.

Assuming 0 would just be true, when having a value of 1 before decrement, therefore can just check for 1 and the logic remains the same, right? Adding +1 to the equation should work out.

I don't feel confident enough to change this logic. Let's leave it as is, just refactor it

Nothing has changed, right?
Its just doing the decrementation operation right before access, instead of dedicating a whole separate line. This might feel a bit niche or new to some developers, but it’s equally effective and logically the same.

The tests fail on missing incrementation case, which shows that this is simply a refactoring, not a logic change.

The result is identical, since performing the math operation before or after access only changes syntax — not behavior — as the tests confirm.

https://javabeginners.de/Grundlagen/Operatoren/Inkrement_und_Dekrement.php

We may misunderstand each other.

I'm ok with if (--services.count.... I'm not ok with checking against 1.

Actually, this is a difference I did not recognize before. Assuming a test blind spot, we should ensure that incrementation and decrementation are fully covered.

ov7a

Thank you for your contribution!

ov7a · 2025-09-16T13:27:15Z

        } finally {
            lock.unlock();
        }
-        throw new IllegalStateException("Gradle user home directory scoped services have already been released.");


Isn't it for the break condition on L:124?

ov7a · 2025-09-16T13:28:27Z

-                        break;
-                    }
-                    services.count--;
-                    if (services.count == 0 && (servicesForHomeDir.size() > 1 || System.getProperty(REUSE_USER_HOME_SERVICES, "true").equals("false"))) {


I don't feel confident enough to change this logic. Let's leave it as is, just refactor it

ov7a · 2025-09-16T13:30:02Z

@bot-gradle test APT

bot-gradle · 2025-09-16T15:31:46Z

The following builds have passed:

All Performance Tests (Trigger)
- Build Scan

Pankraz76 · 2025-09-16T13:47:45Z

-                        break;
-                    }
-                    services.count--;
-                    if (services.count == 0 && (servicesForHomeDir.size() > 1 || System.getProperty(REUSE_USER_HOME_SERVICES, "true").equals("false"))) {


Nothing has changed, right?
Its just doing the decrementation operation right before access, instead of dedicating a whole separate line. This might feel a bit niche or new to some developers, but it’s equally effective and logically the same.

The tests fail on missing incrementation case, which shows that this is simply a refactoring, not a logic change.

The result is identical, since performing the math operation before or after access only changes syntax — not behavior — as the tests confirm.

https://javabeginners.de/Grundlagen/Operatoren/Inkrement_und_Dekrement.php

Pankraz76 · 2025-09-16T13:50:08Z

        } finally {
            lock.unlock();
        }
-        throw new IllegalStateException("Gradle user home directory scoped services have already been released.");


i mean everything is kind of the preset for the last line. Test again demanding this having the need for ifElse as implemented in new fashion way of java featuring streaming api.

Pankraz76 · 2025-09-17T19:35:47Z

@@ -180,8 +284,7 @@ class DefaultGradleUserHomeScopeServiceRegistryTest extends Specification {
        homeDirServices.release(services)

        then:
-        def e = thrown(IllegalStateException)
-        e.message == 'Gradle user home directory scoped services have already been released.'


Is the exception needed, or can we just stay silent here, since the new all-in-one filter would pass through the non-early-exit case due to the functional style?

❌ The exception is required. Please revert this change

Pankraz76 · 2025-09-17T19:36:21Z

+        then: "count should be decremented only once"
+        initialCount == 1
+        countAfterFirstRelease == 0
+        countAfterSecondRelease == -1


is this the right intend? having now full coverage/contro

100% (3/3) 100% (12/12) 100% (54/54) 100% (18/18)

countAfterSecondRelease == -1
is this the right intend?

No, the original code would throw an IllegalStateException. Please preserve that behaviour

octylFractal · 2025-09-17T19:49:35Z

-            // unboxing and boxing does the trick
-            boolean value = options.get(key);
-            options.put(key, value);
+            options.compute(key, (k, value) -> value); // unboxing and boxing does the trick.


This does not do a unbox and re-box. Also, this is still a "modification", so it's not really solving the issue, even though it removes the warning. This could potentially be a false-negative from ErrorProne. I would just make a copy of the key set instead, for (String key : ImmutableSet.copyOf(options.keySet())).

yes sorry, then need to suppress suggesion: Can be replaced with single 'Map.compute' method call .

When inlining the field assuming this is equivalent leads to; Can be replaced with single 'Map.replaceAll' method call

leading to:

Suggested change

options.compute(key, (k, value) -> value); // unboxing and boxing does the trick.

options.replaceAll((k, v) -> v);

is this any good?

No, because again, that does not do an unbox and re-box, which is the whole reason for the call in the first place. options.replaceAll((k, v) -> (boolean) v); should work for that, or perhaps we should be explicit and use options.replaceAll((k, v) -> v ? Boolean.TRUE : Boolean.FALSE);. This prevents a warning from IntelliJ as well.

Casting 'v' to 'boolean' is redundant

integrated your draft thanks.

kindly request feedback.

ov7a

Thanks for covering this code with tests!

❌ Please keep trowing the IllegalStateException on multiple releases to preserve the original behavior

ov7a · 2025-09-24T09:44:55Z

+        then: "count should be decremented only once"
+        initialCount == 1
+        countAfterFirstRelease == 0
+        countAfterSecondRelease == -1


countAfterSecondRelease == -1
is this the right intend?

No, the original code would throw an IllegalStateException. Please preserve that behaviour

ov7a · 2025-09-24T09:47:09Z

@@ -180,8 +284,7 @@ class DefaultGradleUserHomeScopeServiceRegistryTest extends Specification {
        homeDirServices.release(services)

        then:
-        def e = thrown(IllegalStateException)
-        e.message == 'Gradle user home directory scoped services have already been released.'


❌ The exception is required. Please revert this change

Signed-off-by: Vincent Potucek <vpotucek@me.com>

ov7a

LGTM, I did a minor adjustment. Merging.

bot-gradle added from:contributor PR by an external contributor to-triage labels Sep 12, 2025

Pankraz76 commented Sep 12, 2025

View reviewed changes

Comment thread .../main/java/org/gradle/internal/service/scopes/DefaultGradleUserHomeScopeServiceRegistry.java

Pankraz76 commented Sep 13, 2025

View reviewed changes

Comment thread .../main/java/org/gradle/internal/service/scopes/DefaultGradleUserHomeScopeServiceRegistry.java

Pankraz76 commented Sep 14, 2025

View reviewed changes

Comment thread .../main/java/org/gradle/internal/service/scopes/DefaultGradleUserHomeScopeServiceRegistry.java Outdated

ov7a self-assigned this Sep 15, 2025

ov7a added in:building-gradle gradle/gradle build and removed to-triage labels Sep 16, 2025

ov7a reviewed Sep 16, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Pankraz76 commented Sep 16, 2025

View reviewed changes

Pankraz76 commented Sep 17, 2025

View reviewed changes

octylFractal reviewed Sep 17, 2025

View reviewed changes

Pankraz76 commented Sep 18, 2025

View reviewed changes

Comment thread ...m/language-groovy/src/main/java/org/gradle/api/internal/tasks/compile/ApiGroovyCompiler.java

ov7a requested changes Sep 24, 2025

View reviewed changes

Fix ModifyCollectionInEnhancedForLoop

5f95a5f

Signed-off-by: Vincent Potucek <vpotucek@me.com>

ov7a approved these changes Sep 25, 2025

View reviewed changes

ov7a enabled auto-merge September 25, 2025 07:34

ov7a added this pull request to the merge queue Sep 25, 2025

bot-gradle added this to the 9.3.0 RC1 milestone Sep 25, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 25, 2025

ov7a added this pull request to the merge queue Sep 25, 2025

Merged via the queue into gradle:master with commit b03db59 Sep 25, 2025
19 of 20 checks passed

	options.compute(key, (k, value) -> value); // unboxing and boxing does the trick.
	options.replaceAll((k, v) -> v);

Conversation

Pankraz76 commented Sep 12, 2025

Context

Contributor Checklist

Reviewing cheatsheet

Uh oh!

Uh oh!

Pankraz76 commented Sep 13, 2025

Uh oh!

Pankraz76 Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ov7a left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ov7a commented Sep 16, 2025

Uh oh!

This comment has been minimized.

bot-gradle commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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

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!

Pankraz76 Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ov7a left a comment

Choose a reason for hiding this comment

Pankraz76 Sep 13, 2025 •

edited

Loading

Pankraz76 Sep 17, 2025 •

edited

Loading

Pankraz76 Sep 17, 2025 •

edited

Loading

Pankraz76 Sep 18, 2025 •

edited

Loading

ov7a left a comment •

edited

Loading