-
-
Notifications
You must be signed in to change notification settings - Fork 974
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: 8.0.x
Are you sure you want to change the base?
Changes from 1 commit
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
Contributor
Author
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. Updated the javadoc. This issue exists across the Grails code base though, so we may want to consider annotating controllers so the compiler could then reliably detect a controller instead of being name driven. This probably worth a ticket. |
||
| dynamicNamespaceProperties = [] as Set | ||
| } | ||
| } | ||
|
|
||
| afterVisitClass { ClassNode classNode -> | ||
| scopeExit() | ||
| } | ||
|
jdaugherty marked this conversation as resolved.
|
||
|
|
||
| unresolvedVariable { VariableExpression ve -> | ||
| if (currentScope?.isController) { | ||
| currentScope.dynamicNamespaceProperties << ve | ||
| return makeDynamic(ve) | ||
| } | ||
| null | ||
| } | ||
|
Comment on lines
+91
to
+97
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
Contributor
Author
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. I'm still investigating this one.
Contributor
Author
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. I tested removing dynamicNamespaceProperties entirely: the tests fail because makeDynamic(ve) alone is insufficient; methodNotFound does fire on dynamic receivers and must The only genuine fixes are:
The negative tests I added do verify a meaningful guarantee: method calls on declared types still fail — bookService.nonExistentMethod() (where bookService is a declared
Contributor
Author
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 only other solution would be to somehow notify the compiler of namespaces on the classpath, which would require saving the known namespaces and accessing them in the compiler. This is probably possible, but out of scope of this change. Ithink this is a net benefit, but we probably should ticket this as an enhancement |
||
|
|
||
| unresolvedProperty { PropertyExpression pe -> | ||
| if (currentScope?.isController && isThisReceiver(pe)) { | ||
| currentScope.dynamicNamespaceProperties << pe | ||
| return makeDynamic(pe) | ||
| } | ||
| null | ||
| } | ||
|
jdaugherty marked this conversation as resolved.
|
||
|
|
||
| methodNotFound { receiver, name, argList, argTypes, call -> | ||
|
jdaugherty marked this conversation as resolved.
Outdated
|
||
| if (!currentScope?.isController) return null | ||
| if (isThisReceiver(call)) return makeDynamic(call) | ||
| if (call instanceof MethodCallExpression && call.objectExpression in currentScope.dynamicNamespaceProperties) return makeDynamic(call) | ||
|
jdaugherty marked this conversation as resolved.
|
||
| null | ||
| } | ||
| } | ||
|
jdaugherty marked this conversation as resolved.
|
||
|
|
||
| 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> { | ||
|
|
||
|
jdaugherty marked this conversation as resolved.
|
||
| 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'() { | ||
| 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' | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.