codeconsole commented on code in PR #16134: URL: https://github.com/apache/grails-core/pull/16134#discussion_r3793084714
########## 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: All three fixed — `getIndexVariable()`/`getValueVariable()` in place of the deprecated `getVariable()` (which also picks up the classic loop's index), a `visitCatchStatement` override for catch parameters, and `it` for an implicit-parameter closure. The javac deprecation note this module was printing is gone. ########## 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: Deleted. Porting it properly would mean giving `grails-benchmarks` the taglib and Spring dependencies it doesn't have, which is more scope rather than less. The measurements stay in the description. ########## 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: Both cases added, and then one removed again — worth explaining. The `@Artefact('Controller')` case is there and passes, confirming what you found: the trait is present, the call is not rewritten. The `grails-app/controllers` case I removed. It compiled a file into a temporary `grails-app/controllers` directory and relied on the artefact injector recognising it by location, which turns on where the compilation happens rather than what is compiled. It passed on macOS and failed on Ubuntu and Windows in every CI run and again on a rerun, and I couldn't reproduce it locally in any configuration. The convention path is exercised for real by every application under `grails-test-examples`; the spec now records the gap rather than leaving it silent. -- 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]
