Add @GrailsCompileStatic support for controllers that call tag libs#15669
Add @GrailsCompileStatic support for controllers that call tag libs#15669jdaugherty wants to merge 2 commits into
@GrailsCompileStatic support for controllers that call tag libs#15669Conversation
jdaugherty
force-pushed
the
feature/compileStaticTagLibs
branch
2 times, most recently
from
14b0664 to
436b082
Compare
jdaugherty
force-pushed
the
feature/compileStaticTagLibs
branch
from
436b082 to
e5d44d2
Compare
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Nice work on this - it tackles a real long-standing pain point and the architectural choice (a TypeCheckingExtension hooking the methodMissing / propertyMissing weave from TagLibraryInvoker) looks right. CI is green across all 31 jobs.
Left some inline notes below - mostly minor style/consistency things, plus a couple of behavior cases that might be worth a second look (the broadness of unresolvedVariable and the tag-as-property closure form). None are dealbreakers, just thoughts.
| unresolvedVariable { VariableExpression ve -> | ||
| if (currentScope?.isController) { | ||
| currentScope.dynamicNamespaceProperties << ve | ||
| return makeDynamic(ve) | ||
| } | ||
| null | ||
| } |
There was a problem hiding this 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:
@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.
There was a problem hiding this comment.
I'm still investigating this one.
There was a problem hiding this comment.
I tested removing dynamicNamespaceProperties entirely: the tests fail because makeDynamic(ve) alone is insufficient; methodNotFound does fire on dynamic receivers and must
explicitly make the call dynamic. So the tracking is structurally required for namespace dispatch to work.
The only genuine fixes are:
- Whitelist known namespace names — scan all TagLib classes at compile time, collect their static namespace values, and only make variables dynamic if the name matches a
known namespace. This is architecturally sound but requires significant infrastructure. - Accept the limitation — document it (done in the Javadoc "Scope note") and rely on the fact that bookSvce.list() fails immediately at runtime with
MissingPropertyException.
The negative tests I added do verify a meaningful guarantee: method calls on declared types still fail — bookService.nonExistentMethod() (where bookService is a declared
field) produces a compile error. The silencing only affects undeclared identifiers.
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| private boolean isThisReceiver(expr) { |
There was a problem hiding this comment.
Could you maybe type this parameter (perhaps Expression expr)? Helps line up with the rest of the codebase's static-typing lean.
| Object run() { | ||
| beforeVisitClass { ClassNode classNode -> | ||
| newScope { | ||
| isController = classNode.name.endsWith(ControllerArtefactHandler.TYPE) |
There was a problem hiding this 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?
There was a problem hiding this comment.
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.
ControllerTagLibTypeCheckingExtension.groovy — 6 feedback items addressed: 1. @SInCE 8.0 — fixed from 7.0 to match the actual 8.0.0-SNAPSHOT project version 2. Outer scope guards — added setup { newScope() } / finish { scopeExit() } for consistency with CriteriaTypeCheckingExtension and other extensions 3. Explicit methodNotFound types — ClassNode receiver, String name, ArgumentListExpression argList, ClassNode[] argTypes, MethodCall call 4. Typed isThisReceiver parameter — Expression expr 5. Explicit return null at end of run() 6. Javadoc updates — documents both the tag-as-property limitation (def t = link compiles but throws MissingPropertyException at runtime) and the inner class naming behavior CompileStaticControllerIntegrationSpec.groovy — renamed from CompileStaticControllerSpec (both filename and class name) to eliminate the FQN collision with the unit test. ControllerTagLibTypeCheckingExtensionSpec.groovy (new) — 3 negative tests verifying that the extension does not suppress errors for declared types: wrong method on a service field, type mismatch, and wrong method on a declared local variable.
✅ All tests passed ✅🏷️ Commit: 58acccf Learn more about TestLens at testlens.app. |
2 participants
Fixes #14023