Repository navigation
refactor(build-tooling): idiomatic Groovy cleanup of the J2G output - #16661
Open
borinquenkid wants to merge 6 commits into
Open
borinquenkid wants to merge 6 commits into
borinquenkid wants to merge 6 commits into
Conversation
Behaviour-preserving idiom pass over the Java-to-Groovy conversion output for
build-logic/docs-core and grails-gradle/common+model (14 of the 35 files in
scope; the rest were already idiomatic or are interfaces/DTOs left untouched).
Applied:
- property syntax for getters with no isX/getX collision: ~35 sites
(Class.getName() -> .name, URL.getProtocol()/.getFile()/.getPath() -> .protocol/.file/.path,
File.getName()/.getParentFile()/.getAbsolutePath()/.getCanonicalPath(), URLConnection.getContentLength()/
.getResponseCode(), Throwable.getMessage(), Class.isArray()/.getComponentType() -> .array/.componentType,
Thread.getContextClassLoader(), Resource.getURL()/.getInputStream() -> .URL/.inputStream matching the
existing test idiom)
- map.get(k)/map.containsKey(k) read -> map[k]: ~10 sites (CollectionUtils, Holder, CommandLineParser,
DefaultCommandLine, GrailsResourceUtils, SpringIOUtils)
- .equals( on Strings -> ==: ~15 sites (GrailsResourceUtils protocol/path checks, CommandLineParser/
DefaultCommandLine constant comparisons, FileSystemResource/UrlResource equals() field comparisons)
- str.length() == 0 / > 0 -> isEmpty()/!isEmpty(): ~12 sites
- StringBuilder.append( -> << in statement position: ~20 sites
- string concatenation -> GString where it lands in a String-typed slot (return type or exception
message): FileSystemResource/UrlResource#getDescription, SpringIOUtils exception messages
- dropped unnecessary casts after instanceof flow typing: CollectionUtils#getOrCreateChildMap,
GrailsResourceUtils#getPropertyNameConvention-equivalent branches, UrlResource#getInputStream,
AbstractFileResolvingResource#contentLength/lastModified
- x != null ? x.foo() : null -> x?.foo(): SpringIOUtils#closeQuietly
- for (X x : coll) -> for (X x in coll): SpringIOUtils, DefaultCommandLine
- dropped the UnnecessaryPublicModifier on StringEscapeCategory's static methods
Deliberately left (Tier B exclusions / risk):
- Environment.getName() calls (CommandLineParser, DefaultCommandLine): Environment is an enum that
defines its own getName()/setName(), shadowing the inherited Enum.name() the property syntax would
otherwise resolve to differently; left as explicit calls.
- DefaultResourceLoader.getClassLoader() bare calls from getResource()/getResourceByPath(): the private
classLoader field with the same name is accessible by direct-field priority within the declaring
class, so property syntax there would bypass the getter's fallback-to-default logic. The one
cross-class static-dispatch call sites (Class literal -> Class#getClassLoader()) were converted,
matching the idiom the Spec already exercises (DefaultResourceLoader.classLoader / .defaultClassLoader).
- SupplierUtil#memoizedNonEmpty's instanceof-narrowed cast on `value` kept: the variable is shared across
two closures assigned to the same `delegate` field, so Groovy's static type checker refuses to narrow
it (javap/compile verified; dropping the cast failed compileGroovy with a "closure shared variable"
error).
- GrailsResourceUtils#instantiateFromFlatConfig's Class.newInstance() kept: swapping in
getDeclaredConstructor().newInstance() per the table entry would add NoSuchMethodException/
InvocationTargetException to the call site, widening the method's declared checked-exception surface
(a public API change), so it was left as-is.
- SpringIOUtils `in`/`out` parameter names across every copy() overload: a real "in is a keyword, rename
it" Tier A hit, but touching all ~8 overloads' signatures for a non-functional rename was judged
disproportionate to a focused idiom pass; left unchanged (it already compiles - `in` is only a Groovy
keyword in for-loop/membership positions).
- ClassPathResource#nullSafeEquals' o1.equals(o2) kept: o1/o2 are declared Object and may be arrays;
Groovy's == has its own (possibly different) array-comparison semantics, and the very next lines branch
on primitive-array types via Arrays.equals, so swapping to == risked changing which branch is reached.
- Trivial getter/setter field pairs (ClassPathResource#getPath, FileSystemResource#getPath/getFile,
DefaultCommandLine$SpecifiedOption#getOption/getValue) kept as explicit methods: collapsing them to bare
Groovy properties would add a public setter that does not exist today, a public API change.
- XmlParserFeature#getFeatureName kept explicit: carries Javadoc that must stay on the method.
Found and fixed during verification: `map[k] = v` (subscript assignment) on a java.util.Map field
compiles to a dynamic `ScriptBytecodeAdapter.invokeMethodN("putAt", ...)` call under @CompileStatic in
this Groovy version, unlike the read form `map[k]` which resolves to the static
DefaultGroovyMethods.getAt - confirmed via the javap audit across every module. Reverted all ~10
map[k] = v sites introduced by this pass back to map.put(k, v); the read-only subscript conversions were
kept since they compile cleanly.
Verification (build-logic, own gradlew):
- :grails-docs-core:compileGroovy, :compileTestGroovy: BUILD SUCCESSFUL
- :grails-docs-core:test: 67 tests, 1 pre-existing failure unrelated to this change
(FetchTagsTaskSpec, environmental: this sandbox's global git tag.gpgSign=true with no core.editor
breaks `git tag`, nothing to do with StringEscapeCategory); StringEscapeCategorySpec itself 2/2 passed.
- no codenarc/checkstyle task exists for this project (compileGroovy only)
Verification (grails-gradle, own gradlew):
- :grails-gradle-common:compileGroovy, :grails-gradle-model:compileGroovy, both compileTestGroovy:
BUILD SUCCESSFUL
- :grails-gradle-common:codeStyle, :grails-gradle-model:codeStyle: BUILD SUCCESSFUL, 0 CodeNarc
violations in both main.html reports
- :grails-gradle-common:test (24/24) + :grails-gradle-model:test (432/432): 456/456, 0 failures,
0 errors, 0 skipped (test-results XML, not exit code)
- javap -c -p dynamic-dispatch audit on every changed class: clean after the putAt fix above. The only
remaining ScriptBytecodeAdapter/GroovyObject-property hits are inside the compiler-generated
this$dist$invoke$1/this$dist$set$1/this$dist$get$1 trampolines that GrailsResourceUtils, SupplierUtil,
DefaultResourceLoader and DefaultCommandLine already carried before this change (triggered by their
pre-existing anonymous/nested inner classes), confirmed by diffing against untouched classes with no
such nesting (Option, FileSystemResource, ...) which show none.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…uild-tooling # Conflicts: # grails-gradle/model/src/main/groovy/org/grails/build/parsing/DefaultCommandLine.groovy
…uild-tooling # Conflicts: # grails-gradle/model/src/main/groovy/org/grails/build/parsing/DefaultCommandLine.groovy
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor/java-to-groovy-build-tooling #16661 +/- ##
===============================================================================
+ Coverage 64.0971% 64.1063% +0.0092%
+ Complexity 28681 28667 -14
===============================================================================
Files 2260 2260
Lines 112147 112145 -2
Branches 20366 20366
===============================================================================
+ Hits 71883 71892 +9
+ Misses 31140 31124 -16
- Partials 9124 9129 +5
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: fc8c30d Learn more about TestLens at testlens.app/docs. |
…(revert before merge) Pushes, rebases and force-pushes to this stacked PR start no jobs; the PR closest to the release branch keeps CI. Merging with this commit is a blocker. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Member
Author
|
BLOCKER before merge: this branch carries a temporary CI commit. This PR is stacked, and to avoid starting a full CI matrix on every push or rebase of the stack it carries one temporary commit, Condition for merging this PR: that commit must be reverted on this branch first, and CI must then run and be green. The absence of CI checks on this PR is deliberate and does not mean the code was tested. The full build runs on #16576 and #16572, the PRs that target 10.0.x. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacks on #16576 (java-to-groovy conversion for
build-logic/docs-coreandgrails-gradle/common+model).Behaviour-preserving idiom pass over that conversion's output (14 of the 35 files in scope; the rest were already idiomatic or are interfaces/DTOs left untouched).
Applied
Class.getName()→.name,URL/File/URLConnection/Throwable/Resourceaccessors,Class.isArray()/.getComponentType()→.array/.componentType)map.get(k)→map[k]).equals()on Strings →==str.length() == 0→isEmpty()StringBuilder.append(→<<String-typed slotinstanceofflow typingx != null ? x.foo() : null→x?.foo()for (X x : coll)→for (X x in coll)publicmodifier onStringEscapeCategory's static methodsDeliberately left (documented per-site in the commit body):
Environment.getName()(shadowsEnum.name()differently),DefaultResourceLoader.getClassLoader()field-priority call, a closure-shared-variable cast the type checker rejects dropping,Class.newInstance()(avoids widening a public API's checked-exception surface),SpringIOUtils'sin/outparameter names across ~8 overloads (real Tier-A hit, judged disproportionate as a standalone rename),ClassPathResource#nullSafeEquals's.equals()on anObjectthat may be an array, trivial getter/setter pairs that would add a public setter that doesn't exist today, and one Javadoc-bearing getter.Found during verification:
map[k] = v(subscript assignment) compiles to a dynamicScriptBytecodeAdapter.invokeMethodN("putAt", ...)call under@CompileStatichere, unlike the read form which resolves statically. All ~10 sites introduced by this pass were reverted back to.put(k, v); the read-only subscript conversions were kept.This revision also carries forward the rename-compliance fix from #16576 (
DefaultCommandLine.groovykeeping semicolons temporarily) merged and resolved to its fully idiomatic end state — the semicolons and the@SuppressWarningsfrom that fix are removed here, completing the 3-stage conversion→cleanup split for that file.Verification
build-logic: compile/test green (StringEscapeCategorySpec2/2; one pre-existing, unrelatedFetchTagsTaskSpecsandbox failure)grails-gradle: compile green forcommon+model;codeStyle0 CodeNarc violations; fulltest456/456, 0 failuresjavap -c -pdynamic-dispatch audit clean on every changed class after theputAtfix🤖 Generated with Claude Code