jamesfredley commented on code in PR #15669: URL: https://github.com/apache/grails-core/pull/15669#discussion_r3276296471
########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } Review Comment: This part might be a bit broader than necessary - marking *every* unresolved variable in a controller as dynamic could end up silencing genuine typos. For example: ```groovy @GrailsCompileStatic class BookController { BookService bookSvc def index() { bookSvce.list() // typo - 'bookSvce' is unresolved } } ``` Here `bookSvce` would be silently made dynamic (and added to `dynamicNamespaceProperties`, so the subsequent `.list()` call is also silenced) instead of producing the compile error `@GrailsCompileStatic` users probably expect. Could it maybe be worth narrowing this? One option would be to defer the dynamic mark until the variable is actually used as the receiver of a method call (i.e. only silence the namespace-dispatcher access pattern `<ident>.<method>(...)`), so standalone typos still surface. Just a thought - happy to be wrong if there's a reason you've gone broader. ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } + + unresolvedProperty { PropertyExpression pe -> + if (currentScope?.isController && isThisReceiver(pe)) { + currentScope.dynamicNamespaceProperties << pe + return makeDynamic(pe) + } + null + } Review Comment: Just flagging a subtle case here: this hook silences `def t = link` at compile time, but at runtime `TagLibraryInvoker.propertyMissing` only returns a `NamespacedTagDispatcher` for namespace names - it doesn't return a `Closure` for tag-property access the way `TagLibrary.propertyMissing` does inside a tag library itself. So a controller with `def t = link` would compile cleanly but throw `MissingPropertyException` at runtime. Probably fine as a known limitation, but maybe worth a Javadoc note here (or a line in the user docs) so the boundary is explicit? ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 Review Comment: Tiny thing - since this PR targets `8.0.0-SNAPSHOT` and `whatsNew.adoc` describes the change under "introduced in Grails 8", `@since 8.0` would maybe be more accurate here? ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } + + unresolvedProperty { PropertyExpression pe -> + if (currentScope?.isController && isThisReceiver(pe)) { + currentScope.dynamicNamespaceProperties << pe + return makeDynamic(pe) + } + null + } + + methodNotFound { receiver, name, argList, argTypes, call -> + if (!currentScope?.isController) return null + if (isThisReceiver(call)) return makeDynamic(call) + if (call instanceof MethodCallExpression && call.objectExpression in currentScope.dynamicNamespaceProperties) return makeDynamic(call) Review Comment: Cosmetic only - line 84 uses `currentScope?.isController` but this line dereferences `currentScope.dynamicNamespaceProperties` directly. They're functionally equivalent given the guard above, but `currentScope?.dynamicNamespaceProperties` here might read a touch more uniformly? ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } + + unresolvedProperty { PropertyExpression pe -> + if (currentScope?.isController && isThisReceiver(pe)) { + currentScope.dynamicNamespaceProperties << pe + return makeDynamic(pe) + } + null + } + + methodNotFound { receiver, name, argList, argTypes, call -> + if (!currentScope?.isController) return null + if (isThisReceiver(call)) return makeDynamic(call) + if (call instanceof MethodCallExpression && call.objectExpression in currentScope.dynamicNamespaceProperties) return makeDynamic(call) + null + } + } + + private boolean isThisReceiver(expr) { Review Comment: Could you maybe type this parameter (perhaps `Expression expr`)? Helps line up with the rest of the codebase's static-typing lean. ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) Review Comment: Since detection is purely by name suffix, an inner class called something like `FooController` declared inside, say, a service would also receive the silencing treatment. Probably rare in practice - but might be worth a Javadoc note so the behavior is documented? ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } + + unresolvedProperty { PropertyExpression pe -> + if (currentScope?.isController && isThisReceiver(pe)) { + currentScope.dynamicNamespaceProperties << pe + return makeDynamic(pe) + } + null + } + + methodNotFound { receiver, name, argList, argTypes, call -> Review Comment: Could you maybe add parameter types here for consistency with the other extensions in this package? They all use the fully typed form: ```groovy methodNotFound { ClassNode receiver, String name, ArgumentListExpression argList, ClassNode[] argTypes, MethodCall call -> ``` ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } Review Comment: Minor consistency note: the other class-scoped extensions in this package (`ValidateableTypeCheckingExtension`, `DomainMappingTypeCheckingExtension`, `NamedQueryTypeCheckingExtension`) pair `beforeVisitClass` / `afterVisitClass` with an outer `setup { newScope() }` / `finish { scopeExit() }` guard: ```groovy setup { newScope() } finish { scopeExit() } ``` The `currentScope?` null-safe accesses below make this functionally safe without them, but could you maybe add the outer pair to keep this in lockstep with the rest of the package? ########## grails-core/src/main/groovy/org/grails/compiler/ControllerTagLibTypeCheckingExtension.groovy: ########## @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.compiler + +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.grails.core.artefact.ControllerArtefactHandler + +/** + * A type-checking extension that allows {@code @GrailsCompileStatic} controllers + * to invoke tag library methods without compile-time errors. + * + * <p>Tag calls in controllers are dispatched at runtime through + * {@code TagLibraryInvoker#methodMissing} and + * {@code TagLibraryInvoker#propertyMissing}. These hooks are + * invisible to the static type checker, so this extension marks the affected + * expressions as dynamic, silencing the false-positive errors while preserving + * full type checking for all other code in the controller. + * + * <p>Controller detection mirrors {@code ControllerActionTransformer}: a class is + * treated as a controller when its qualified name ends with {@code "Controller"}. + * + * <p>Two calling patterns are supported: + * <ul> + * <li>Direct calls on {@code this}: {@code link(controller: 'home')}, + * {@code message(code: 'key')}</li> + * <li>Namespaced calls via a namespace dispatcher property: + * {@code g.message(code: 'key')}, {@code my.customTag(attr: 'val')}</li> + * </ul> + * + * @since 7.0 + */ +class ControllerTagLibTypeCheckingExtension extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + @Override + Object run() { + beforeVisitClass { ClassNode classNode -> + newScope { + isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) + dynamicNamespaceProperties = [] as Set + } + } + + afterVisitClass { ClassNode classNode -> + scopeExit() + } + + unresolvedVariable { VariableExpression ve -> + if (currentScope?.isController) { + currentScope.dynamicNamespaceProperties << ve + return makeDynamic(ve) + } + null + } + + unresolvedProperty { PropertyExpression pe -> + if (currentScope?.isController && isThisReceiver(pe)) { + currentScope.dynamicNamespaceProperties << pe + return makeDynamic(pe) + } + null + } + + methodNotFound { receiver, name, argList, argTypes, call -> + if (!currentScope?.isController) return null + if (isThisReceiver(call)) return makeDynamic(call) + if (call instanceof MethodCallExpression && call.objectExpression in currentScope.dynamicNamespaceProperties) return makeDynamic(call) + null + } + } Review Comment: The other extensions in this package all end `run()` with an explicit `null`. Could you maybe add one here for consistency? Just a style nit. ########## grails-gsp/plugin/src/test/groovy/org/grails/web/taglib/ControllerCompileStaticTagLibSpec.groovy: ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.taglib + +import grails.artefact.Artefact +import grails.compiler.GrailsCompileStatic +import grails.testing.web.controllers.ControllerUnitTest +import spock.lang.Specification + +class ControllerCompileStaticTagLibSpec extends Specification implements ControllerUnitTest<CompileStaticTagController> { + Review Comment: Could it maybe be worth adding a negative test alongside these positive ones? Something pinning down the boundary - e.g. a spec asserting that a genuinely undefined reference inside an `@GrailsCompileStatic` controller still fails to compile - would help guard against the extension drifting to silence more than intended in future changes. Not a blocker, just a thought. ########## grails-test-examples/demo33/src/integration-test/groovy/demo/CompileStaticControllerSpec.groovy: ########## @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package demo + +import grails.testing.mixin.integration.Integration +import org.apache.grails.testing.http.client.HttpClientSupport +import spock.lang.Specification +import spock.lang.Tag + +@Integration +@Tag('http-client') +class CompileStaticControllerSpec extends Specification implements HttpClientSupport { + + void 'controller with @GrailsCompileStatic can call a default-namespace tag directly'() { Review Comment: Tiny convenience nit: this integration spec and the unit spec at `src/test/groovy/demo/CompileStaticControllerSpec.groovy` share the same fully qualified name `demo.CompileStaticControllerSpec`. Different source sets so the build is fine, but `--tests "demo.CompileStaticControllerSpec"` becomes ambiguous when running them individually. Could you maybe rename one to something like `CompileStaticControllerIntegrationSpec` (or `CompileStaticControllerHttpSpec`)? -- 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]
