VariableScopeVisitor / @slf4j workaround audit - keep what is needed, drop the rest

LoggingTransformer.java: Restored the original (Grails 2.0 era) comment. The post-Groovy-5 commit only swapped out the comment - the actual code (manual log field injection) has been the same since 2012. There is no Groovy 5 difference here.

GrailsASTUtils.java / AstUtils.groovy / AbstractMethodDecoratingTransformation.groovy: tested locally by reverting all four try/catch + non-null-VariableScope guards. Compilation of grails-datamapping-tck fails with BUG! exception in phase 'canonicalization' in source unit 'DataServiceRoutingProductDataService.groovy' unexpected NullPointerException on Groovy 5.0.6-SNAPSHOT, so the guards ARE needed. Restored them with a comment that points at the upstream bug (Groovy 5 VariableScopeVisitor NPE on certain Grails AST transformation outputs) and away from the incorrect 'Groovy 5 changed how VariableScopeVisitor handles certain AST states' framing - we have no evidence the visitor itself changed; what changed is that some Grails AST transforms now produce a node shape that the visitor fails on.

Net effect: zero functional change vs HEAD, but every comment now reflects what the audit verified rather than the original misdiagnoses. The four try/catch sites in compiler/AST code are flagged as upstream-Groovy-bug-bandaids that should be removed once the upstream fix lands.

Assisted-by: claude-code:claude-opus-4-7

Author: jamesfredley

grails-core/src/main/groovy/org/grails/compiler/injection/GrailsASTUtils.java

@@ -1507,12 +1507,8 @@ public static void processVariableScopes(SourceUnit source, ClassNode classNode)
     }
 
     public static void processVariableScopes(SourceUnit source, ClassNode classNode, MethodNode methodNode) {
-        // Groovy 5 changed how VariableScopeVisitor handles certain AST states.
-        // In some transformation scenarios, the visitor may throw NPE due to
-        // uninitialized scopes or missing AST nodes. Since variable scope processing
-        // is primarily for error reporting and doesn't affect code generation for
-        // transformations that have already set up their scopes, we can safely
-        // skip it when it fails.
+        // Groovy 5 VariableScopeVisitor NPE on certain Grails AST transformation outputs.
+        // See org.grails.datastore.mapping.reflect.AstUtils.processVariableScopes for context.
         try {
             VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source);
             if (methodNode == null) {
@@ -1521,10 +1517,8 @@ public static void processVariableScopes(SourceUnit source, ClassNode classNode,
                 scopeVisitor.prepareVisit(classNode);
                 scopeVisitor.visitMethod(methodNode);
             }
-        } catch (NullPointerException e) {
-            // Groovy 5 compatibility: silently ignore NPE from VariableScopeVisitor
-            // The transformation has already completed its work and the code will
-            // compile correctly without the scope validation.
+        } catch (NullPointerException ignored) {
+            // Groovy 5 VariableScopeVisitor NPE - upstream bug
         }
     }
 

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy

@@ -297,8 +297,8 @@ abstract class AbstractMethodDecoratingTransformation extends AbstractGormASTTra
      */
     protected MethodCallExpression makeDelegatingClosureCall(Expression targetObject, String executeMethodName, ArgumentListExpression arguments, Parameter[] closureParameters, MethodCallExpression originalMethodCall, VariableScope variableScope) {
         final ClosureExpression closureExpression = closureX(closureParameters, createDelegingMethodBody(closureParameters, originalMethodCall))
-        // Groovy 5 requires ClosureExpression to have a non-null VariableScope for bytecode generation.
-        // If the provided scope is null, create a new empty one to avoid NPE in ClosureWriter.
+        // Groovy 5 ClosureWriter NPEs on a ClosureExpression with a null VariableScope; default
+        // to an empty scope when the caller does not provide one.
         closureExpression.setVariableScope(
                 variableScope != null ? variableScope : new VariableScope()
         )
@@ -355,15 +355,14 @@ abstract class AbstractMethodDecoratingTransformation extends AbstractGormASTTra
         methodNode.setCode(null)
         classNode.addMethod(renamedMethodNode)
 
-        // Use a dummy source unit to process the variable scopes to avoid the issue where this is run twice producing an error
-        // Groovy 5 changed how VariableScopeVisitor handles certain AST states, which can cause NPE.
-        // Wrap in try-catch to gracefully handle this since the method node already has its scope set above.
+        // Use a dummy source unit to process the variable scopes to avoid the issue where this is run twice producing an error.
+        // Groovy 5 VariableScopeVisitor NPE: see AstUtils.processVariableScopes for the upstream bug.
         try {
             VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(new SourceUnit('dummy', 'dummy', source.getConfiguration(), source.getClassLoader(), new ErrorCollector(source.getConfiguration())))
             scopeVisitor.prepareVisit(classNode)
             scopeVisitor.visitMethod(renamedMethodNode)
-        } catch (NullPointerException e) {
-            // Groovy 5 compatibility: silently ignore NPE from VariableScopeVisitor
+        } catch (NullPointerException ignored) {
+            // Groovy 5 VariableScopeVisitor NPE - upstream bug
         }
 
         return renamedMethodNode

grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy

@@ -245,12 +245,12 @@ class AstUtils {
     }
 
     static void processVariableScopes(SourceUnit source, ClassNode classNode, MethodNode methodNode) {
-        // Groovy 5 changed how VariableScopeVisitor handles certain AST states.
-        // In some transformation scenarios, the visitor may throw NPE due to
-        // uninitialized scopes or missing AST nodes. Since variable scope processing
-        // is primarily for error reporting and doesn't affect code generation for
-        // transformations that have already set up their scopes, we can safely
-        // skip it when it fails.
+        // Groovy 5 (5.0.5 / 5.0.6-SNAPSHOT) throws NullPointerException from inside
+        // VariableScopeVisitor when invoked by certain Grails AST transformations
+        // (e.g. on `org.apache.grails.data.testing.tck.domains.DataServiceRoutingProductDataService`).
+        // The transformation has already produced valid bytecode by the time we get here -
+        // VariableScopeVisitor is only re-running for late scope validation - so swallowing
+        // the NPE keeps compilation green. To be filed upstream against Apache Groovy.
         try {
             VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(source)
             if (methodNode == null) {
@@ -259,10 +259,8 @@ class AstUtils {
                 scopeVisitor.prepareVisit(classNode)
                 scopeVisitor.visitMethod(methodNode)
             }
-        } catch (NullPointerException e) {
-            // Groovy 5 compatibility: silently ignore NPE from VariableScopeVisitor
-            // The transformation has already completed its work and the code will
-            // compile correctly without the scope validation.
+        } catch (NullPointerException ignored) {
+            // Groovy 5 VariableScopeVisitor NPE - upstream bug, see comment above
         }
     }
 

grails-logging/src/main/groovy/org/grails/compiler/logging/LoggingTransformer.java

@@ -80,10 +80,8 @@ public void performInjectionOnAnnotatedClass(SourceUnit source, ClassNode classN
             return;
         }
 
-        // Groovy 5 compatibility: Instead of adding @Slf4j annotation and running LogASTTransformation
-        // (which throws NPE in VariableScopeVisitor during canonicalization in Groovy 5),
-        // manually inject the log field. This mimics what @Slf4j does without triggering
-        // the VariableScopeVisitor codepath.
+        // Instead of adding @Slf4j annotation (which won't be processed if added during AST transformation),
+        // manually inject the log field. This mimics what @Slf4j does.
         // final Logger log = LoggerFactory.getLogger(ClassName.class)
         MethodCallExpression getLoggerCall = new MethodCallExpression(
                 new ClassExpression(LOGGER_FACTORY_CLASSNODE),