jamesfredley commented on code in PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#discussion_r3343108331
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/reflect/ClassPropertyFetcherTests.groovy:
##########
@@ -114,8 +114,8 @@ class ClassPropertyFetcherTests {
}
}
-trait TestTrait<F extends Serializable> {
- F from
+trait TestTrait<T> {
Review Comment:
Restored the `Serializable` bound: `trait TestTrait<F extends
Serializable>`. It was an incidental change; the datastore test keeps
exercising the bounded trait type parameter.
##########
grails-fields/src/main/groovy/grails/plugin/formfields/BeanPropertyAccessorFactory.groovy:
##########
@@ -125,12 +125,31 @@ class BeanPropertyAccessorFactory implements
GrailsApplicationAware {
private Constrained resolveConstraints(BeanWrapper beanWrapper, String
propertyName) {
Class<?> type = beanWrapper.wrappedClass
- boolean defaultNullable = Validateable.isAssignableFrom(type) ?
type.metaClass.invokeStaticMethod(type, 'defaultNullable') : false
+ boolean defaultNullable = Validateable.isAssignableFrom(type) ?
resolveDefaultNullable(type) : false
ConstrainedProperty constraint = constraintsEvaluator.evaluate(type,
defaultNullable)[propertyName]
new Constrained(constraint ?: createDefaultConstraint(beanWrapper,
propertyName))
}
+ private static boolean resolveDefaultNullable(Class<?> clazz) {
Review Comment:
Added a doc comment explaining it. The switch from
`type.metaClass.invokeStaticMethod(type, 'defaultNullable')` to reflection is
needed because under Groovy 5 `Validateable`'s `TraitReceiverTransformer`
rewrites the in-trait `defaultNullable()` call to a direct trait-helper static
call, which loses the implementing class's override and always returns the
trait default. A reflective `Class.getMethod('defaultNullable').invoke(null)`
resolves the actual override. See GROOVY-11985. Mirrors
`Validateable.resolveDefaultNullable`.
##########
grails-geb/src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy:
##########
@@ -499,7 +504,8 @@ class WebDriverContainerHolder {
private static class InterceptingProperties extends Properties {
@Override
String getProperty(String key) {
- def v = OVERRIDDEN_SYSTEM_PROPERTIES.get().get(key)
+ Map<String, String> overrides = getOverriddenProperties()
Review Comment:
It is a Groovy 5 STC limitation: from the nested
`InterceptingProperties.getProperty(String)`, Groovy 5 could not statically
resolve the enclosing class's private static `OVERRIDDEN_SYSTEM_PROPERTIES`
ThreadLocal and the type of its `.get()`, so the typed
`getOverriddenProperties()` accessor is the workaround. It could instead be an
inline `((Map<String, String>) OVERRIDDEN_SYSTEM_PROPERTIES.get())` cast if
you'd rather not keep the helper - let me know your preference.
##########
grails-gradle/gradle/docs-config.gradle:
##########
@@ -25,11 +25,13 @@ dependencies {
// TODO: Remove jline:jline (JLine 2) when upgrading to Groovy 5
(groovy-groovysh 5.x uses JLine 3)
add('documentation', 'jline:jline')
add('documentation', 'com.github.javaparser:javaparser-core')
- add('documentation',
"org.apache.groovy:groovy:${bomDependencyVersions['groovy.version']}")
- add('documentation',
"org.apache.groovy:groovy-groovydoc:${bomDependencyVersions['groovy.version']}")
- add('documentation',
"org.apache.groovy:groovy-ant:${bomDependencyVersions['groovy.version']}")
- add('documentation',
"org.apache.groovy:groovy-docgenerator:${bomDependencyVersions['groovy.version']}")
- add('documentation',
"org.apache.groovy:groovy-templates:${bomDependencyVersions['groovy.version']}")
+ // grails-gradle subprojects target Gradle's embedded Groovy 4 (see
gradleBomDependencyVersions['gradle-groovy.version']).
+ // Do NOT use the main Groovy version here - that is Groovy 5.x in Grails
8 and would override the gradle-groovy-bom platform.
Review Comment:
Simplified to a single line: `// Use Gradle's embedded Groovy
(gradle-groovy.version), not the project's main Groovy version.`
##########
grails-gradle/tasks/build.gradle:
##########
@@ -40,7 +40,9 @@ ext {
dependencies {
implementation platform(project(':grails-gradle-bom'))
- implementation
"org.apache.groovy:groovy:${bomDependencyVersions['groovy.version']}"
+ // grails-gradle-tasks targets Gradle's embedded Groovy 4
(gradleBomDependencyVersions['gradle-groovy.version']).
Review Comment:
Simplified, version reference dropped: `// Use Gradle's embedded Groovy, not
the project's main Groovy version.`
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:
##########
@@ -146,6 +153,9 @@ out.print(messageClosure('World'))
t.metaInfo.compilationException.message.contains('Cannot find matching
method java.util.Date#getTimeTypo()')
}
+ // Note: In Groovy 5, the type checking extension behavior changed and
undeclared variables
+ // in GSP templates may not trigger compilation errors. This is a known
limitation.
Review Comment:
To be clear about scope: GSP **static compilation still works** - `g.*`
taglib calls from `@CompileStatic` GSPs resolve via the namespace-matching
`methodNotFound` handler in `GroovyPageTypeCheckingExtension` (the GROOVY-12041
approach). What these two `@IgnoreIf` tests assert is the *negative* case: that
referencing an **undeclared** variable (`${somename}`) raises a compile error.
Under Groovy 5 the STC `unresolvedVariable`/`unresolvedProperty` callbacks do
not fire when the receiver inherits `getProperty(String)`, so that compile
error is not produced and the negative assertion cannot hold. So this is a loss
of *strictness* in detecting undeclared GSP variables, not a loss of static
compilation. Tracked against GROOVY-6362 / GROOVY-11817 / GROOVY-12041, and
related to #15669 (`@GrailsCompileStatic` taglib support). @paulk-asert any
history on the undeclared-variable callback change would be appreciated.
##########
grails-logging/src/main/groovy/org/grails/compiler/logging/LoggingTransformer.java:
##########
@@ -78,11 +80,24 @@ public void performInjectionOnAnnotatedClass(SourceUnit
source, ClassNode classN
return;
}
- AnnotationNode annotationNode = new
AnnotationNode(ClassHelper.make(Slf4j.class));
- LogASTTransformation logASTTransformation = new LogASTTransformation();
- logASTTransformation.setCompilationUnit(new CompilationUnit(new
GroovyClassLoader(getClass().getClassLoader())));
- logASTTransformation.visit(new ASTNode[]{ annotationNode, classNode},
source);
- classNode.putNodeMetaData(Slf4j.class, annotationNode);
+ // Groovy 5: an @Slf4j added during an AST transform is not processed,
so inject the log field manually.
Review Comment:
The issue is applying `@Slf4j` *programmatically from inside another AST
transform*: in Groovy 5 the `LogASTTransformation` invoked from our injector is
not picked up (the annotation added mid-transform is not processed), so no
`log` field is generated. The PR injects the `log` field directly
(`LoggerFactory.getLogger(<class>)`) instead. @paulk-asert if this was fixed in
a `GROOVY_5_0_X` snapshot, point me at it and I'll revert to letting
`LogASTTransformation` do it.
--
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]
