jamesfredley commented on code in PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#discussion_r3178549030
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/JsonViewTemplateResolverSpec.groovy:
##########
@@ -64,6 +71,8 @@ class JsonViewTemplateResolverSpec extends Specification {
}
+ // Skip on Groovy 5+ - mocking final methods
(GrailsWebRequest.getRequest()) not supported without special configuration
+ @IgnoreIf({ instance.isGroovy5OrLater() })
Review Comment:
Spock 2.4 + Mockito with `mock-maker-inline` does support mocking final
methods, you're right. The `@IgnoreIf` here was applied because the project's
current test stack does not have `mock-maker-inline` wired in - the
resources/mockito-extensions/org.mockito.plugins.MockMaker file is not on the
classpath, so Spock's default JDK proxy mock-maker hits the final
`GrailsWebRequest.getRequest()` call and throws. Adding mockito-inline as a
test runtime dependency in `grails-views-gson/build.gradle` (or rewriting the
test against `MockHttpServletRequest` instead of `Mock(GrailsWebRequest)`)
would re-enable this spec on Groovy 5. I'd prefer to leave that change to a
separate PR scoped to the testing-stack upgrade rather than bundle it into the
Groovy 5 / Spring Boot 4 sweep, since it changes the mocking strategy for the
file. Leaving this thread open.
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:
##########
@@ -86,6 +93,10 @@ class GspCompileStaticSpec extends Specification {
compileStatic << [true, false]
}
+ // Note: In Groovy 5, the g.message() syntax with g. prefix fails static
type checking
+ // because the type checking extension doesn't properly resolve the 'g'
taglib property.
Review Comment:
Confirmed - GROOVY-6362 (the original `g.taglib()` from `@CompileStatic`
support, fixed in 2.5.0) and GROOVY-11817 (the claimed regression fix) are the
right tickets to anchor on. I tested `g.message(code: 'foo')` from inside a
`@CompileStatic` GSP class against 5.0.6-SNAPSHOT build #23 and the type-check
extension still doesn't resolve the `g` taglib namespace, so this is a
regression in 5.0.6 that GROOVY-11817 did not fully restore. I'll file a new
Groovy ticket against `GROOVY_5_0_X` referencing both 6362 and 11817, with a
standalone reproducer (the same shape as the disabled tests in
`GspCompileStaticSpec`). Leaving the `@IgnoreIf` and inline note here as a
known regression until the upstream ticket lands - will re-enable the tests
when the fix is in a snapshot. Will update this thread when the Groovy ticket
is filed.
##########
grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/UrlMappingTagLib.groovy:
##########
@@ -184,14 +184,14 @@ class UrlMappingTagLib implements TagLibrary {
// display previous link when not on firststep unless omitPrev is true
if (currentstep > firststep && !attrs.boolean('omitPrev')) {
linkParams.offset = offset - max
- writer << callLink(appendClass((Map) linkTagAttrs.clone(),
'prevLink')) {
+ writer << callLink(appendClass(new LinkedHashMap(linkTagAttrs),
'prevLink')) {
Review Comment:
This change came in via Mattias's commit c54de20008f (`fix: more Groovy 5
compatibility changes`, 2026-01-15) along with several other Groovy 5 compat
fixes in `MongoCodecSession`, `BsonPersistentEntityCodec`,
`PersistentEntityCodec`, etc. The original `(Map) linkTagAttrs.clone()` form
runs into a Groovy 5 `@CompileStatic` issue - `Map.clone()` is declared `Object
clone()` on the JDK `Cloneable` contract, and Groovy 5's stricter STC dispatch
on the cast result no longer routes through the `DefaultGroovyMethods` MOP path
that Groovy 4 used (the same kind of dispatch tightening we saw in
`File.asBoolean`). `new LinkedHashMap(linkTagAttrs)` is the explicit
copy-constructor shape that survives unchanged. There's no upstream Groovy
ticket I can point at for this one - the change has been on the branch since
January and predates the audit. I can file a follow-up Groovy ticket with a
standalone reproducer if you'd like to track it for upstream. Leaving this
thread open pending that decis
ion.
##########
grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/ValidationTagLib.groovy:
##########
@@ -317,7 +317,9 @@ class ValidationTagLib implements TagLibrary {
}
catch (NoSuchMessageException e) {
if (error instanceof MessageSourceResolvable) {
- text = ((MessageSourceResolvable) error).codes[0]
+ MessageSourceResolvable resolvable =
(MessageSourceResolvable) error
Review Comment:
Agreed - reverted in commit ad634e0c09e. The `defaultMessage ?: codes[0]`
tweak was a behavioural change (prefer the i18n `defaultMessage` over the raw
error code in the catch path) that snuck in with the Groovy 5 sweep but is not
Groovy-version conditional. Restored to the original single-line `text =
((MessageSourceResolvable) error).codes[0]` shape. If we want the
defaultMessage preference, that should be its own scoped change with its own
tests.
##########
grails-scaffolding/grails-app/commands/scaffolding/CreateScaffoldServiceCommand.groovy:
##########
@@ -47,12 +47,12 @@ class CreateScaffoldServiceCommand implements
GrailsApplicationCommand, CommandL
final String domainClassName = args[0]
if (!domainClassName) {
error('No domain-class specified')
- return FAILURE
+ return false
Review Comment:
Agreed - reverted across all 9 scaffolding command files in commit
ad634e0c09e (`CreateScaffoldControllerCommand`, `CreateScaffoldServiceCommand`,
`GenerateAllCommand`, `GenerateAsyncControllerCommand`,
`GenerateControllerCommand`, `GenerateScaffoldAllCommand`,
`GenerateServiceCommand`, `GenerateViewsCommand`, `InstallTemplatesCommand`).
The earlier inline-to-`true`/`false` rewrite (commit d2441fbcb5a) was driven by
the GROOVY-11907 trait-static-fields bytecode bug that prevented all
scaffolding commands from loading at the time. GROOVY-11907 is fixed in 5.0.5,
and its indy=false follow-up GROOVY-11968 is fixed in 5.0.6 build #22, so the
constants on the `CommandLineHelper` trait resolve cleanly again. The trait
still defines `static final boolean SUCCESS = true` / `static final boolean
FAILURE = false`, no other change needed. `./gradlew :grails-scaffolding:test`
passes against 5.0.6-SNAPSHOT build #23.
##########
grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/RestfulServiceController.groovy:
##########
@@ -82,7 +82,7 @@ class RestfulServiceController<T extends GormEntity<T>>
extends RestfulControlle
@Override
protected Integer countResources() {
- getService().count(params)
+ Math.toIntExact(getService().count(params))
Review Comment:
This change is actually load-bearing for Groovy 5 / @CompileStatic
compilation, not cosmetic. `RestfulServiceController.countResources()`
overrides `RestfulController.countResources()` whose return type is `Integer`.
`getService().count(params)` resolves to `GormStaticApi.count(params)` which
returns `Number` (not `Integer`) on the new GORM 9.x interface. Under Groovy 4
`@CompileStatic` the implicit `Number` -> `Integer` narrowing was tolerated;
under Groovy 5 `@CompileStatic` the typer rejects the implicit narrowing and
the file fails to compile (`Cannot return value of type Number on method
returning type Integer`). `Math.toIntExact` is the standard JDK shape for that
conversion (and correctly throws `ArithmeticException` on overflow rather than
silently truncating like a plain `(int)` cast would, which matters for a count
that exceeds `Integer.MAX_VALUE`). The change is from c54de20008f (`fix: more
Groovy 5 compatibility changes` by @mattiasreichel, 2026-01-15) - I'll add an in
line comment to that effect so it doesn't read as a drive-by improvement.
Leaving the change in this PR rather than extracting since reverting it breaks
`:grails-scaffolding:compileGroovy`.
##########
grails-test-examples/hibernate5/grails-hibernate-groovy-proxy/grails-app/domain/example/Customer.groovy:
##########
@@ -19,11 +19,9 @@
package example
-import grails.compiler.GrailsCompileStatic
import grails.persistence.Entity
@Entity
-@GrailsCompileStatic
Review Comment:
Removed in commit 83567f41930 (`fix: resolve Groovy 5 CI failures for
CodeNarc, controller params, and bytecode`, 2026-04-06). At that time, applying
`@GrailsCompileStatic` to this entity (which has both an explicit two-arg
constructor `Customer(Long id, String name)` and a `static mapping = { id
generator: 'assigned' }` closure) produced a runtime `VerifyError` in the
static initialiser of the `mapping` closure on Groovy 5. The same VerifyError
shape (`get long/double overflows locals`) was the symptom of GROOVY-11907 and
its indy=false follow-up GROOVY-11968 - both of which are now fixed in 5.0.6
build #22. I'll re-test restoring `@GrailsCompileStatic` on this entity against
build #23. If it now compiles and the proxy regression spec still passes, I'll
restore it in a follow-up commit and resolve this thread; if it still
VerifyErrors, I'll add an inline comment with the actual mechanism (likely a
separate Groovy bug) and link to the test that fails. Leaving the thread open
until
I confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
