jamesfredley commented on code in PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#discussion_r3343107155
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GrailsASTUtils.java:
##########
@@ -1510,8 +1510,20 @@ public static void processVariableScopes(SourceUnit
source, ClassNode classNode,
VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source);
if (methodNode == null) {
scopeVisitor.visitClass(classNode);
+ return;
+ }
+ scopeVisitor.prepareVisit(classNode);
+ if (methodNode.getExceptions() == null) {
+ // Groovy 5's VariableScopeVisitor reads the method's exceptions
array without a null check, and AST
Review Comment:
It is fundamentally an upstream Groovy 5 regression:
`VariableScopeVisitor.visitConstructorOrMethod` now reads
`methodNode.getExceptions()` without the null-check it had in Groovy 4, and
`MethodNode.exceptions` is **final**, so a method created with null exceptions
cannot be repaired afterwards. The methods that reach `processVariableScopes`
come from several synthetic-creation sites
(`AbstractMethodDecoratingTransformation`, `AbstractWhereImplementer`,
`ResourceTransform`) plus the original user method, so normalising once at this
visitor chokepoint via a proxy is the minimal, complete fix rather than
auditing every `ClassNode.addMethod(..., null, ...)` call - and it also covers
methods we do not create. I can switch to passing `ClassNode.EMPTY_ARRAY` at
the Grails creation sites if you prefer, but it is more invasive and would not
cover externally-created methods. Reproducer:
https://github.com/jamesfredley/groovy5-variablescope-canonicalization-bug
##########
grails-datamapping-core/src/test/groovy/grails/gorm/annotation/transactions/TransactionalTransformSpec.groovy:
##########
@@ -197,10 +197,6 @@ import grails.gorm.transactions.Transactional
mySpec.getDeclaredMethod('$spock_feature_0_0', Object, Object, Object)
mySpec.getDeclaredMethod('$tt__$spock_feature_0_0', Object, Object,
Object, TransactionStatus)
- and:"The spec can be called"
- mySpec.newInstance().'$tt__$spock_feature_0_0'(2,2,4,new
DefaultTransactionStatus(null, new Object(), true, true, false, false, false,
null))
Review Comment:
Restored - both `and:"The spec can be called"` blocks that invoke
`$tt__$spock_feature_0_0(...)` with a `DefaultTransactionStatus` are back, so
the generated transactional method is exercised again.
##########
grails-datamapping-core/src/test/groovy/grails/gorm/services/ServiceTransformSpec.groovy:
##########
@@ -987,10 +986,10 @@ interface MyService {
then:"A compilation error occurred"
def e = thrown(MultipleCompilationErrorsException)
- e.message.normalize().contains '''No implementations possible for
method 'void foo()'. Please use an abstract class instead and provide an
implementation.
- @ line 6, column 5.
- void foo()
- ^'''
+ // Note: Groovy 5 changed the method signature format from 'void
foo()' to 'foo():void'
Review Comment:
Removed the inline note comment at this line. (I could not find a comment
containing "bridge" in this spec - if you meant a different one, point me at it
and I'll drop it too.) The assertion still checks both `'void foo()'` and
`'foo():void'` because Groovy 5 changed how that error message renders the
method signature, but the chatty comment is gone.
##########
grails-datamapping-core/src/test/groovy/org/grails/compiler/gorm/JpaEntityTransformSpec.groovy:
##########
@@ -45,7 +45,7 @@ class JpaEntityTransformSpec extends Specification {
@GeneratedValue(strategy=GenerationType.AUTO)
Long myId
- @Digits
+ @Digits(integer = 10, fraction = 2)
Review Comment:
`jakarta.validation.constraints.Digits` has no defaults - `integer()` and
`fraction()` are both required members with no default value. Bare `@Digits`
compiled under Groovy 4's more lenient annotation handling but fails under
Groovy 5's stricter checking, which is why explicit values were added. I can
pick values that read more like a typical field, but they cannot be omitted.
Happy to change `10/2` to something more representative if you have a
preference.
##########
grails-datamapping-validation/src/main/groovy/org/grails/datastore/gorm/validation/constraints/AbstractConstraint.java:
##########
@@ -233,13 +233,30 @@ protected String getDefaultMessage(String code) {
return messageSource.getMessage(code, null,
LocaleContextHolder.getLocale());
}
- return ConstrainedProperty.DEFAULT_MESSAGES.get(code);
+ return getDefaultMessageFromBundle(code);
}
catch (Exception e) {
- return ConstrainedProperty.DEFAULT_MESSAGES.get(code);
+ return getDefaultMessageFromBundle(code);
}
}
+ /**
+ * Gets default message from MESSAGE_BUNDLE when the interface-constant
static-init order (Groovy 5)
Review Comment:
Expanded the comment to explain the mechanism: `ConstrainedProperty` is a
Groovy interface (fields are implicitly `static final`), and `DEFAULT_MESSAGES`
is a map literal whose values reference the sibling `DEFAULT_*_MESSAGE` String
constants. Under Groovy 4 those constants were assigned before the map literal
ran; under Groovy 5 the interface field-initialisation order changed so the map
literal runs first and captures nulls. `MESSAGE_BUNDLE` is unaffected, hence
the fallback. Guarded by `DefaultMessageResolutionSpec`.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy:
##########
@@ -248,8 +248,20 @@ class AstUtils {
VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source)
if (methodNode == null) {
scopeVisitor.visitClass(classNode)
+ return
+ }
+ scopeVisitor.prepareVisit(classNode)
+ if (methodNode.exceptions == null) {
+ // Groovy 5's VariableScopeVisitor reads the method's exceptions
array without a null check, and AST
+ // transforms routinely create methods via
ClassNode.addMethod(..., null, ...). MethodNode.exceptions is
+ // final, so recompute scopes on a proxy that shares the same
parameters and code but carries an empty
+ // exceptions array, then copy the computed scope back onto the
real method.
+ MethodNode proxy = new MethodNode(methodNode.name,
methodNode.modifiers, methodNode.returnType,
Review Comment:
Same answer as the `GrailsASTUtils` thread: `MethodNode.exceptions` is
final, so a method created with null exceptions cannot be "populated" after the
fact - you would have to pass `ClassNode.EMPTY_ARRAY` at every creation site.
Since the methods reaching `processVariableScopes` originate from several
transforms (and the original user method), normalising once at this chokepoint
via a proxy is the minimal fix. The root cause is upstream: Groovy 5's
`VariableScopeVisitor` dropped the null-check on `getExceptions()` that Groovy
4 had. Happy to switch to per-site `EMPTY_ARRAY` if you prefer.
--
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]
