-
-
Notifications
You must be signed in to change notification settings - Fork 973
Add @GrailsCompileStatic support for controllers that call tag libs
#15669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/taglib-method-actions
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since detection is purely by name suffix, an inner class called something like |
||
| dynamicNamespaceProperties = [] as Set | ||
| } | ||
| } | ||
|
|
||
| afterVisitClass { ClassNode classNode -> | ||
| scopeExit() | ||
| } | ||
|
Comment on lines
+56
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor consistency note: the other class-scoped extensions in this package ( setup { newScope() }
finish { scopeExit() }The |
||
|
|
||
| unresolvedVariable { VariableExpression ve -> | ||
| if (currentScope?.isController) { | ||
| currentScope.dynamicNamespaceProperties << ve | ||
| return makeDynamic(ve) | ||
| } | ||
| null | ||
| } | ||
|
Comment on lines
+67
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: @GrailsCompileStatic
class BookController {
BookService bookSvc
def index() {
bookSvce.list() // typo - 'bookSvce' is unresolved
}
}Here 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 |
||
|
|
||
| unresolvedProperty { PropertyExpression pe -> | ||
| if (currentScope?.isController && isThisReceiver(pe)) { | ||
| currentScope.dynamicNamespaceProperties << pe | ||
| return makeDynamic(pe) | ||
| } | ||
| null | ||
| } | ||
|
Comment on lines
+75
to
+81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just flagging a subtle case here: this hook silences 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? |
||
|
|
||
| methodNotFound { receiver, name, argList, argTypes, call -> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you maybe add parameter types here for consistency with the other extensions in this package? They all use the fully typed form: methodNotFound { ClassNode receiver, String name, ArgumentListExpression argList, ClassNode[] argTypes, MethodCall call -> |
||
| if (!currentScope?.isController) return null | ||
| if (isThisReceiver(call)) return makeDynamic(call) | ||
| if (call instanceof MethodCallExpression && call.objectExpression in currentScope.dynamicNamespaceProperties) return makeDynamic(call) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cosmetic only - line 84 uses |
||
| null | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other extensions in this package all end |
||
|
|
||
| private boolean isThisReceiver(expr) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you maybe type this parameter (perhaps |
||
| if (!(expr instanceof MethodCallExpression || expr instanceof PropertyExpression)) return false | ||
| expr.implicitThis || (expr.objectExpression instanceof VariableExpression && expr.objectExpression.thisExpression) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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> { | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Not a blocker, just a thought. |
||
| void setup() { | ||
| mockTagLibs(CompileStaticDefaultTagLib, CompileStaticNamespacedTagLib) | ||
| } | ||
|
|
||
| void "controller with @GrailsCompileStatic can call a default-namespace tag directly"() { | ||
| when: | ||
| controller.useDefaultNamespaceTag() | ||
|
|
||
| then: | ||
| response.contentAsString == 'hello! World' | ||
| } | ||
|
|
||
| void "controller with @GrailsCompileStatic can call a tag via namespace dispatcher property"() { | ||
| when: | ||
| controller.useNamespacedTag() | ||
|
|
||
| then: | ||
| response.contentAsString == 'hello! World' | ||
| } | ||
| } | ||
|
|
||
| @Artefact('Controller') | ||
| @GrailsCompileStatic | ||
| class CompileStaticTagController { | ||
|
|
||
| def useDefaultNamespaceTag() { | ||
| // tag in default namespace invoked directly on this; dispatched at runtime | ||
| // through TagLibraryInvoker.methodMissing | ||
| response.writer << greet(name: 'World') | ||
| } | ||
|
|
||
| def useNamespacedTag() { | ||
| // namespace dispatcher property resolved at runtime through | ||
| // TagLibraryInvoker.propertyMissing, tag invoked on the resulting dispatcher | ||
| response.writer << cst.greet(name: 'World') | ||
| } | ||
| } | ||
|
|
||
| @Artefact('TagLib') | ||
| class CompileStaticDefaultTagLib { | ||
| Closure greet = { attrs, body -> | ||
| out << "hello! ${attrs.name}" | ||
| } | ||
| } | ||
|
|
||
| @Artefact('TagLib') | ||
| class CompileStaticNamespacedTagLib { | ||
| static namespace = 'cst' | ||
|
|
||
| Closure greet = { attrs, body -> | ||
| out << "hello! ${attrs.name}" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * 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.compiler.GrailsCompileStatic | ||
|
|
||
| /** | ||
| * Demonstrates that a controller annotated with {@code @GrailsCompileStatic} can | ||
| * invoke tag library methods — both in the default namespace (direct call) and via | ||
| * a namespace dispatcher property — without compile errors. | ||
| */ | ||
| @GrailsCompileStatic | ||
| class CompileStaticController { | ||
|
|
||
| def invokeDefaultNamespaceTag() { | ||
| // link() is a core tag in the default 'g' namespace; invoked directly on this | ||
| response.writer << link(controller: 'demo', action: 'clearDatabase') | ||
| } | ||
|
|
||
| def invokeNamespacedTag() { | ||
| // one.sayHello() accesses the 'one' namespace dispatcher, then invokes the tag | ||
| response.writer << one.sayHello() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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'() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiny convenience nit: this integration spec and the unit spec at Could you maybe rename one to something like |
||
| when: | ||
| def response = http('/compileStatic/invokeDefaultNamespaceTag') | ||
|
|
||
| then: | ||
| response.assertContains('<a href="/demo/clearDatabase"></a>') | ||
| } | ||
|
|
||
| void 'controller with @GrailsCompileStatic can call a tag via namespace dispatcher property'() { | ||
| when: | ||
| def response = http('/compileStatic/invokeNamespacedTag') | ||
|
|
||
| then: | ||
| response.assertEquals('BEFORE Hello From SecondTagLib AFTER') | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.web.controllers.ControllerUnitTest | ||
| import spock.lang.Specification | ||
|
|
||
| class CompileStaticControllerSpec extends Specification implements ControllerUnitTest<CompileStaticController> { | ||
|
|
||
| void setup() { | ||
| mockTagLibs FirstTagLib, SecondTagLib | ||
| } | ||
|
|
||
| void 'controller with @GrailsCompileStatic can call a default-namespace tag directly'() { | ||
| when: | ||
| controller.invokeDefaultNamespaceTag() | ||
|
|
||
| then: | ||
| response.text == '<a href="/demo/clearDatabase"></a>' | ||
| } | ||
|
|
||
| void 'controller with @GrailsCompileStatic can call a tag via namespace dispatcher property'() { | ||
| when: | ||
| controller.invokeNamespacedTag() | ||
|
|
||
| then: | ||
| response.text == 'BEFORE Hello From SecondTagLib AFTER' | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny thing - since this PR targets
8.0.0-SNAPSHOTandwhatsNew.adocdescribes the change under "introduced in Grails 8",@since 8.0would maybe be more accurate here?