jdaugherty commented on code in PR #16134: URL: https://github.com/apache/grails-core/pull/16134#discussion_r3792000458
########## grails-gsp/grails-web-taglib/src/main/groovy/grails/gsp/taglib/compiler/CompiledTagCallRewriter.java: ########## @@ -0,0 +1,519 @@ +/* + * 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 grails.gsp.taglib.compiler; + +import java.util.Collections; +import java.util.List; +import java.util.Set; + +import org.codehaus.groovy.ast.AnnotationNode; +import org.codehaus.groovy.ast.ClassCodeExpressionTransformer; +import org.codehaus.groovy.ast.ClassHelper; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.ConstructorNode; +import org.codehaus.groovy.ast.DynamicVariable; +import org.codehaus.groovy.ast.FieldNode; +import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.Parameter; +import org.codehaus.groovy.ast.Variable; +import org.codehaus.groovy.ast.expr.ArgumentListExpression; +import org.codehaus.groovy.ast.expr.ClosureExpression; +import org.codehaus.groovy.ast.expr.ConstantExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.MapExpression; +import org.codehaus.groovy.ast.expr.MethodCallExpression; +import org.codehaus.groovy.ast.expr.PropertyExpression; +import org.codehaus.groovy.ast.expr.StaticMethodCallExpression; +import org.codehaus.groovy.ast.expr.TupleExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.control.SourceUnit; + +import org.grails.compiler.injection.GrailsASTUtils; +import org.grails.taglib.CompiledTagInvocation; +import org.grails.taglib.discovery.TagLibraryAstDiscovery; +import org.grails.taglib.index.TagLibraryIndex; + +/** + * Rewrites a call to a known tag into a direct invocation. + * + * <p>Writing {@code g.message(code: 'x')} reaches the tag library through {@code propertyMissing} to + * find the namespace and {@code invokeMethod} to find the tag, which is a dynamic call site even in a + * statically compiled class. Both the namespace and the tag name are fixed in the source, and the tag + * library index says whether that tag exists, so the call is replaced with a call to + * {@link CompiledTagInvocation}, an ordinary static method call. + * + * <p>The tag is still selected by name at runtime, through the same lookup the dynamic path uses, so + * a tag library registered later, one that overrides another, and the order tag libraries are + * registered in all decide the outcome exactly as they did before. Nothing is bound to a particular + * tag library class. + * + * <p>A namespace the index does not know is left alone, which is what keeps a tag library registered + * at runtime working, as is a name that something else in scope already answers to. + * + * @since 8.0.0 + */ +public class CompiledTagCallRewriter extends ClassCodeExpressionTransformer { + + private static final ClassNode INVOCATION_TYPE = ClassHelper.make(CompiledTagInvocation.class); + private static final String LOOKUP_ACCESSOR = "getTagLibraryLookup"; + private static final String OUTPUT_CONTEXT_ACCESSOR = "getOutputContext"; + private static final String INVOKE = "invoke"; + private static final String INVOKE_ARGUMENTS = "invokeArguments"; + private static final String INVOKE_ARGUMENTS_IN_CONTEXT = "invokeArgumentsInContext"; + private static final String GROOVY_PAGE_TYPE = "org.grails.gsp.GroovyPage"; + private static final String COMPILE_STATIC_TYPE = "groovy.transform.CompileStatic"; + private static final String GRAILS_COMPILE_STATIC_TYPE = "grails.compiler.GrailsCompileStatic"; + private static final String MARKUP_TAG_CALL = "invokeTag"; + private static final String DEFAULT_NAMESPACE = "g"; + + /** + * Names the dispatch treats as its own before it ever considers a tag, so an unqualified call to + * one of them is not a tag call however the index reads. + */ + private static final Set<String> RESERVED_NAMES = Set.of("body", "render"); + + private static final String REWRITTEN_MARKER = CompiledTagCallRewriter.class.getName(); + + private final SourceUnit sourceUnit; + private final TagLibraryIndex index; + private final ClassNode classNode; + private final String callerNamespace; + private final boolean page; + private final boolean rewritingPermitted; + private Set<String> localNames = Collections.emptySet(); + private Set<String> pageBindings = Collections.emptySet(); + private int rewritten; + + public CompiledTagCallRewriter(SourceUnit sourceUnit, TagLibraryIndex index, ClassNode classNode) { + this.sourceUnit = sourceUnit; + this.index = index; + this.classNode = classNode; + this.page = isGroovyPage(classNode); + // A page resolves a name against the model it was rendered with before it reaches a tag + // library, and that model is not visible here, so rewriting a page's tag call can only be + // sound where the page has given up dynamic resolution. Declaring compileStatic is that: it + // reserves the namespace names for tag libraries. A page that has not declared it keeps + // resolving its tags exactly as before. + this.rewritingPermitted = !this.page || isCompileStatic(classNode); + // An unqualified call is offered to the caller's own namespace before the default one, which is + // what a tag library declaring a namespace does at runtime. A page and a controller have no + // namespace of their own, so for them the two are the same. + String declared = this.page ? DEFAULT_NAMESPACE : TagLibraryAstDiscovery.resolveNamespace(classNode); + this.callerNamespace = declared != null ? declared : DEFAULT_NAMESPACE; + } + + /** + * @return how many calls were rewritten, for tests to assert against + */ + public int getRewrittenCount() { + return rewritten; + } + + public void rewrite() { + // A tag library is reached both as an artefact and as a class carrying the invoker trait, so + // rewriting can be asked for twice. Rewriting again would be harmless, but reporting a + // misspelled tag twice would not be. + if (classNode.getNodeMetaData(REWRITTEN_MARKER) != null) { + return; + } + classNode.putNodeMetaData(REWRITTEN_MARKER, Boolean.TRUE); + if (page) { + pageBindings = PageBindingCollector.collect(classNode); + } + for (MethodNode method : classNode.getMethods()) { + // getMethods() reaches inherited methods, whose bodies belong to the class that declared + // them. Rewriting one here would change a superclass through a subclass that happens to be + // able to call tags. Trait methods are woven as declarations on this class and so remain. + if (method.getDeclaringClass() != null && !classNode.equals(method.getDeclaringClass())) { + continue; + } + if (method.getCode() != null && !method.isAbstract()) { + rewriteBody(method.getCode(), method.getParameters()); + } + } + for (ConstructorNode constructor : classNode.getDeclaredConstructors()) { + if (constructor.getCode() != null) { + rewriteBody(constructor.getCode(), constructor.getParameters()); + } + } + for (FieldNode field : classNode.getFields()) { + if (field.getDeclaringClass() != null && !classNode.equals(field.getDeclaringClass())) { + continue; + } + Expression initial = field.getInitialExpression(); + if (initial != null) { + localNames = Collections.emptySet(); + field.setInitialValueExpression(transform(initial)); + } + } + for (Statement statement : classNode.getObjectInitializerStatements()) { + rewriteBody(statement, null); + } + } + + private void rewriteBody(Statement code, Parameter[] parameters) { + // An unqualified call reaches a tag only when nothing nearer answers to the name, and a local + // holding a closure answers to it. Which locals are in scope at a given point is not tracked + // here: a name declared anywhere in the body is treated as claimed throughout it, which can + // leave a call dispatched dynamically but never sends one to the wrong place. + localNames = LocalNameCollector.collect(code, parameters); + visitClassCodeContainer(code); + } + + @Override + protected SourceUnit getSourceUnit() { + return sourceUnit; + } + + @Override + public Expression transform(Expression expression) { + if (expression instanceof ClosureExpression closure) { + // ClassCodeExpressionTransformer deliberately does not descend into closures, and documents + // this override as the way to reach them. Without it a tag call written in a tag body, in a + // withFormat block, or in anything else taking a closure is never resolved - which is most + // of the tag calls in a real tag library. + closure.visit(this); + return closure; + } + if (expression instanceof MethodCallExpression call) { + Expression rewrite = rewriteTagCall(call); + if (rewrite != null) { + rewritten++; + return rewrite; + } + validateMarkupTagCall(call); + } + return super.transform(expression); + } + + private Expression rewriteTagCall(MethodCallExpression call) { + if (!(call.getMethod() instanceof ConstantExpression methodName) || + methodName.getValue() == null) { + return null; + } + String tagName = methodName.getValue().toString(); + String namespace = namespaceOf(call.getObjectExpression()); + if (namespace != null) { + // A namespace the build declared as filled in at runtime is left alone entirely: that + // declaration is how an application says its tags are decided while it runs, whether by a + // tag library registered then or by metaprogramming, and binding a call now would settle + // what it asked to keep open. + if (index.isDynamicNamespace(namespace)) { + return null; + } + if (!index.hasNamespace(namespace) || isShadowed(call.getObjectExpression(), namespace) || + pageBindings.contains(namespace)) { + return null; + } + if (!index.isKnown(namespace, tagName)) { + // Only where the name means a tag library for certain. In a page that has not given up + // dynamic resolution the receiver may just as well be the model it was rendered with, + // and reporting there would reject a call this release deliberately still allows. + if (this.rewritingPermitted) { + reportUnknownTag(namespace, tagName, call); + } + return null; + } + } + else { + namespace = unqualifiedNamespaceOf(call, tagName); + if (namespace == null) { + return null; + } + } + if (!this.rewritingPermitted) { + return null; + } + Expression invocation = invocation(namespace, tagName, call.getArguments()); + if (invocation != null) { + // Kept where the tag was written, so a stack trace and any later diagnostic still point at + // the line the author wrote rather than at the start of the file. + setSourcePosition(invocation, call); + } + return invocation; + } + + /** + * The namespace an unqualified call such as {@code message(code: 'x')} resolves in. + * + * <p>Only a name nothing else answers to reaches a tag at all: a real method of the class, an + * inherited one, a field, a property or a local wins, and whether such a member exists is what + * decides the call. Where nothing claims the name, dispatch offers it to the caller's own + * namespace and then to the default one, which is the order reproduced here. + * + * @return the namespace to invoke in, or {@code null} when the call is not resolvably a tag + */ + private String unqualifiedNamespaceOf(MethodCallExpression call, String tagName) { + if (page) { + // A page resolves an unqualified name against its binding before it reaches a tag, and what + // a page's binding holds - the model it was rendered with - is not visible here. A call + // written with its namespace says which tag library it means and is rewritten; one without + // is left to resolve as it did. + return null; + } + if (!call.isImplicitThis() || RESERVED_NAMES.contains(tagName)) { + return null; + } + if (declaresMember(tagName) || localNames.contains(tagName)) { + return null; + } + if (index.isDynamicNamespace(callerNamespace) || index.isDynamicNamespace(DEFAULT_NAMESPACE)) { + // The namespaces an unqualified call could reach were declared as decided at runtime. + return null; + } + if (index.isKnown(callerNamespace, tagName)) { + return callerNamespace; + } + if (index.isKnown(DEFAULT_NAMESPACE, tagName)) { + return DEFAULT_NAMESPACE; + } + // Not a tag this build knows about. It is not reported: an unqualified name in a controller is + // as likely to be a dynamic finder, an injected service method or anything else contributed at + // runtime as it is a misspelled tag. + return null; + } + + /** + * Builds the invocation, passing the attributes and body directly where the source says what they + * are and forwarding the arguments as written where it does not. + */ + private Expression invocation(String namespace, String tagName, Expression arguments) { + if (!(arguments instanceof TupleExpression tuple)) { + return null; + } + ArgumentListExpression invocationArgs = new ArgumentListExpression(); + invocationArgs.addExpression(new MethodCallExpression(VariableExpression.THIS_EXPRESSION, + LOOKUP_ACCESSOR, MethodCallExpression.NO_ARGUMENTS)); + invocationArgs.addExpression(new ConstantExpression(namespace)); + invocationArgs.addExpression(new ConstantExpression(tagName)); + + Expression[] attrsAndBody = attributesAndBody(tuple); + if (attrsAndBody != null) { + invocationArgs.addExpression(attrsAndBody[0]); + invocationArgs.addExpression(attrsAndBody[1]); + if (page) { + invocationArgs.addExpression(outputContext()); + } + return new StaticMethodCallExpression(INVOCATION_TYPE, INVOKE, invocationArgs); + } + + // The shape is only known once the arguments have been evaluated - a map held in a variable, a + // single value the tag reads under its own name, and so on - so they are forwarded as written + // and sorted out by the same rules the dynamic path applies. + if (page) { + invocationArgs.addExpression(outputContext()); + } + for (Expression argument : tuple.getExpressions()) { + invocationArgs.addExpression(transform(argument)); + } + return new StaticMethodCallExpression(INVOCATION_TYPE, + page ? INVOKE_ARGUMENTS_IN_CONTEXT : INVOKE_ARGUMENTS, invocationArgs); + } + + private Expression outputContext() { + return new MethodCallExpression(VariableExpression.THIS_EXPRESSION, OUTPUT_CONTEXT_ACCESSOR, + MethodCallExpression.NO_ARGUMENTS); + } + + /** + * @return the attributes and body to pass, or {@code null} when the source does not say what they + * are and the arguments have to be forwarded instead + */ + private Expression[] attributesAndBody(TupleExpression tuple) { + List<Expression> args = tuple.getExpressions(); + Expression noAttributes = new MapExpression(); + Expression noBody = ConstantExpression.NULL; + switch (args.size()) { + case 0: + return new Expression[] { noAttributes, noBody }; + case 1: + if (args.get(0) instanceof MapExpression) { + return new Expression[] { transform(args.get(0)), noBody }; + } + if (args.get(0) instanceof ClosureExpression) { + return new Expression[] { noAttributes, transform(args.get(0)) }; + } + return null; + case 2: + if (args.get(0) instanceof MapExpression && args.get(1) instanceof ClosureExpression) { + return new Expression[] { transform(args.get(0)), transform(args.get(1)) }; + } + return null; + default: + return null; + } + } + + /** + * Checks a tag written as markup, which a page compiles into a call naming the tag and namespace + * directly. Such a call is already an ordinary method call and needs no rewriting, but the names in + * it are worth the same check as the ones written in an expression. + */ + private void validateMarkupTagCall(MethodCallExpression call) { + if (!page || !MARKUP_TAG_CALL.equals(call.getMethodAsString()) || + !(call.getArguments() instanceof TupleExpression tuple) || + tuple.getExpressions().size() < 2) { + return; + } + if (!(tuple.getExpression(0) instanceof ConstantExpression tagName) || + !(tuple.getExpression(1) instanceof ConstantExpression namespace) || + tagName.getValue() == null || namespace.getValue() == null) { + return; + } + String namespaceName = namespace.getValue().toString(); + String tag = tagName.getValue().toString(); + if (index.isDynamicNamespace(namespaceName)) { + return; + } + if (index.hasNamespace(namespaceName) && !index.isKnown(namespaceName, tag)) { + reportUnknownTag(namespaceName, tag, call); + } + } + + /** + * Reports a tag that no compiled tag library declares. + * + * <p>Silent unless the build declared its tag libraries complete. A namespace holding some + * compiled tag libraries is not the same as one holding all of them: a plugin built before + * descriptors existed contributes tags to {@code g} without one, and a tag library registered + * while an application runs contributes more. Reporting by default would mean warning about calls + * that are perfectly correct - this framework calls one such tag itself - so a build says when it + * knows better. + */ + private void reportUnknownTag(String namespace, String tagName, Expression call) { + if (!index.isStrict() || index.isDynamicNamespace(namespace)) { + return; + } + if (!index.isNamespaceComplete(namespace)) { + // Something contributing to this namespace could not be described. A tag missing from it + // is as likely to be one of those as a misspelling, and reporting it would fail a build + // over code that is correct. + return; + } + String message = "No such tag [" + tagName + "] in namespace [" + namespace + "]. Known tags: " + + String.join(", ", index.getTagNames(namespace)); + // Collected rather than fatal, so that every misspelling in a file is reported at once instead + // of one per build. + GrailsASTUtils.error(sourceUnit, call, message, false); + } + + /** + * @return the namespace a call is made through, or {@code null} when the receiver is not a plain + * name that could be one + */ + private static String namespaceOf(Expression objectExpression) { + if (objectExpression instanceof VariableExpression variable) { + return variable.isThisExpression() || variable.isSuperExpression() ? null : variable.getName(); + } + if (objectExpression instanceof PropertyExpression property && + property.getObjectExpression() instanceof VariableExpression receiver && + receiver.isThisExpression()) { + return property.getPropertyAsString(); + } + return null; + } + + /** + * Whether something in scope has already claimed the name, in which case it is that thing rather + * than a tag library namespace. + * + * <p>A namespace is not declared anywhere: it is reached because nothing else answers to the name. + * A local variable, a parameter or a field called {@code g} does answer to it, and rewriting such a + * call would silently send it to a tag library instead of the object the author wrote. + */ + private boolean isShadowed(Expression objectExpression, String namespace) { + if (objectExpression instanceof VariableExpression variable) { + Variable accessed = variable.getAccessedVariable(); + // A name that resolves to something - a local, a parameter, a field, a property - is that + // thing. Only a name nothing has claimed is left to mean a namespace. + if (accessed != null && !(accessed instanceof DynamicVariable)) { + return true; + } + } + // Reached as this.g, or as a bare name resolved dynamically: a field or property of that name + // anywhere in the hierarchy is the member, not a namespace. + return declaresProperty(namespace); + } + + /** + * Whether the class, or anything it inherits from, reads a property of this name. A method of the + * same name does not count: {@code g.link()} reads {@code g} as a property whatever methods exist. + */ + private boolean declaresProperty(String name) { + return classNode.getField(name) != null || + classNode.getProperty(name) != null || + hasGetter(name); + } + + /** + * Whether the class, or anything it inherits from, already answers to a name at all. An + * unqualified call reaches a tag only when nothing else does, so here a method counts too. + */ + private boolean declaresMember(String name) { Review Comment: **Blocking — unqualified rewriting ignores `DefaultGroovyMethods` and extension modules.** `declaresMember` only inspects the `ClassNode`'s own and inherited methods/fields/properties. It cannot see anything the metaclass supplies, so DGM and extension-module methods never claim their name. Reproduced on this branch. Tag library: ```groovy @TagLib class DgmTagLib { static namespace = 'g' def sleep(Map attrs) { out << 'slept' } } ``` Controller, compiled with that descriptor on the classpath: ```groovy class DgmController { def index() { sleep(100); 'done' } } ``` The class file contains `CompiledTagInvocation.invokeArguments(lookup, "g", "sleep", 100)`. Before this PR the call reached `DefaultGroovyMethods.sleep(Object, long)` — `methodMissing` was never involved, because DGM methods *are* registered on the metaclass. So this is a silent behaviour change, not a missed optimisation. `RESERVED_NAMES` is `{"body", "render"}`. The exposed surface is every DGM name that applies to `Object`: `print`, `println`, `printf`, `sprintf`, `sleep`, `with`, `tap`, `use`, `dump`, `inspect`, `is`, `identity`, `each`, `find`, `every`, `any`, `collect`, `grep`, `sort`, `sum`, `min`, `max`, `getAt`, `putAt`, `asType`, `respondsTo`, `hasProperty`, `addShutdownHook` — plus anything an extension module a project or plugin registers. It is worse than the `g` namespace alone suggests: `unqualifiedNamespaceOf` tries `callerNamespace` **before** `g`, so a tag library whose own namespace declares a tag called `each` breaks every unqualified `each { }` in every other tag library of that namespace. The upgrade guide covers `doWithDynamicMethods` but not this, and this one is far easier to hit by accident because it needs no dynamic registration at all — just a name collision. Options, roughly in order of preference: 1. seed `RESERVED_NAMES` from `DefaultGroovyMethods` / `DefaultGroovyStaticMethods` plus `ExtensionModuleScanner`, so any name the metaclass would answer is left alone; 2. consult `GroovySystem.getMetaClassRegistry().getMetaClass(Object)` for the receiver's meta-methods; 3. make unqualified rewriting opt-in, and rewrite only namespaced calls by default. Whichever way it goes, this needs a test: a tag whose name collides with a DGM method, asserting the DGM method still wins. ########## grails-gsp/grails-web-taglib/src/main/groovy/grails/gsp/taglib/compiler/CompiledTagCallRewriter.java: ########## @@ -0,0 +1,519 @@ +/* + * 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 grails.gsp.taglib.compiler; + +import java.util.Collections; +import java.util.List; +import java.util.Set; + +import org.codehaus.groovy.ast.AnnotationNode; +import org.codehaus.groovy.ast.ClassCodeExpressionTransformer; +import org.codehaus.groovy.ast.ClassHelper; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.ConstructorNode; +import org.codehaus.groovy.ast.DynamicVariable; +import org.codehaus.groovy.ast.FieldNode; +import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.Parameter; +import org.codehaus.groovy.ast.Variable; +import org.codehaus.groovy.ast.expr.ArgumentListExpression; +import org.codehaus.groovy.ast.expr.ClosureExpression; +import org.codehaus.groovy.ast.expr.ConstantExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.MapExpression; +import org.codehaus.groovy.ast.expr.MethodCallExpression; +import org.codehaus.groovy.ast.expr.PropertyExpression; +import org.codehaus.groovy.ast.expr.StaticMethodCallExpression; +import org.codehaus.groovy.ast.expr.TupleExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.control.SourceUnit; + +import org.grails.compiler.injection.GrailsASTUtils; +import org.grails.taglib.CompiledTagInvocation; +import org.grails.taglib.discovery.TagLibraryAstDiscovery; +import org.grails.taglib.index.TagLibraryIndex; + +/** + * Rewrites a call to a known tag into a direct invocation. + * + * <p>Writing {@code g.message(code: 'x')} reaches the tag library through {@code propertyMissing} to + * find the namespace and {@code invokeMethod} to find the tag, which is a dynamic call site even in a + * statically compiled class. Both the namespace and the tag name are fixed in the source, and the tag + * library index says whether that tag exists, so the call is replaced with a call to + * {@link CompiledTagInvocation}, an ordinary static method call. + * + * <p>The tag is still selected by name at runtime, through the same lookup the dynamic path uses, so + * a tag library registered later, one that overrides another, and the order tag libraries are + * registered in all decide the outcome exactly as they did before. Nothing is bound to a particular + * tag library class. + * + * <p>A namespace the index does not know is left alone, which is what keeps a tag library registered + * at runtime working, as is a name that something else in scope already answers to. + * + * @since 8.0.0 + */ +public class CompiledTagCallRewriter extends ClassCodeExpressionTransformer { + + private static final ClassNode INVOCATION_TYPE = ClassHelper.make(CompiledTagInvocation.class); + private static final String LOOKUP_ACCESSOR = "getTagLibraryLookup"; + private static final String OUTPUT_CONTEXT_ACCESSOR = "getOutputContext"; + private static final String INVOKE = "invoke"; + private static final String INVOKE_ARGUMENTS = "invokeArguments"; + private static final String INVOKE_ARGUMENTS_IN_CONTEXT = "invokeArgumentsInContext"; + private static final String GROOVY_PAGE_TYPE = "org.grails.gsp.GroovyPage"; + private static final String COMPILE_STATIC_TYPE = "groovy.transform.CompileStatic"; + private static final String GRAILS_COMPILE_STATIC_TYPE = "grails.compiler.GrailsCompileStatic"; + private static final String MARKUP_TAG_CALL = "invokeTag"; + private static final String DEFAULT_NAMESPACE = "g"; + + /** + * Names the dispatch treats as its own before it ever considers a tag, so an unqualified call to + * one of them is not a tag call however the index reads. + */ + private static final Set<String> RESERVED_NAMES = Set.of("body", "render"); + + private static final String REWRITTEN_MARKER = CompiledTagCallRewriter.class.getName(); + + private final SourceUnit sourceUnit; + private final TagLibraryIndex index; + private final ClassNode classNode; + private final String callerNamespace; + private final boolean page; + private final boolean rewritingPermitted; + private Set<String> localNames = Collections.emptySet(); + private Set<String> pageBindings = Collections.emptySet(); + private int rewritten; + + public CompiledTagCallRewriter(SourceUnit sourceUnit, TagLibraryIndex index, ClassNode classNode) { + this.sourceUnit = sourceUnit; + this.index = index; + this.classNode = classNode; + this.page = isGroovyPage(classNode); + // A page resolves a name against the model it was rendered with before it reaches a tag + // library, and that model is not visible here, so rewriting a page's tag call can only be + // sound where the page has given up dynamic resolution. Declaring compileStatic is that: it + // reserves the namespace names for tag libraries. A page that has not declared it keeps + // resolving its tags exactly as before. + this.rewritingPermitted = !this.page || isCompileStatic(classNode); + // An unqualified call is offered to the caller's own namespace before the default one, which is + // what a tag library declaring a namespace does at runtime. A page and a controller have no + // namespace of their own, so for them the two are the same. + String declared = this.page ? DEFAULT_NAMESPACE : TagLibraryAstDiscovery.resolveNamespace(classNode); + this.callerNamespace = declared != null ? declared : DEFAULT_NAMESPACE; + } + + /** + * @return how many calls were rewritten, for tests to assert against + */ + public int getRewrittenCount() { + return rewritten; + } + + public void rewrite() { + // A tag library is reached both as an artefact and as a class carrying the invoker trait, so + // rewriting can be asked for twice. Rewriting again would be harmless, but reporting a + // misspelled tag twice would not be. + if (classNode.getNodeMetaData(REWRITTEN_MARKER) != null) { + return; + } + classNode.putNodeMetaData(REWRITTEN_MARKER, Boolean.TRUE); + if (page) { + pageBindings = PageBindingCollector.collect(classNode); + } + for (MethodNode method : classNode.getMethods()) { + // getMethods() reaches inherited methods, whose bodies belong to the class that declared + // them. Rewriting one here would change a superclass through a subclass that happens to be + // able to call tags. Trait methods are woven as declarations on this class and so remain. + if (method.getDeclaringClass() != null && !classNode.equals(method.getDeclaringClass())) { + continue; + } + if (method.getCode() != null && !method.isAbstract()) { + rewriteBody(method.getCode(), method.getParameters()); + } + } + for (ConstructorNode constructor : classNode.getDeclaredConstructors()) { + if (constructor.getCode() != null) { + rewriteBody(constructor.getCode(), constructor.getParameters()); + } + } + for (FieldNode field : classNode.getFields()) { + if (field.getDeclaringClass() != null && !classNode.equals(field.getDeclaringClass())) { + continue; + } + Expression initial = field.getInitialExpression(); + if (initial != null) { + localNames = Collections.emptySet(); + field.setInitialValueExpression(transform(initial)); + } + } + for (Statement statement : classNode.getObjectInitializerStatements()) { + rewriteBody(statement, null); + } + } + + private void rewriteBody(Statement code, Parameter[] parameters) { + // An unqualified call reaches a tag only when nothing nearer answers to the name, and a local + // holding a closure answers to it. Which locals are in scope at a given point is not tracked + // here: a name declared anywhere in the body is treated as claimed throughout it, which can + // leave a call dispatched dynamically but never sends one to the wrong place. + localNames = LocalNameCollector.collect(code, parameters); + visitClassCodeContainer(code); + } + + @Override + protected SourceUnit getSourceUnit() { + return sourceUnit; + } + + @Override + public Expression transform(Expression expression) { + if (expression instanceof ClosureExpression closure) { + // ClassCodeExpressionTransformer deliberately does not descend into closures, and documents + // this override as the way to reach them. Without it a tag call written in a tag body, in a + // withFormat block, or in anything else taking a closure is never resolved - which is most + // of the tag calls in a real tag library. + closure.visit(this); + return closure; + } + if (expression instanceof MethodCallExpression call) { + Expression rewrite = rewriteTagCall(call); + if (rewrite != null) { + rewritten++; + return rewrite; + } + validateMarkupTagCall(call); + } + return super.transform(expression); + } + + private Expression rewriteTagCall(MethodCallExpression call) { + if (!(call.getMethod() instanceof ConstantExpression methodName) || + methodName.getValue() == null) { + return null; + } + String tagName = methodName.getValue().toString(); + String namespace = namespaceOf(call.getObjectExpression()); + if (namespace != null) { + // A namespace the build declared as filled in at runtime is left alone entirely: that + // declaration is how an application says its tags are decided while it runs, whether by a + // tag library registered then or by metaprogramming, and binding a call now would settle + // what it asked to keep open. + if (index.isDynamicNamespace(namespace)) { + return null; + } + if (!index.hasNamespace(namespace) || isShadowed(call.getObjectExpression(), namespace) || + pageBindings.contains(namespace)) { + return null; + } + if (!index.isKnown(namespace, tagName)) { + // Only where the name means a tag library for certain. In a page that has not given up + // dynamic resolution the receiver may just as well be the model it was rendered with, + // and reporting there would reject a call this release deliberately still allows. + if (this.rewritingPermitted) { + reportUnknownTag(namespace, tagName, call); + } + return null; + } + } + else { + namespace = unqualifiedNamespaceOf(call, tagName); + if (namespace == null) { + return null; + } + } + if (!this.rewritingPermitted) { + return null; + } + Expression invocation = invocation(namespace, tagName, call.getArguments()); + if (invocation != null) { + // Kept where the tag was written, so a stack trace and any later diagnostic still point at + // the line the author wrote rather than at the start of the file. + setSourcePosition(invocation, call); + } + return invocation; + } + + /** + * The namespace an unqualified call such as {@code message(code: 'x')} resolves in. + * + * <p>Only a name nothing else answers to reaches a tag at all: a real method of the class, an + * inherited one, a field, a property or a local wins, and whether such a member exists is what + * decides the call. Where nothing claims the name, dispatch offers it to the caller's own + * namespace and then to the default one, which is the order reproduced here. + * + * @return the namespace to invoke in, or {@code null} when the call is not resolvably a tag + */ + private String unqualifiedNamespaceOf(MethodCallExpression call, String tagName) { + if (page) { + // A page resolves an unqualified name against its binding before it reaches a tag, and what + // a page's binding holds - the model it was rendered with - is not visible here. A call + // written with its namespace says which tag library it means and is rewritten; one without + // is left to resolve as it did. + return null; + } + if (!call.isImplicitThis() || RESERVED_NAMES.contains(tagName)) { + return null; + } + if (declaresMember(tagName) || localNames.contains(tagName)) { + return null; + } + if (index.isDynamicNamespace(callerNamespace) || index.isDynamicNamespace(DEFAULT_NAMESPACE)) { + // The namespaces an unqualified call could reach were declared as decided at runtime. + return null; + } + if (index.isKnown(callerNamespace, tagName)) { + return callerNamespace; + } + if (index.isKnown(DEFAULT_NAMESPACE, tagName)) { + return DEFAULT_NAMESPACE; + } + // Not a tag this build knows about. It is not reported: an unqualified name in a controller is + // as likely to be a dynamic finder, an injected service method or anything else contributed at + // runtime as it is a misspelled tag. + return null; + } + + /** + * Builds the invocation, passing the attributes and body directly where the source says what they + * are and forwarding the arguments as written where it does not. + */ + private Expression invocation(String namespace, String tagName, Expression arguments) { + if (!(arguments instanceof TupleExpression tuple)) { + return null; + } + ArgumentListExpression invocationArgs = new ArgumentListExpression(); + invocationArgs.addExpression(new MethodCallExpression(VariableExpression.THIS_EXPRESSION, Review Comment: `VariableExpression.THIS_EXPRESSION` (here and at `outputContext()`) and `ConstantExpression.NULL` (in `attributesAndBody`) are process-wide singletons, and this reuses them as generated nodes at every rewritten call site in every class in the compilation. `StaticTypeCheckingVisitor.storeType` writes `INFERRED_TYPE` node metadata onto the expression it visits, so a `@CompileStatic` page or tag library will stamp inferred types onto the shared instances and the last write wins. I didn't manage to make it misbehave in a quick test, but it's a latent hazard rather than a safe idiom — note that `TagLibraryTransformer`, doing the same job a few files over, deliberately uses `new VariableExpression("this")` for exactly this reason. Suggest `new VariableExpression("this")` and `new ConstantExpression(null)` per call site. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/TagLibraryIndexFiles.groovy: ########## @@ -0,0 +1,84 @@ +/* + * 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.gradle.plugin.views.gsp + +import java.nio.charset.StandardCharsets + +import groovy.transform.CompileStatic + +/** + * The files the tag library index is made of, as the build writes them. + * + * <p>Written here rather than by the forked generator because they say what the build asked for + * rather than what the sources declare, and because they have to be written even for a project with + * no tag libraries of its own. + * + * @since 8.0.0 + */ +@CompileStatic +final class TagLibraryIndexFiles { + + /** Review Comment: These constants restate the on-disk format that `org.grails.taglib.index.TagLibraryIndex` owns, as string literals, in a different module, with `// matching TagLibraryIndex.X` comments to hold them together. Two of the three already don't match: - here: `INDEX_LOCATION = 'META-INF/grails/taglibs'` - there: `INDEX_LOCATION = "META-INF/grails/taglibs/"` (trailing slash) `writeSettings` also hard-codes the `dynamicTagNamespaces` and `strictTags` keys, which exist as `TagLibraryIndex.DYNAMIC_NAMESPACES_KEY` / `STRICT_KEY` (package-private). It works today only because `new File(dir, path)` is indifferent to the trailing slash — which is precisely the kind of coincidence that stops holding. Since `GenerateTagLibraryIndexTask` already forks against the project's compile classpath, the settings file could be written by the generator too, or these keys promoted to public constants and depended on. If the duplication has to stay, please add a test that asserts the two sets of constants agree, so a rename can't silently split them. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/TagMethodInvoker.java: ########## @@ -36,36 +36,25 @@ import groovy.lang.GroovyObject; import groovy.lang.MissingMethodException; -import grails.gsp.NotATag; -import grails.gsp.Tag; +import org.grails.taglib.discovery.ReflectedTagMethodView; +import org.grails.taglib.discovery.TagDiscoveryRules; public final class TagMethodInvoker { /** * Method names from framework traits, Spring lifecycle interfaces, and the like * that must never be treated as tag methods regardless of the declaring class. */ - private static final Set<String> FRAMEWORK_METHOD_NAMES = Set.of( - "afterPropertiesSet", - "currentRequestAttributes", - "destroy", - "initializeTagLibrary", - "onApplicationEvent", - "raw", - "throwTagError", - "withCodec" - ); - - private static final Set<String> OBJECT_METHOD_SIGNATURES = collectSignatures(Object.class); - private static final Set<String> GROOVY_OBJECT_METHOD_SIGNATURES = collectSignatures(GroovyObject.class); - - private static Set<String> collectSignatures(Class<?> type) { - Set<String> signatures = new HashSet<>(); - for (Method method : type.getMethods()) { - signatures.add(signature(method)); - } - return Collections.unmodifiableSet(signatures); - } + /** Review Comment: The javadoc that used to document `FRAMEWORK_METHOD_NAMES` was left in place above the replacement, so there are now two consecutive doc comments on one field: ```java /** * Method names from framework traits, Spring lifecycle interfaces, and the like * that must never be treated as tag methods regardless of the declaring class. */ /** * Names that live on every tag library through the framework traits ... */ public static final Set<String> FRAMEWORK_METHOD_NAMES = ... ``` The first one should go. (Same pattern on `TagLibraryAstDiscovery.findTags`.) While here: this field is now `public` and exposes `TagDiscoveryRules.getFrameworkMethodNames()` directly. That returns the `Set.of(...)` instance so it is immutable in practice, but since it's now public API it's worth wrapping in `Collections.unmodifiableSet` at the source, or documenting the immutability guarantee on `getFrameworkMethodNames()`. ########## grails-gsp/plugin/src/test/groovy/org/grails/web/taglib/TagDispatchBenchmarkSpec.groovy: ########## @@ -0,0 +1,241 @@ +/* + * 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 java.nio.file.Files +import java.nio.file.Path + +import grails.testing.web.taglib.TagLibUnitTest +import groovy.text.Template +import org.grails.gsp.GroovyPagesTemplateEngine +import org.grails.taglib.TagLibraryLookup +import org.grails.taglib.index.TagLibraryIndex +import org.grails.plugins.web.taglib.ApplicationTagLib +import spock.lang.Requires +import spock.lang.Shared +import spock.lang.Specification + +/** + * What compiling a tag call into an invocation is worth, separately from removing the metaclass work + * that used to surround every call. + * + * <p>Both are measured against the same framework, so the metaclass writes are already gone from both + * sides. What varies is only whether a call was compiled into an invocation, which is what a build can + * still turn off per namespace. That isolates the part of the change whose value was never measured + * on its own. + * + * <p>Off unless asked for, since a timing run is neither quick nor a pass/fail assertion: + * + * <pre> + * GRAILS_TAGLIB_BENCH=true ./gradlew :grails-gsp:test \ + * --tests '*TagDispatchBenchmarkSpec' --rerun-tasks -i + * </pre> + * + * <p>Gated on the environment rather than a system property because a forked test process inherits + * the environment, where this build bridges only a few named properties into it. + */ +@Requires({ System.getenv('GRAILS_TAGLIB_BENCH') }) +class TagDispatchBenchmarkSpec extends Specification implements TagLibUnitTest<ApplicationTagLib> { Review Comment: 241 lines of hand-rolled timing harness in `src/test`, `@Requires`-gated on an environment variable so it never runs and nothing keeps it compiling-and-correct beyond `compileTestGroovy`. This repo already has JMH benchmark infrastructure. A hand-rolled loop with a warmup count and a median can't control for JIT state, GC, or dead-code elimination the way JMH's blackholes and forks do — and the spec's own comment ("reported rather than asserted: a timing is evidence, not a contract") concedes that it produces no verdict. Suggest moving it to the JMH module, where the numbers in the PR description could then be reproduced by anyone, or dropping it and keeping the measurements in the description. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/discovery/TagDiscoveryRules.java: ########## @@ -0,0 +1,159 @@ +/* + * 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.taglib.discovery; + +import java.util.Set; + +/** + * Decides whether a method is a tag. + * + * <p>The single statement of those rules. Both the reflective discovery an application performs at + * startup and the syntax-tree discovery a build performs while compiling a tag library route through + * here, so the two cannot drift apart: a method is a tag for a compiler exactly when it is a tag for + * the runtime. + * + * <p>The rules, in order: + * <ol> + * <li>plumbing — non-public, static, or compiler-generated methods are never tags;</li> + * <li>{@code @NotATag} excludes, {@code @Tag} includes, each overriding everything below;</li> + * <li>names belonging to Object, Groovy, or the framework traits are never tags;</li> + * <li>property accessors are never tags;</li> + * <li>what remains is a tag if it can be called as {@code (attrs)} or {@code (attrs, body)}.</li> + * </ol> + * + * @since 8.0.0 + */ +public final class TagDiscoveryRules { + + /** + * The name a {@link java.util.Map} parameter must carry to be the attributes parameter, when the + * method retains parameter names. + */ + public static final String ATTRS_PARAMETER_NAME = "attrs"; + + /** + * The name a {@link groovy.lang.Closure} parameter must carry to be the body parameter, when the + * method retains parameter names. + */ + public static final String BODY_PARAMETER_NAME = "body"; + + /** + * Names that are Groovy or Object plumbing on any class. + */ + private static final Set<String> LANGUAGE_METHOD_NAMES = Set.of( Review Comment: Worth calling out explicitly in the description: extracting these rules also **changes runtime tag discovery**, which is what `DefaultGrailsTagLibClass` and `TagMethodInvoker.INVOKABLE_METHODS_BY_NAME` are built from. Three differences from the code this replaces: 1. `equals` / `hashCode` / `toString` / `getProperty` / `setProperty` / `getMetaClass` / `setMetaClass` are now excluded **by name**, where before `Object` and `GroovyObject` members were excluded by full *signature*. A tag called `equals(Map attrs)` was previously discoverable and no longer is. 2. `isPropertyAccessor` drops the boolean-return-type check on `is*`, so any zero-arg `is*` is now an accessor. 3. Names containing `$` are newly excluded. I worked through all three and couldn't construct a case that behaves differently in practice — (1) and (2) are unreachable because the shape check rejects them anyway, and (3) only removes synthetic trait accessors that were never tags. So I think this is safe. But it is a change to runtime behaviour riding along in a compile-time feature, and reviewers shouldn't have to derive that themselves. Please note it in the description, and add a spec pinning the rules directly (`TagDiscoveryRulesSpec` covers the AST view — a reflective equivalent asserting the same verdicts for the same shapes would make the "the two cannot drift apart" claim in the class javadoc actually enforced). ########## grails-doc/src/en/guide/theWebLayer/gsp/taglibs/compiledTags.adoc: ########## @@ -0,0 +1,237 @@ +//// +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. +//// + +Tag libraries are described when they are compiled, and that description is used to resolve tag calls +in pages, tag libraries and controllers compiled afterwards. + +==== Defining tags as methods + +Define a tag as a method: + +[source,groovy] +---- +class GreetingTagLib { + + static namespace = 'greet' + + def hello(Map attrs) { + out << "Hello ${attrs.name}" + } + + def wrapped(Map attrs, Closure body) { + out << '<div>' << body() << '</div>' + } +} +---- + +The attributes parameter must be a `Map` named `attrs`, and the body parameter a `Closure` named +`body`. A method taking anything else is an ordinary method of the tag library rather than a tag. +Where that convention does not suit, `@Tag` marks a method as a tag whatever its signature and +`@NotATag` excludes one that would otherwise match. + +The older form, a `Closure` field, still works: + +[source,groovy] +---- +// Deprecated +Closure hello = { Map attrs -> + out << "Hello ${attrs.name}" +} +---- + +A tag declared this way is described and called like any other — the tag is selected by name when the +call runs, and a closure answers to a name as readily as a method does. The form remains deprecated +because a closure carries no signature, so nothing about the call can be checked. Compiling a tag +library that declares one produces a warning naming the tag. + +==== Calling tags + +In a tag library or a controller, a call to a tag whose namespace and name are known is compiled into +a direct invocation rather than being dispatched through the metaclass: + +[source,groovy] +---- +class BookController { + def index() { + String markup = g.createLink(controller: 'book') // compiled into a direct invocation + String other = greet.hello(name: 'Grails') // likewise + } +} +---- + +The attributes and body are passed straight through where the call says what they are. Where it does +not — attributes assembled at runtime, or a single value the tag reads under its own name — the +arguments are forwarded as written and sorted out by the same rules dynamic dispatch applies: + +[source,groovy] +---- +Map attrs = buildAttributes() +g.createLink(attrs) // still compiled into an invocation +---- + +The tag is always selected by name when the call runs, through the same lookup dynamic dispatch uses. +A tag library that overrides another, one registered while the application is running, and the order +tag libraries are registered in all decide the outcome exactly as they did before. Nothing is bound to +a particular tag library class, so a tag declared by more than one of them is compiled the same way. + +A call into a namespace no compiled tag library declares is left alone, which is what allows a tag +library registered while an application is running to keep working. + +A name that something else in scope already answers to is not a namespace. A local variable, a +parameter or a property called `g` is that thing, and a call on it is left alone: + +[source,groovy] +---- +def index() { + def g = someClient + g.createLink(controller: 'book') // someClient.createLink, not the tag +} +---- + +A call written without a namespace follows the same rule. It reaches a tag only when nothing nearer +answers to the name — not a method of the class, not one it inherits, not a field, property or local — +and is then offered to the calling tag library's own namespace before the default one, which is the +order dispatch uses at runtime: + +[source,groovy] +---- +class BookController { + def index() { + String markup = createLink(controller: 'book') // compiled into an invocation + } +} +---- + +Tags called from within a closure — a tag body, a `withFormat` block, anything taking a block — are +compiled the same way as tags called directly. + +==== Tags in pages + +A page resolves a name against the model it was rendered with before it reaches a tag library, and +that model is not known when the page is compiled. A page therefore keeps resolving its tags as it +always has, unless it declares `compileStatic`: + +[source,html] +---- +<%@ page compileStatic="true" %> +${g.createLink(controller: 'book')} <%-- compiled into a direct invocation --%> +---- + +Declaring `compileStatic` on a page reserves the namespace names for tag libraries: a model attribute +called `g` no longer shadows the `g` namespace there. Without it, an expression is dispatched exactly +as before. Set `grails.views.gsp.compileStatic` in configuration to apply it to every page. + +Two things hold in a page either way. A call written without a namespace, as +`${createLink(controller: 'book')}` is, is always left to resolve against the binding. And a name the +page puts into its own binding is that variable rather than a namespace: + +[source,html] +---- +<g:set var="g" value="${someObject}"/> +${g.createLink(controller: 'book')} <%-- someObject.createLink, not the tag --%> +---- + +A tag written as markup, as `<g:createLink controller="book"/>` is, already compiles into a direct +call naming the tag and needs no rewriting. It is unambiguously a tag whatever the page does, so under +strict checking it is checked in every page. An expression is checked only where it is resolved, in a +page declaring `compileStatic`, since elsewhere the receiver may be part of the model. + +==== Reporting unknown tags + +By default nothing is reported: a tag that no compiled tag library declares is left to resolve at +runtime, exactly as it did before. A namespace can hold tag libraries that were not compiled with a +description — a plugin built against an earlier version of Grails contributes tags to `g` without one, +and a tag library registered at runtime contributes more — so a tag missing from the description is +not necessarily a misspelling, and reporting one by default would mean complaining about correct code. + +An application whose tag libraries are all described can ask for an error instead: + +[source,groovy] +.build.gradle +---- +grails { + compileStatic { + strictTags = true + dynamicTagNamespaces = ['legacy'] // <1> + } +} +---- +<1> namespaces genuinely filled in while the application runs + +Strict checking applies where the source says a call is a tag: one naming its namespace, as +`g.message(code: 'x')` does, and one written as markup, as `<g:message/>` is. A call written without a +namespace is never checked — such a name may equally be a method contributed while the application +runs, and in a page it may come from the model. A namespaced expression in a page is checked only when +that page declares `compileStatic`, for the same reason its calls are only rewritten there. + +`dynamicTagNamespaces` names the namespaces whose tags are decided while the application runs rather +than described when it is compiled. It turns compile-time resolution off for them completely: a call +into such a namespace is never rewritten, never reported, and is dispatched exactly as it was before +this release, whether or not a compiled tag library also declares the namespace. Declare a namespace +here when a tag library is registered at runtime, or when the tags in it are contributed by +metaprogramming. + +Both settings are read from the build, not from a system property, so changing either recompiles what +depends on it. + +==== Where the description lives + +Each tag library contributes one file under `META-INF/grails/taglibs` in the artifact it is packaged +in. Descriptions from every jar on the classpath are combined, so a plugin's tag libraries are +resolvable by an application that depends on it without any extra build configuration. + +Under the Grails Gradle plugin the description is written twice, because the two things that read it +need different guarantees. + +`generateTagLibraryIndex` runs before compilation and reads the sources under `grails-app/taglib`, so +tags an application declares are resolvable in the same compilation that defines them. Being read from +source it cannot describe everything: a tag library referring to a type written in Java, or generated +by the build, is left out. What it missed is recorded, and nothing in a namespace it could not fully +describe is ever reported as an unknown tag, so strict checking cannot fail a build over a tag that +does exist. This index is used only to compile this project, and is never packaged. + +`packageTagLibraryIndex` runs after compilation, with the project's own classes on its classpath, +where every tag library resolves whatever language its collaborators were written in. That index is +the authoritative one: pages are compiled against it, it travels with the artifact, and a project +depending on this one reads it. Every run replaces the directory, so a tag library that is renamed or +deleted disappears from it. + +A project keeping tag libraries elsewhere adds those directories once, by task type, so that both +indexes describe the same set — configuring them separately would let the index this project compiles +against differ from the one it publishes: + +[source,groovy] +.build.gradle +---- +import org.grails.gradle.plugin.views.gsp.GenerateTagLibraryIndexTask + +tasks.withType(GenerateTagLibraryIndexTask).configureEach { + sourceDirectories.from(file('src/main/groovy')) +} +---- + +Before compilation, a tag library referring to something the same project declares — a service it +injects, a base class it extends, a trait it carries — is described too, by reading that Groovy source +alongside it, so an inherited namespace, a tag a trait contributes and an attributes parameter of a +project-declared type are all read rather than guessed. A tag library naming a type that does not +exist is left out, as it would be by the compiler. + +Where no build writes the index — a plain Groovy compilation, or one that does not apply the Grails +Gradle plugin — each tag library describes itself as it is compiled instead, which makes it resolvable Review Comment: This claim doesn't hold for the normal case. The self-describing path is `TagLibArtefactTypeAstTransformation`, a local AST transform bound to `@TagLib`. A conventional `grails-app/taglib/FooTagLib.groovy` has no annotation in source — `@Artefact` is added by `GlobalGrailsClassInjectorTransformation` at `CANONICALIZATION`, after local transforms were collected — so nothing describes it. I verified this: compiling an unannotated tag library emits no descriptor; adding `@grails.gsp.TagLib` to the same class emits one. So "a plain Groovy compilation, or one that does not apply the Grails Gradle plugin" describes only tag libraries that were annotated by hand. Please either make the fallback cover convention-based tag libraries or reword this paragraph to say what it actually does. ########## grails-gsp/plugin/src/ast/groovy/grails/compiler/traits/CompiledTagCallTransformation.groovy: ########## @@ -0,0 +1,98 @@ +/* + * 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 grails.compiler.traits + +import groovy.transform.CompileStatic +import org.codehaus.groovy.ast.ASTNode +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.ModuleNode +import org.codehaus.groovy.control.CompilePhase +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.transform.ASTTransformation +import org.codehaus.groovy.transform.GroovyASTTransformation + +import grails.artefact.gsp.TagLibraryInvoker +import grails.gsp.taglib.compiler.CompiledTagCallRewriter +import org.grails.taglib.index.TagLibraryIndex + +/** + * Compiles a call to a known tag into a direct invocation, wherever tags can be called from. + * + * <p>A tag library rewrites its own calls as it is compiled, but a controller can call tags too, and + * gains that ability from the {@link TagLibraryInvoker} trait rather than from being a tag library. + * Any class carrying that trait is therefore a candidate, which covers controllers without naming + * them and without a second copy of the rewriting rules. A compiled GSP calls tags as well, and + * reaches them through {@code GroovyPage} rather than through the trait, so it is matched separately. + * + * <p>Runs after trait injection, since whether a class can call tags is only settled once its traits + * have been applied. + * + * @since 8.0.0 + */ +@CompileStatic +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +class CompiledTagCallTransformation implements ASTTransformation { Review Comment: **Blocking — `@Artefact('Controller')` classes outside `grails-app/controllers` are never rewritten.** Reproduced on this branch: ```groovy // src/main/groovy/pkg/AnnotatedController.groovy @Artefact('Controller') class AnnotatedController { def index() { g.createLink(controller: 'book') } } ``` `javap` shows `implements grails.artefact.gsp.TagLibraryInvoker` — the trait is there — but the class file contains no `CompiledTagInvocation` reference. The same class under `grails-app/controllers` **is** rewritten. The reason is transform ordering. In `ASTTransformationVisitor.addPhaseOperations`, global transforms are registered as phase operations before the local-transform visitor, so every global at `CANONICALIZATION` runs before every local one. The convention path works because `GlobalGrailsClassInjectorTransformation` is itself global and has a high `TransformWithPriority`, whereas this transform doesn't implement `TransformWithPriority` at all and so defaults to `0` — last among globals. That is accidentally correct for the convention path and unavoidably wrong for locally-annotated artefacts, where trait injection happens in `ArtefactTypeAstTransformation` after all globals have finished. So the class javadoc > Runs after trait injection, since whether a class can call tags is only settled once its traits have been applied. isn't guaranteed — it holds for one of the two ways to declare a controller. Two things to fix: - implement `TransformWithPriority` and add an entry to `org.apache.grails.common.compiler.GroovyTransformOrder`, as every other Grails global transform does. Relying on an undeclared default of `0` is exactly the fragility that registry exists to prevent. - decide what to do about `@Artefact`-annotated artefacts. Either hook the rewrite into `ArtefactTypeAstTransformation` the way `TagLibArtefactTypeAstTransformation` already does for tag libraries, or document that the fast path is convention-only. Either way the behaviour is currently a silent, undetectable difference between two source layouts that are meant to be equivalent. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndex.java: ########## @@ -0,0 +1,485 @@ +/* + * 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.taglib.index; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.WeakHashMap; + +/** + * The set of tag libraries and tag names known at compile time. + * + * <p>Each tag library contributes one descriptor under {@value #INDEX_LOCATION}, written by the + * {@code TagLib} AST transformation as the tag library is compiled. Descriptors are per class rather + * than per module so that libraries packaged in separate jars merge on the classpath without any + * build step having to combine them, in the same way {@code META-INF/services} entries do. + * + * <p>Reading the index answers "which tags exist in namespace x" without loading or reflecting over a + * single tag library class, which is what allows GSP expressions to be resolved when a page is + * compiled rather than dispatched dynamically when it renders. + * + * @since 8.0.0 + */ +public final class TagLibraryIndex { + + /** + * Classpath directory holding one descriptor per compiled tag library. + */ + public static final String INDEX_LOCATION = "META-INF/grails/taglibs/"; + + /** + * Descriptor format this build writes and understands. A descriptor carrying anything else was + * produced by a different version of Grails and is ignored, so its tags resolve dynamically rather + * than being read under the wrong set of rules. + */ + public static final int FORMAT_VERSION = 2; + + /** + * Settings the build states for the compilation the index is read in, written alongside the + * descriptors by the build and deliberately not packaged into the artifact: they describe how this + * project is compiled, not what its tag libraries declare. + */ + public static final String SETTINGS_LOCATION = INDEX_LOCATION + "compile-settings.properties"; + + /** + * What the index could not describe, written by whatever produced it. + * + * <p>An index generated before its project is compiled cannot always read every tag library: one + * referring to a type that does not exist yet, in a language it cannot parse, or generated by the + * build itself, is left out. A namespace missing some of its tags must not have a call to one of + * them reported as a misspelling, so what was missed is recorded rather than left to be inferred + * from the absence. + */ + public static final String INCOMPLETE_LOCATION = INDEX_LOCATION + "incomplete.properties"; Review Comment: `incomplete.properties` records only what the *local* generator skipped, so it can't express the case that actually bites: a namespace that some jars describe and others don't. Grails' own tag libraries are annotated and therefore self-describe, so in any real application `hasNamespace("g")` is true. Every plugin contributing to `g` without descriptors — every pre-8 plugin, and (see the `TagLibArtefactTypeAstTransformation` comment) most convention-based ones — then looks to `reportUnknownTag` like a set of misspellings. With `strictTags = true` that fails the build over correct code. The only escape is `dynamicTagNamespaces = ['g']`, which switches the whole feature off for the namespace that matters most, so `strictTags` is effectively unusable for any application with third-party tag libraries. Worth considering: record in each descriptor which *project* described a namespace, and treat a namespace as complete only when every tag library the runtime will register for it has a descriptor — or scope strictness to namespaces the compiling project itself declares, where completeness is actually knowable. At minimum, the `strictTags` documentation should say plainly that a single undescribed plugin in a namespace makes it unusable. ########## grails-gsp/grails-web-taglib/src/main/groovy/grails/gsp/taglib/compiler/TagLibArtefactTypeAstTransformation.java: ########## @@ -45,9 +52,79 @@ public class TagLibArtefactTypeAstTransformation extends ArtefactTypeAstTransfor @Override protected String resolveArtefactType(SourceUnit sourceUnit, AnnotationNode annotationNode, ClassNode classNode) { addClosureTagDeprecationWarnings(sourceUnit, classNode); + writeIndexEntry(sourceUnit, classNode); + rewriteResolvedTagCalls(sourceUnit, classNode); return "TagLibrary"; } + /** + * Records the namespace and tag names this tag library declares, so that a GSP compiled later can + * resolve a tag call without loading the tag library or consulting its metaclass. + * + * <p>Failure to write is never fatal: the index is an optimisation, and a missing descriptor + * degrades to the runtime resolution that applies when a tag library is registered dynamically. + */ + protected void writeIndexEntry(SourceUnit sourceUnit, ClassNode classNode) { Review Comment: **Blocking — this never runs for conventional tag libraries.** `TagLibArtefactTypeAstTransformation` is a *local* transform, bound to `@TagLib` via `@GroovyASTTransformationClass` on `grails.gsp.TagLib`. A conventional `grails-app/taglib/FooTagLib.groovy` carries no annotation in source: `GlobalGrailsClassInjectorTransformation` adds `@Artefact('TagLib')` during `CANONICALIZATION`, long after `ASTTransformationCollectorCodeVisitor` ran at `SEMANTIC_ANALYSIS`, so this transform is never collected and `writeIndexEntry` is never called. Reproduced. Compiling `grails-app/taglib/pkg/DgmTagLib.groovy` (no annotation) with a plain `CompilationUnit` and a target directory emits **no** `META-INF/grails/taglibs/` output. Adding `@grails.gsp.TagLib` to the same class emits the descriptor. Grails' own tag libraries all carry `@TagLib`, which is why the framework's own build looks like it works. Consequences: - Every plugin that declares tag libraries by convention and doesn't apply the Grails GSP Gradle plugin publishes no descriptors — `grails-fields`, `grails-cache`, `grails-redis` and `grails-spring-security` in this repo are all in that position. - `buildOwnsIndex()` is designed around a fallback that mostly doesn't exist. - The claim in `compiledTags.adoc` that a tag library "describes itself as it is compiled" when no build writes the index is wrong for the normal case (separate comment there). It degrades safely — a missing descriptor just means dynamic dispatch — so this isn't a correctness bug. But the design and the docs both assume a fallback that only covers annotated tag libraries, and that assumption should either be fixed (have `GlobalGrailsClassInjectorTransformation` drive the index write for artefacts it recognises) or stated plainly. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndexEntry.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.taglib.index; + +/** + * One tag recorded in the {@link TagLibraryIndex} at compile time. + * + * @param namespace the tag library namespace the tag is reachable through + * @param tagName the tag name within that namespace + * @param tagLibraryClassName the binary name of the tag library declaring the tag + * @param kind how the tag is implemented, which decides whether a call to it can be resolved + * @param acceptsBody whether the tag can be called with a body + * @since 8.0.0 + */ +public record TagLibraryIndexEntry(String namespace, String tagName, String tagLibraryClassName, Review Comment: `Kind` is written into every descriptor as `name:KIND`, is the reason `TagLibraryIndex.FORMAT_VERSION` exists, and is parsed back out in `TagLibraryIndex.load` — but nothing ever consults it. `CompiledTagCallRewriter` decides purely on `index.isKnown(...)`, so a `LEGACY_CLOSURE` tag is rewritten into `CompiledTagInvocation.invoke` exactly like a `METHOD` one. `isMethod()` has no callers. That contradicts the rationale given for the deprecation — "a closure carries no signature, so nothing about the call can be checked" — since in practice a closure tag call is bound identically to a method tag call. Either use it (skip rewriting or skip strict checking for `LEGACY_CLOSURE`, whichever was intended) or drop the encoding and the version bump until there's a consumer. Same applies to the other currently-unused index API: `lookup`, `getAmbiguousTagNames`, `getTagNamesForClass`, `isClassDescribed`, `getIncompleteNamespaces`, and `TagLibraryAstDiscovery.findTagNames`. Related: `FORMAT_VERSION = 2` for a format that ships for the first time in this PR — worth resetting to 1 so the number means something later. ########## grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java: ########## @@ -296,6 +310,23 @@ public Object getProperty(String property) { return resolveProperty(property); } + /** + * Resolves a tag called without a namespace, as {@code ${message(code: 'x')}} is. + * + * <p>A real method rather than one installed onto this page's metaclass. Installing it, along with + * a method for every tag and a property for every namespace, meant writing to an + * ExpandoMetaClass for every page compiled and made every later tag call a read of an initialised + * metaclass, which is guarded by a lock. + * + * @param name the tag name + * @param args the arguments the tag was called with + * @return whatever the tag produces + */ + public Object methodMissing(String name, Object args) { + return TagLibraryMetaUtils.methodMissingForTagLib(getMetaClass(), getClass(), gspTagLibraryLookup, Review Comment: `gspTagLibraryLookup` can be null here and `methodMissingForTagLib` dereferences it immediately (`gspTagLibraryLookup.lookupTagLibrary(...)`), so this turns a missing method into an NPE. The code being replaced guarded exactly this: `GroovyPagesMetaUtils.registerMethodMissingForGSP` opened with `if (gspTagLibraryLookup == null) return`, so with no lookup the page simply had no `methodMissing` and an unresolved call produced the expected `MissingMethodException`. Now `methodMissing` always exists. `CompiledTagInvocation.invoke` gets this right: ```java if (lookup == null) { throw new GrailsTagException("Tag [" + tagName + "] cannot be invoked without a tag library lookup"); } ``` Suggest the same shape here, or an early `throw new MissingMethodException(name, getClass(), args)` when the lookup is null so the previous behaviour is preserved exactly. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePlugin.groovy: ########## @@ -64,20 +119,114 @@ class GroovyPagePlugin implements Plugin<Project> { Provider<Directory> webappDestDir = project.layout.buildDirectory.dir('gsp-classes/webapp') output?.dir('gsp-classes') + // The Java the rest of the project is built with, so that pages are built with it too. + // Absent a toolchain this resolves to the JVM running Gradle, which is what compiling + // pages fell back to before and remains the right answer when nothing else was asked for. + JavaPluginExtension javaExtension = project.extensions.getByType(JavaPluginExtension) + JavaToolchainService toolchains = project.extensions.getByType(JavaToolchainService) + Provider<JavaLauncher> launcher = toolchains.launcherFor(javaExtension.toolchain) + + // The index is written twice, because the two things that read it need different guarantees. + // + // This one exists before this project is compiled, so that a call to a tag the project itself + // declares can be resolved as it compiles. It is read from source, so it cannot describe + // everything: a tag library referring to a type written in another language, or generated by + // the build, is left out, and what was missed is recorded so that nothing in an incompletely + // described namespace is reported as a misspelling. It is never packaged - a consumer must not + // be given a partial description - and pages are not compiled against it either. + // Everything both indexes must agree on is configured once, by type. Configuring the two + // tasks separately would let them describe different sets of tag libraries, and the one that + // is published is not the one this project compiles against - so they would diverge silently. + // A project keeping tag libraries elsewhere adds them the same way. + tasks.withType(GenerateTagLibraryIndexTask).configureEach { GenerateTagLibraryIndexTask index -> + index.sourceDirectories.from(project.layout.projectDirectory.dir('grails-app/taglib')) + index.parameterNamesRetained.set(resolvePreserveParameterNames(project)) Review Comment: All four of these resolvers (`resolvePreserveParameterNames`, `resolveStrictTags`, `resolveDynamicTagNamespaces`, `resolveGroovySourceRoots`) build a `project.provider { ... project.extensions ... }` that captures `Project` and reads it when queried, and the results are `set()` into task `@Input` properties. That's the classic configuration-cache hazard: whether it works depends on the value being resolved during CC store rather than at execution. `TagLibraryIndexWiringFunctionalSpec` and `GenerateTagLibraryIndexTaskSpec` never run with `--configuration-cache`, so nothing here would catch a regression. Suggest reading the extension eagerly into a `Provider` that doesn't capture `Project` (e.g. `objects.property(...).convention(extension.strictTags)`), and adding `--configuration-cache` to at least one of the functional specs. ########## grails-gsp/grails-web-taglib/src/main/groovy/grails/artefact/gsp/TagLibraryInvoker.groovy: ########## @@ -93,11 +93,14 @@ trait TagLibraryInvoker extends WebAttributes { } if (tagLibrary) { - if (!developmentMode) { - MetaClass thisMc = GrailsMetaClassUtils.getMetaClass(this) - TagLibraryMetaUtils.registerMethodMissingForTags(thisMc, lookup, usedNamespace, methodName) - } - return tagLibrary.invokeMethod(methodName, args) + // Resolving the tag used to install it onto this object's metaclass so that later + // calls bypassed methodMissing. That made every caller mutate its own + // ExpandoMetaClass the first time it used a tag, and made every later call pay the + // read lock guarding an initialised metaclass. The tag is dispatched through the + // lookup each time instead, which is a map read. + return TagLibraryMetaUtils.methodMissingForTagLib( Review Comment: This changes dispatch semantics, not just where the metaclass write happened. The old line was `return tagLibrary.invokeMethod(methodName, args)` — a direct invocation on the tag library bean. For a **closure-defined** tag that lands on the `TagLibraryTransformer`-generated wrapper and ends in `captureTagOutput`, so the two are equivalent. For a **method-defined** tag (`def foo(Map attrs)`) no wrapper exists, so the old path called `foo(Map)` straight on the bean and returned its own return value, with no output capture and no codec handling. The new path routes through `methodMissingForTagLib` → `TagOutput.captureTagOutput`, which returns the captured, encoded buffer subject to `doesTagReturnObject`. It also loses `methodMissingForTagLib`'s second branch: when the name isn't a closure property or a conventional tag method, that method falls back to `tagBeanMc.respondsTo(tagBean, name, args)` and invokes whatever matches. `invokeMethod` reached those; the rewritten call reaches them only if the shape happens to match. The new behaviour is arguably the correct one, but it's a behaviour change to a public trait that isn't in the description, isn't in `upgrading80x.adoc`, and has no test. Could you add a spec covering a method-defined tag invoked unqualified through `methodMissing` from a controller, asserting the return value and the codec applied? ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePlugin.groovy: ########## @@ -64,20 +119,114 @@ class GroovyPagePlugin implements Plugin<Project> { Provider<Directory> webappDestDir = project.layout.buildDirectory.dir('gsp-classes/webapp') output?.dir('gsp-classes') + // The Java the rest of the project is built with, so that pages are built with it too. + // Absent a toolchain this resolves to the JVM running Gradle, which is what compiling + // pages fell back to before and remains the right answer when nothing else was asked for. + JavaPluginExtension javaExtension = project.extensions.getByType(JavaPluginExtension) + JavaToolchainService toolchains = project.extensions.getByType(JavaToolchainService) + Provider<JavaLauncher> launcher = toolchains.launcherFor(javaExtension.toolchain) + + // The index is written twice, because the two things that read it need different guarantees. + // + // This one exists before this project is compiled, so that a call to a tag the project itself + // declares can be resolved as it compiles. It is read from source, so it cannot describe + // everything: a tag library referring to a type written in another language, or generated by + // the build, is left out, and what was missed is recorded so that nothing in an incompletely + // described namespace is reported as a misspelling. It is never packaged - a consumer must not + // be given a partial description - and pages are not compiled against it either. + // Everything both indexes must agree on is configured once, by type. Configuring the two + // tasks separately would let them describe different sets of tag libraries, and the one that + // is published is not the one this project compiles against - so they would diverge silently. + // A project keeping tag libraries elsewhere adds them the same way. + tasks.withType(GenerateTagLibraryIndexTask).configureEach { GenerateTagLibraryIndexTask index -> + index.sourceDirectories.from(project.layout.projectDirectory.dir('grails-app/taglib')) + index.parameterNamesRetained.set(resolvePreserveParameterNames(project)) + index.strictTags.set(resolveStrictTags(project)) + index.dynamicTagNamespaces.set(resolveDynamicTagNamespaces(project)) + index.javaLauncher.convention(launcher) + } + + // The settings live apart from the descriptors. The descriptors are published; the settings + // say how this project is compiled and must reach no one else, and a directory on the runtime + // classpath is copied wholesale into an executable archive, where excluding a file from an + // archive task cannot reach it. + Provider<Directory> settingsDir = project.layout.buildDirectory.dir('generated/grails-taglib-settings') + Provider<Directory> tagLibIndexDir = project.layout.buildDirectory.dir('generated/grails-taglibs') + def generateTagLibraryIndex = tasks.register('generateTagLibraryIndex', GenerateTagLibraryIndexTask) { + it.destinationDirectory.set(tagLibIndexDir) + it.settingsDirectory.set(settingsDir) + it.generatorClasspath.from(project.configurations.named('compileClasspath')) + // A tag library referring to a service, base class or trait of this project needs that + // source to be read, not guessed, or it would be described wrongly or not at all. + it.resolutionSourceRoots.from(project.provider { resolveGroovySourceRoots(mainSourceSet) }) + } + FileCollection tagLibIndex = project.files(tagLibIndexDir).builtBy(generateTagLibraryIndex) + + // And this one is written again once the project has been compiled, with its own classes on + // the classpath, where every tag library resolves whatever language it was written in. It is + // the authoritative index: the one pages are compiled against, the one packaged, and the one a + // project depending on this one reads. Every run replaces the directory, so a renamed or + // deleted tag library cannot survive in it. + Provider<Directory> packagedIndexDir = + project.layout.buildDirectory.dir('generated/grails-taglibs-packaged') + Provider<Directory> packagedSettingsDir = + project.layout.buildDirectory.dir('generated/grails-taglib-settings-packaged') + def packageTagLibraryIndex = tasks.register('packageTagLibraryIndex', GenerateTagLibraryIndexTask) { + it.description = 'Regenerates the tag library index against the compiled project' + it.destinationDirectory.set(packagedIndexDir) + it.settingsDirectory.set(packagedSettingsDir) + // The compiled classes, and a dependency on the task that gathers them, so this waits + // for everything that writes into those directories rather than for the compile tasks + // alone - the ast classes are copied in after compiling, for one. + // + // Deliberately the class directories and not the whole source set output. A view compiler + // registers its own output directory into that output and runs after the classes task, so + // it cannot declare the classes task as its producer without a cycle, and anything reading + // the whole output is left consuming a directory nothing says it produced. Compiled views + // are no use in resolving what a tag library declares anyway. + it.generatorClasspath.from(project.configurations.named('compileClasspath'), classesDirs) + it.dependsOn(tasks.named('classes')) + } + FileCollection packagedSettings = + project.files(packagedSettingsDir).builtBy(packageTagLibraryIndex) + FileCollection packagedTagLibIndex = + project.files(packagedIndexDir).builtBy(packageTagLibraryIndex) + + // Pages resolve tag calls against the index and are compiled in a process of their own, so the + // authoritative index has to be on their classpath. FileCollection allClasspath = project.getObjects().fileCollection().from( [ project.configurations.named('compileClasspath'), classesDirs, + packagedTagLibIndex, + packagedSettings, project.configurations.findByName('providedCompile') ?: null ].findAll { it } ) - // The Java the rest of the project is built with, so that pages are built with it too. - // Absent a toolchain this resolves to the JVM running Gradle, which is what compiling - // pages fell back to before and remains the right answer when nothing else was asked for. - JavaPluginExtension javaExtension = project.extensions.getByType(JavaPluginExtension) - JavaToolchainService toolchains = project.extensions.getByType(JavaToolchainService) - Provider<JavaLauncher> launcher = toolchains.launcherFor(javaExtension.toolchain) + // Carried into the artifact and onto the runtime classpath directly rather than through + // processResources, which the classes task waits for - and this waits for the classes task. + if (mainSourceSet != null) { + mainSourceSet.runtimeClasspath = mainSourceSet.runtimeClasspath.plus(packagedTagLibIndex) Review Comment: This puts the packaged index on `main.runtimeClasspath` only. The Java plugin derives `test.runtimeClasspath` from `test.output + main.output + configurations.testRuntimeClasspath`, not from `main.runtimeClasspath`, so tests never see it. That means a GSP rendered from a test resolves its tags against a different index than the same page in production — the application's own tag libraries are missing from it. Given that GSP rendering tests are how most people would notice a tag resolution problem, this is the one place the index most wants to be consistent. Adding it to `sourceSets.test.runtimeClasspath` (and `integrationTest`, where the convention plugin defines one) would close the gap. Worth an assertion in `TagLibraryIndexWiringFunctionalSpec` alongside the existing `RUNTIME_SEES_PACKAGED=true` check. ########## grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GenerateTagLibraryIndexTask.groovy: ########## @@ -0,0 +1,198 @@ +/* + * 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.gradle.plugin.views.gsp + +import javax.inject.Inject + +import groovy.transform.CompileStatic +import org.gradle.api.DefaultTask +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.provider.Property +import org.gradle.api.tasks.CacheableTask +import org.gradle.api.tasks.Classpath +import org.gradle.api.tasks.IgnoreEmptyDirectories +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Optional +import org.gradle.api.tasks.OutputDirectory +import org.gradle.api.provider.SetProperty +import org.gradle.api.tasks.Nested +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.TaskAction +import org.gradle.jvm.toolchain.JavaLauncher +import org.gradle.api.tasks.util.PatternSet +import org.gradle.process.ExecOperations +import org.gradle.process.JavaExecSpec + +/** + * Writes the tag library index describing the tag libraries in this project. + * + * <p>The index has to exist before anything that resolves tag calls is compiled, which is why this + * runs ahead of compilation rather than being produced as a side effect of it. Generating it for the + * whole source set at once is also what lets a renamed or deleted tag library disappear from it, + * where an index accumulated class by class keeps describing tags that no longer exist. + * + * <p>The work runs in a forked process against the project's own compile classpath, because the rules + * that decide what a tag is belong to the framework being built rather than to the build tooling, and + * must be the same rules the application applies when it starts. + * + * @since 8.0.0 + */ +@CacheableTask +@CompileStatic +abstract class GenerateTagLibraryIndexTask extends DefaultTask { + + static final String GENERATOR_CLASS = 'org.grails.taglib.index.TagLibraryIndexGenerator' + + private final ExecOperations execOperations + + @Inject + GenerateTagLibraryIndexTask(ExecOperations execOperations) { + this.execOperations = execOperations + description = 'Generates the tag library index used to resolve tag calls at compile time' + group = 'build' + } + + /** + * The directories holding tag library sources. + * + * <p>Defaults to {@code grails-app/taglib}. A project keeping tag libraries elsewhere can add + * those directories, which is what makes them resolvable in the same compilation that defines + * them; without that they are still described as they compile, and so are resolvable to whatever + * is compiled afterwards. + */ + @InputFiles + @IgnoreEmptyDirectories + @PathSensitive(PathSensitivity.RELATIVE) + abstract ConfigurableFileCollection getSourceDirectories() + + /** + * The source roots a type this project declares may be resolved from. + * + * <p>A tag library commonly refers to a service, base class or trait of the same project, none of + * which exist as classes yet. Their source is compiled alongside it so that what they contribute - + * a namespace, tags, a parameter type - is read rather than guessed. + */ + @InputFiles + @IgnoreEmptyDirectories + @PathSensitive(PathSensitivity.RELATIVE) + abstract ConfigurableFileCollection getResolutionSourceRoots() + + /** + * Where the index is written. Placed on the compile classpath and packaged with the artifact. + */ + @OutputDirectory + abstract DirectoryProperty getDestinationDirectory() + + /** + * Where the settings this build declared are written. + * + * <p>Kept apart from the descriptors because the two travel differently: the descriptors are + * published, and the settings say how this project is compiled and must reach no one else. Sharing + * a directory would put them wherever the descriptors go, including into an executable archive + * built from the runtime classpath, where no exclusion on an archive task can reach them. + * + * <p>Written beside the descriptors when unset, which suits a caller with nothing to publish. + */ + @OutputDirectory + @Optional + abstract DirectoryProperty getSettingsDirectory() + + /** + * The classpath the generator runs against, which supplies the framework's discovery rules. + */ + @Classpath + abstract ConfigurableFileCollection getGeneratorClasspath() + + /** + * Whether this compilation writes parameter names into class files. It decides whether a tag's + * attributes and body parameters have to carry those names to be dispatchable, so the index must + * be generated under the same setting the sources are compiled with. + */ + @Input + abstract Property<Boolean> getParameterNamesRetained() + + /** + * The source encoding, matching the one compilation uses. + */ + @Input + @Optional + abstract Property<String> getSourceEncoding() Review Comment: `sourceEncoding` is declared but never set by `GroovyPagePlugin` — neither in the `withType(GenerateTagLibraryIndexTask).configureEach` block nor on either task — so both indexes are always generated as UTF-8 regardless of `compileGroovy.options.encoding`. For a project that sets a non-UTF-8 encoding the generator will read sources differently from the compiler that will later compile them, and a tag name or namespace containing a non-ASCII character would be mis-decoded (silently, since the index degrades to dynamic dispatch rather than failing). The `configureEach` block already exists for exactly this kind of shared setting: ```groovy index.sourceEncoding.convention(project.provider { (tasks.findByName('compileGroovy') as GroovyCompile)?.options?.encoding }) ``` Same for `parameterNamesRetained`, which is read from the `grails` extension rather than from the actual `compileGroovy` configuration it has to agree with — worth a comment explaining why those can't diverge. ########## grails-gsp/grails-web-taglib/src/main/groovy/grails/gsp/taglib/compiler/LocalNameCollector.java: ########## @@ -0,0 +1,107 @@ +/* + * 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 grails.gsp.taglib.compiler; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import org.codehaus.groovy.ast.CodeVisitorSupport; +import org.codehaus.groovy.ast.Parameter; +import org.codehaus.groovy.ast.expr.ClosureExpression; +import org.codehaus.groovy.ast.expr.DeclarationExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.TupleExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.ast.stmt.ForStatement; +import org.codehaus.groovy.ast.stmt.Statement; + +/** + * Collects every name declared within a body: its parameters, its local variables, the parameters of + * the closures inside it and the variables its loops introduce. + * + * <p>Used to decide whether an unqualified call such as {@code message(code: 'x')} could be reaching + * something local rather than a tag. Scope is not tracked, so a name declared anywhere in the body + * counts throughout it. That errs towards leaving a call to be dispatched dynamically, which is only + * a missed optimisation, rather than towards sending it somewhere the author did not write. + * + * @since 8.0.0 + */ +final class LocalNameCollector extends CodeVisitorSupport { + + private final Set<String> names = new HashSet<>(); + + private LocalNameCollector() { + } + + /** + * @param code the body to read, or {@code null} when there is none + * @param parameters the declaring method's parameters, or {@code null} when there are none + * @return every name declared within, never {@code null} + */ + static Set<String> collect(Statement code, Parameter[] parameters) { + LocalNameCollector collector = new LocalNameCollector(); + collector.addParameters(parameters); + if (code != null) { + code.visit(collector); + } + return collector.names.isEmpty() ? Collections.emptySet() : collector.names; + } + + private void addParameters(Parameter[] parameters) { + if (parameters == null) { + return; + } + for (Parameter parameter : parameters) { + names.add(parameter.getName()); + } + } + + @Override + public void visitDeclarationExpression(DeclarationExpression expression) { + if (expression.isMultipleAssignmentDeclaration()) { + TupleExpression tuple = expression.getTupleExpression(); + for (Expression declared : tuple.getExpressions()) { + if (declared instanceof VariableExpression variable) { + names.add(variable.getName()); + } + } + } + else { + names.add(expression.getVariableExpression().getName()); + } + super.visitDeclarationExpression(expression); + } + + @Override + public void visitClosureExpression(ClosureExpression expression) { + if (expression.isParameterSpecified()) { + addParameters(expression.getParameters()); + } + super.visitClosureExpression(expression); + } + + @Override + public void visitForLoop(ForStatement forLoop) { + if (forLoop.getVariable() != null) { Review Comment: `ForStatement.getVariable()` is deprecated in Groovy 5 — javac reports it on every build of this module: ``` Note: .../LocalNameCollector.java uses or overrides a deprecated API. ``` The replacement is `getIndexVariable()` / `getValueVariable()` (and the classic form now carries both), so this also currently misses the index variable of a classic `for (int i = 0; ...)` loop. Two other gaps in the same collector, both in the "leaves a call dynamic" direction so neither is unsound, but both cost the optimisation and are cheap to close: - `catch` parameters. `CodeVisitorSupport.visitTryCatchFinally` visits the catch *body* but not its `Parameter`, so a `catch (SomeException message)` doesn't claim `message`. - an implicit-`it` closure. `visitClosureExpression` only adds parameters when `isParameterSpecified()`, so `it` is never collected. Worth a `visitCatchStatement` override and adding `"it"` for the implicit case. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndexWriter.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.taglib.index; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collection; +import java.util.Map; +import java.util.Properties; +import java.util.TreeMap; +import java.util.TreeSet; + +/** + * Writes the compile-time descriptor for a single tag library. + * + * <p>Two files are produced per tag library: a descriptor named after the tag library class, and an + * entry in a shared {@code index.properties} manifest naming it. The manifest exists because a + * classpath directory cannot be enumerated from inside a jar, so the reader needs the names up front. + * Both live under {@link TagLibraryIndex#INDEX_LOCATION} and merge across jars without a build step. + * + * @since 8.0.0 + */ +public final class TagLibraryIndexWriter { + + private TagLibraryIndexWriter() { + } + + /** + * Removes any index previously written beneath a directory, so that a regenerated index describes + * only the tag libraries that exist now. Without this a renamed or deleted tag library would keep + * a descriptor, and the manifest naming it, until the build directory was cleaned. + * + * @param outputDirectory the directory the index is written beneath + * @throws IOException if an existing index cannot be removed + */ + public static void clear(File outputDirectory) throws IOException { + if (outputDirectory == null) { + return; + } + File indexDirectory = new File(outputDirectory, TagLibraryIndex.INDEX_LOCATION); + File[] existing = indexDirectory.listFiles(); + if (existing == null) { + return; + } + for (File file : existing) { + if (file.isFile() && file.getName().endsWith(".properties")) { + Files.deleteIfExists(file.toPath()); + } + } + } + + /** + * Writes the descriptor for a tag library into a compiler output directory. + * + * @param outputDirectory the compilation target directory; nothing is written when {@code null} + * @param className the binary name of the tag library + * @param namespace the namespace the tag library declares + * @param tagNames the tag names the tag library declares + * @throws IOException if the descriptor cannot be written + */ + public static void write(File outputDirectory, String className, String namespace, + Collection<String> tagNames) throws IOException { + Map<String, TagLibraryIndexEntry.Kind> asMethods = new TreeMap<>(); + for (String tagName : tagNames) { + asMethods.put(tagName, TagLibraryIndexEntry.Kind.METHOD); + } + write(outputDirectory, className, namespace, asMethods); + } + + /** + * Writes the descriptor for a tag library, recording how each tag is implemented. + * + * @param outputDirectory the compilation target directory; nothing is written when {@code null} + * @param className the binary name of the tag library + * @param namespace the namespace the tag library declares + * @param tags each tag mapped to how it is implemented + * @throws IOException if the descriptor cannot be written + */ + public static void write(File outputDirectory, String className, String namespace, + Map<String, TagLibraryIndexEntry.Kind> tags) throws IOException { + if (outputDirectory == null || className == null || className.isEmpty() || + namespace == null || namespace.isEmpty()) { + return; + } + File indexDirectory = new File(outputDirectory, TagLibraryIndex.INDEX_LOCATION); + if (!indexDirectory.isDirectory() && !indexDirectory.mkdirs() && !indexDirectory.isDirectory()) { + return; + } + + Properties descriptor = new Properties(); + descriptor.setProperty(TagLibraryIndex.VERSION_KEY, String.valueOf(TagLibraryIndex.FORMAT_VERSION)); + descriptor.setProperty(TagLibraryIndex.NAMESPACE_KEY, namespace); + descriptor.setProperty(TagLibraryIndex.CLASS_KEY, className); + // Sorted so that recompiling unchanged sources produces byte-identical output, which keeps + // the build reproducible and avoids spurious up-to-date checks failing downstream. + // Recorded as "name:KIND" so that a caller can tell a tag it can bind to from one that has to + // be dispatched dynamically, without a second file or a nested format. + StringBuilder encoded = new StringBuilder(); + for (Map.Entry<String, TagLibraryIndexEntry.Kind> tag : new TreeMap<>(tags).entrySet()) { + if (encoded.length() > 0) { + encoded.append(','); + } + encoded.append(tag.getKey()).append(':').append(tag.getValue().name()); + } + descriptor.setProperty(TagLibraryIndex.TAGS_KEY, encoded.toString()); + store(new File(indexDirectory, className + ".properties"), descriptor); + + File manifest = new File(indexDirectory, "index.properties"); Review Comment: Two things about the shared manifest. **Read-modify-write with no synchronisation.** Every tag library compiled into the same target directory loads `index.properties`, adds one key and writes the whole file back. Within a single `CompilationUnit` that's sequential, but Groovy joint compilation and any parallel compile task targeting the same output directory would interleave and lose entries. A lost entry is silent — the descriptor exists but is never discovered, so tags fall back to dynamic dispatch with nothing reported. A per-class marker file (the `META-INF/services` pattern this class cites as its model) or a lock would remove the shared mutable file entirely. **Stale entries.** In the self-describing path nothing ever calls `clear()`, so a renamed or deleted tag library keeps both its descriptor and its manifest entry until the build directory is cleaned. `TagLibArtefactTypeAstTransformation.writeIndexEntry` acknowledges this in a comment as the reason `buildOwnsIndex()` exists — but that only sidesteps it under the Gradle plugin. Everywhere else the problem is real and unmitigated: reading a descriptor for a class that no longer exists puts phantom tags into the index, which under `strictTags` is the difference between an error and no error. At minimum this should be called out in `compiledTags.adoc` next to the "each tag library describes itself as it is compiled" paragraph. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/discovery/TagLibraryAstDiscovery.java: ########## @@ -0,0 +1,137 @@ +/* + * 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.taglib.discovery; + +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; + +import groovy.lang.Closure; +import org.codehaus.groovy.ast.ClassHelper; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.FieldNode; +import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.expr.ConstantExpression; +import org.codehaus.groovy.ast.expr.Expression; + +import org.grails.taglib.index.TagLibraryIndexEntry; + +/** + * Reads a tag library's namespace and tag names from its syntax tree. + * + * <p>Classification is delegated to {@link TagDiscoveryRules}, the same rules an application applies + * when it registers tag libraries, so the two cannot disagree about what a tag is. What remains here + * is reading the namespace and gathering the candidate members from the tree. + * + * @since 8.0.0 + */ +public final class TagLibraryAstDiscovery { + + public static final String DEFAULT_NAMESPACE = "g"; + + private static final String NAMESPACE_FIELD = "namespace"; + + private static final ClassNode CLOSURE_TYPE = ClassHelper.make(Closure.class); + + private TagLibraryAstDiscovery() { + } + + /** + * Resolves the namespace the way {@code DefaultGrailsTagLibClass} does at runtime, which reads the + * static {@code namespace} property through the class hierarchy. + * + * @param classNode the tag library + * @return the namespace, or {@code null} when it cannot be determined without running the code, in + * which case no descriptor should be written and the tag library resolves dynamically + */ + public static String resolveNamespace(ClassNode classNode) { + for (ClassNode current = classNode; current != null && !ClassHelper.isObjectType(current); + current = current.getSuperClass()) { + FieldNode namespaceField = current.getDeclaredField(NAMESPACE_FIELD); + if (namespaceField == null || !namespaceField.isStatic()) { + continue; + } + Expression initial = namespaceField.getInitialExpression(); + if (initial instanceof ConstantExpression constant && constant.getValue() != null) { + String value = constant.getValue().toString().trim(); + return value.isEmpty() ? DEFAULT_NAMESPACE : value; + } + // Declared, but its value is only known once the initialiser runs - a reference to a shared + // constant, a concatenation, and so on. Guessing "g" here would file the tags under the + // wrong namespace, so the tag library is left out of the index entirely. + return null; + } + return DEFAULT_NAMESPACE; + } + + /** + * @param classNode the tag library + * @param parameterNamesRetained whether this compilation writes parameter names into the class file + * @return every tag the library declares, whether as a tag method or a legacy closure field + */ + /** Review Comment: Two javadoc blocks stacked on `findTags` — the first describes the old `Collection<String>` return and should be deleted. Also, `findTagNames` just below is a near-duplicate of `findTags` with the same two loops and the same declaring-class guard, and has no callers in main code. Worth deleting it, or implementing it as `findTags(...).keySet()` so the two can't drift. One behavioural note on the field loop in `findTags`: it runs after the method loop and unconditionally `put`s `LEGACY_CLOSURE`, so a tag that is both a `Closure` field and a method gets recorded as `LEGACY_CLOSURE`. That's invisible today because nothing reads `Kind`, but it's the wrong way round if `Kind` ever starts driving a decision. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndex.java: ########## @@ -0,0 +1,485 @@ +/* + * 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.taglib.index; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.WeakHashMap; + +/** + * The set of tag libraries and tag names known at compile time. + * + * <p>Each tag library contributes one descriptor under {@value #INDEX_LOCATION}, written by the + * {@code TagLib} AST transformation as the tag library is compiled. Descriptors are per class rather + * than per module so that libraries packaged in separate jars merge on the classpath without any + * build step having to combine them, in the same way {@code META-INF/services} entries do. + * + * <p>Reading the index answers "which tags exist in namespace x" without loading or reflecting over a + * single tag library class, which is what allows GSP expressions to be resolved when a page is + * compiled rather than dispatched dynamically when it renders. + * + * @since 8.0.0 + */ +public final class TagLibraryIndex { + + /** + * Classpath directory holding one descriptor per compiled tag library. + */ + public static final String INDEX_LOCATION = "META-INF/grails/taglibs/"; + + /** + * Descriptor format this build writes and understands. A descriptor carrying anything else was + * produced by a different version of Grails and is ignored, so its tags resolve dynamically rather + * than being read under the wrong set of rules. + */ + public static final int FORMAT_VERSION = 2; + + /** + * Settings the build states for the compilation the index is read in, written alongside the + * descriptors by the build and deliberately not packaged into the artifact: they describe how this + * project is compiled, not what its tag libraries declare. + */ + public static final String SETTINGS_LOCATION = INDEX_LOCATION + "compile-settings.properties"; + + /** + * What the index could not describe, written by whatever produced it. + * + * <p>An index generated before its project is compiled cannot always read every tag library: one + * referring to a type that does not exist yet, in a language it cannot parse, or generated by the + * build itself, is left out. A namespace missing some of its tags must not have a call to one of + * them reported as a misspelling, so what was missed is recorded rather than left to be inferred + * from the absence. + */ + public static final String INCOMPLETE_LOCATION = INDEX_LOCATION + "incomplete.properties"; + + static final String VERSION_KEY = "version"; + static final String NAMESPACE_KEY = "namespace"; + static final String CLASS_KEY = "class"; + static final String TAGS_KEY = "tags"; + static final String STRICT_KEY = "strictTags"; + static final String INCOMPLETE_NAMESPACES_KEY = "namespaces"; + static final String INCOMPLETE_ALL_KEY = "all"; + static final String DYNAMIC_NAMESPACES_KEY = "dynamicTagNamespaces"; + + /** + * One index per class loader. A compilation gets a class loader of its own, so this is read once + * per compilation rather than once per source file, and is not held after that compilation ends. + * Caching in a plain static field instead would carry one project's tag libraries into the next + * compilation in the same Gradle daemon. + */ + private static final Map<ClassLoader, TagLibraryIndex> BY_CLASS_LOADER = + Collections.synchronizedMap(new WeakHashMap<>()); + + private final Map<String, Map<String, TagLibraryIndexEntry>> byNamespace; + private final Map<String, Set<String>> ambiguousByNamespace; + private final Map<String, Set<String>> tagNamesByClass; + private final boolean strict; + private final Set<String> dynamicNamespaces; + private final Set<String> incompleteNamespaces; + private final boolean everythingIncomplete; + + private TagLibraryIndex(Map<String, Map<String, TagLibraryIndexEntry>> byNamespace, + Map<String, Set<String>> ambiguousByNamespace, Map<String, Set<String>> tagNamesByClass, + boolean strict, Set<String> dynamicNamespaces, Set<String> incompleteNamespaces, + boolean everythingIncomplete) { + this.byNamespace = byNamespace; + this.ambiguousByNamespace = ambiguousByNamespace; + this.tagNamesByClass = tagNamesByClass; + this.strict = strict; + this.dynamicNamespaces = dynamicNamespaces; + this.incompleteNamespaces = incompleteNamespaces; + this.everythingIncomplete = everythingIncomplete; + } + + /** + * Reads the index for a class loader, reusing the one already read for it. + * + * <p>Reading walks every jar on the classpath, so a compiler that consults the index for each + * source file it compiles would walk it once per file. Use this from compilation; use + * {@link #load(ClassLoader)} where a fresh read is wanted. + * + * @param classLoader the loader to scan; when {@code null} the thread context loader is used + * @return the merged index, never {@code null} + */ + public static TagLibraryIndex forClassLoader(ClassLoader classLoader) { + ClassLoader loader = classLoader != null ? classLoader : Thread.currentThread().getContextClassLoader(); + if (loader == null) { + return load(null); + } + return BY_CLASS_LOADER.computeIfAbsent(loader, TagLibraryIndex::load); + } + + /** + * Reads every tag library descriptor visible to the given class loader. + * + * @param classLoader the loader to scan; when {@code null} the thread context loader is used + * @return the merged index, never {@code null} + */ + public static TagLibraryIndex load(ClassLoader classLoader) { + ClassLoader loader = classLoader != null ? classLoader : Thread.currentThread().getContextClassLoader(); + Map<String, Map<String, TagLibraryIndexEntry>> merged = new TreeMap<>(); + Map<String, Set<String>> ambiguous = new TreeMap<>(); + Map<String, Set<String>> byClass = new TreeMap<>(); + if (loader == null) { + return new TagLibraryIndex(merged, ambiguous, byClass, false, Collections.emptySet(), + Collections.emptySet(), false); + } + // A directory resource enumerates its children on some classpath layouts but not inside jars, + // so the descriptors are discovered through the manifest of names each descriptor records + // rather than by listing the directory. + for (URL url : listDescriptors(loader)) { + Properties properties = read(url); + if (properties == null) { + continue; + } + if (!String.valueOf(FORMAT_VERSION).equals(properties.getProperty(VERSION_KEY))) { + continue; + } + String namespace = properties.getProperty(NAMESPACE_KEY); + String className = properties.getProperty(CLASS_KEY); + String tags = properties.getProperty(TAGS_KEY, ""); + if (namespace == null || namespace.isEmpty() || className == null || className.isEmpty()) { + continue; + } + // Recorded from the descriptor rather than from its tags, so that a tag library declaring + // none of them is still known to have been described. Deciding that from the tags alone + // would have such a tag library described twice. + byClass.computeIfAbsent(className, k -> new TreeSet<>()); + Map<String, TagLibraryIndexEntry> tagsForNamespace = + merged.computeIfAbsent(namespace, k -> new TreeMap<>()); + for (String encodedTag : tags.split(",")) { + String trimmed = encodedTag.trim(); + if (trimmed.isEmpty()) { + continue; + } + // Recorded as "name:KIND"; an unrecognised kind is treated as the dynamic one so that a + // descriptor from a later version cannot cause a call to be bound wrongly. + int separator = trimmed.lastIndexOf(':'); + String tagName = separator > 0 ? trimmed.substring(0, separator) : trimmed; + TagLibraryIndexEntry.Kind kind = TagLibraryIndexEntry.Kind.LEGACY_CLOSURE; + if (separator > 0) { + try { + kind = TagLibraryIndexEntry.Kind.valueOf(trimmed.substring(separator + 1)); + } catch (IllegalArgumentException unknownKind) { + kind = TagLibraryIndexEntry.Kind.LEGACY_CLOSURE; + } + } + trimmed = tagName; + // Recorded against the declaring class before ambiguity is considered, so that asking + // what one tag library declares is answered from its own descriptor and is unaffected + // by whether some other tag library happens to declare the same name. + byClass.computeIfAbsent(className, k -> new TreeSet<>()).add(trimmed); + TagLibraryIndexEntry existing = tagsForNamespace.get(trimmed); + if (existing != null && !existing.tagLibraryClassName().equals(className)) { + // At runtime the tag library registered last wins, and registration order comes + // from artefact scanning rather than from classpath order, so which of these two + // will win cannot be known here. Resolving it either way risks compiling against + // one implementation and dispatching to the other, so the tag is marked ambiguous + // and left to runtime resolution. + ambiguous.computeIfAbsent(namespace, k -> new TreeSet<>()).add(trimmed); + continue; + } + tagsForNamespace.put(trimmed, + new TagLibraryIndexEntry(namespace, trimmed, className, kind, true)); + } + } + Properties settings = readSettings(loader); + boolean strict = Boolean.parseBoolean(settings.getProperty(STRICT_KEY, "false")); + Set<String> dynamic = new TreeSet<>(); + for (String namespace : settings.getProperty(DYNAMIC_NAMESPACES_KEY, "").split(",")) { + String trimmed = namespace.trim(); + if (!trimmed.isEmpty()) { + dynamic.add(trimmed); + } + } + Set<String> incomplete = new TreeSet<>(); + boolean allIncomplete = false; + for (URL url : urls(loader, INCOMPLETE_LOCATION)) { + Properties recorded = read(url); + if (recorded == null) { + continue; + } + allIncomplete |= Boolean.parseBoolean(recorded.getProperty(INCOMPLETE_ALL_KEY, "false")); + for (String namespace : recorded.getProperty(INCOMPLETE_NAMESPACES_KEY, "").split(",")) { + String trimmed = namespace.trim(); + if (!trimmed.isEmpty()) { + incomplete.add(trimmed); + } + } + } + return new TagLibraryIndex(merged, ambiguous, byClass, strict, + Collections.unmodifiableSet(dynamic), Collections.unmodifiableSet(incomplete), + allIncomplete); + } + + private static Set<URL> urls(ClassLoader loader, String location) { + Set<URL> found = new LinkedHashSet<>(); + try { + Enumeration<URL> resources = loader.getResources(location); + while (resources.hasMoreElements()) { + found.add(resources.nextElement()); + } + } + catch (IOException unreadable) { + return found; + } + return found; + } + + /** + * Reads the settings the build states for this compilation. Only the project being compiled + * contributes them, so the first one found wins rather than several being merged. + */ + private static Properties readSettings(ClassLoader loader) { + URL url = loader.getResource(SETTINGS_LOCATION); + if (url == null) { + return new Properties(); + } + Properties settings = read(url); + return settings != null ? settings : new Properties(); + } + + private static Set<URL> listDescriptors(ClassLoader loader) { Review Comment: This issues one `ClassLoader.getResources()` call — a full scan of every entry on the classpath — for **each** class name found in **each** `index.properties` manifest. For an application with a few hundred tag libraries over a few hundred jars that's a few hundred full classpath walks on first read. It's cached per class loader by `forClassLoader`, so it's paid once per compilation rather than per source file, and the comment correctly explains why a directory resource can't just be listed. But the manifest already names the classes, and the descriptors always sit alongside the manifest that names them — so the URLs can be derived from each manifest URL directly (resolve the sibling `<class>.properties` against the manifest's own URL) instead of re-querying the loader. That turns O(taglibs × classpath) into O(taglibs). Not urgent given the caching, but it will show up in cold-build timings on large applications. ########## grails-mail/src/main/groovy/grails/plugins/mail/PlainTextMailTagLib.groovy: ########## @@ -22,7 +22,7 @@ class PlainTextMailTagLib { static namespace = 'text' - def newLine = { + def newLine(Map attrs) { Review Comment: Why is this here? It's a change to a different module's published tag signature, in a PR about compile-time tag resolution, with no test and no mention in the description. `def newLine = { out << '\n' }` is an `Object`-typed field, so it isn't picked up by `TagLibraryAstDiscovery`'s `Closure`-typed field scan, and this file isn't under `grails-app/taglib` so `TagLibraryTransformer` never forced the type or generated wrappers for it. As far as I can tell it therefore wasn't in the index either before or after, and nothing in this PR requires the change. If it's an unrelated fix for a `text:newLine` tag that was already broken, it deserves its own commit and a spec. If this PR *does* require it, that implies something about zero-parameter closure tags that should be explained — and tested — because other plugins will have the same shape. ########## grails-gsp/plugin/src/test/groovy/org/grails/web/taglib/ControllerTagCallRewriteSpec.groovy: ########## @@ -0,0 +1,88 @@ +/* + * 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 java.nio.file.Files +import java.nio.file.Path + +import org.codehaus.groovy.control.CompilationUnit +import org.codehaus.groovy.control.CompilerConfiguration +import spock.lang.Specification +import spock.lang.TempDir + +/** + * A controller can call tags too, through the tag library invoker trait rather than by being a tag + * library, so the same rewriting has to reach it. + * + * <p>Checked in the class file, because a rewritten call and a dynamic one produce the same output. + */ +class ControllerTagCallRewriteSpec extends Specification { + + @TempDir + Path tempDir + + void 'a class that can call tags has its tag calls compiled into invocations'() { + when: 'a class carrying the tag library invoker trait, as a controller does' + byte[] compiled = compile(''' + import grails.artefact.gsp.TagLibraryInvoker + class TagCallingController implements TagLibraryInvoker { Review Comment: This hand-writes `implements TagLibraryInvoker`, which is the one shape a controller never has in real code. It therefore can't catch the ordering problem described on `CompiledTagCallTransformation`: a real `@Artefact('Controller')` class gets the trait from a *local* transform that runs after this global one, and is not rewritten (I verified this). Please add two cases: - a class under a `grails-app/controllers/...` path, which exercises the `GlobalGrailsClassInjectorTransformation` route that works today; - a class carrying `@Artefact('Controller')` outside that directory, which currently does not get rewritten. The second one should fail as written today, which is the point. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/TagLibraryMetaUtils.groovy: ########## @@ -32,6 +32,18 @@ import grails.core.gsp.GrailsTagLibClass import grails.util.GrailsClassUtils import org.grails.taglib.encoder.OutputContextLookupHelper +/** + * Installs tags onto metaclasses. + * + * <p>Tags are resolved through {@link TagLibraryLookup} and invoked through + * {@link CompiledTagInvocation}, so nothing needs installing onto a metaclass to call a tag. What + * remains here is the dynamic dispatch that a tag library registered at runtime still relies on, + * reachable through {@code methodMissingForTagLib} with metaclass installation switched off. + * + * @deprecated Installing tags onto metaclasses is no longer part of dispatching a tag. Resolve + * through {@link TagLibraryLookup} and invoke through {@link CompiledTagInvocation}. + */ +@Deprecated Review Comment: Deprecating the whole class overstates it and will produce warnings for code that has no alternative. `methodMissingForTagLib` is not going anywhere — it is the dynamic dispatch path, and this PR *increases* its use: `TagLibraryInvoker.methodMissing`, `NamespacedTagDispatcher.methodMissing` and the new `GroovyPage.methodMissing` all call it, and `GrailsWebUnitTest` still calls `enhanceTagLibMetaClass` deliberately. Marking the container `@Deprecated` says "stop using this" about a method the framework requires. Suggest deprecating the metaclass-installing methods specifically — `enhanceTagLibMetaClass`, `registerTagMetaMethods`, `registerMethodMissingForTags`, `registerNamespaceMetaProperties`, `registerPropertyMissingForTag`, `addTagLibMethodToMetaClass` — and leaving `methodMissingForTagLib` undeprecated as the supported entry point. Also please add `since` and, if the intent is removal, `forRemoval = true` with a target, per the deprecation convention used elsewhere in the codebase. Same for the `@Deprecated` on `GroovyPagesMetaUtils.registerMethodMissingForGSP`, which is now a silent no-op — a method that no longer does what its name says is a stronger candidate for removal-with-an-upgrade-note than for quiet deprecation. -- 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]
