codeconsole commented on code in PR #16134: URL: https://github.com/apache/grails-core/pull/16134#discussion_r3827398804
########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/discovery/TagDiscoveryRules.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.discovery; + +import java.util.LinkedHashSet; +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( + "invokeMethod", "methodMissing", "propertyMissing", "getProperty", "setProperty", + "getMetaClass", "setMetaClass", "equals", "hashCode", "toString"); + + /** + * Names every tag library carries through the framework traits and lifecycle interfaces. + */ + private static final Set<String> FRAMEWORK_METHOD_NAMES = Set.of( + "afterPropertiesSet", + "currentRequestAttributes", + "destroy", + "initializeTagLibrary", + "onApplicationEvent", + "raw", + "throwTagError", + "withCodec"); + + private TagDiscoveryRules() { + } + + /** + * @return the names that are never tags, whatever their shape + */ + public static Set<String> getFrameworkMethodNames() { + return FRAMEWORK_METHOD_NAMES; + } + + /** + * @param method the method to classify + * @return true if the method can be invoked as a tag + */ + /** + * Finds every tag a tag library declares, from either view of it. + * + * <p>The two kinds are enumerated differently, because the runtime dispatches them differently. A + * method tag is read from the declaring class alone, since dispatch scans declared methods and an + * inherited one is not callable as a tag. A closure tag is read up the whole hierarchy, since + * dispatch finds it as a property and a property is inherited. + * + * @param view the tag library, from a syntax tree or from a compiled class + * @return every tag name the library declares + */ + public static Set<String> findTags(TagLibraryView view) { Review Comment: All three fixed in 79a69b3 — `isTagMethod`, `TagLibraryIndex.isStrict` and `GrailsCompileStaticOptions.strictTags` each have their block back. Same slip as the two earlier ones; worth me checking for it deliberately rather than one report at a time. ########## grails-gsp/grails-web-taglib/src/test/groovy/org/grails/taglib/index/TagLibraryIndexGeneratorSpec.groovy: ########## @@ -0,0 +1,300 @@ +/* + * 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.nio.file.Files +import java.nio.file.Path + +import spock.lang.Specification +import spock.lang.TempDir + +/** + * Generating the index for a whole source set, rather than accumulating it as each class compiles, + * is what allows a renamed or deleted tag library to disappear from it. + */ +class TagLibraryIndexGeneratorSpec extends Specification { + + @TempDir + Path tempDir + + Path sources + Path output + + def setup() { + sources = Files.createDirectories(tempDir.resolve('src')) + output = Files.createDirectories(tempDir.resolve('out')) + } + + void 'a closure based tag is recorded as such'() { + given: + write('Legacy.groovy', ''' + import grails.gsp.TagLib + @TagLib + class LegacyTagLib { + static namespace = 'legacy' + def asMethod(Map attrs) { } + Closure asClosure = { Map attrs -> } + } + ''') + + when: + TagLibraryIndexGenerator.generate(sources.toFile(), output.toFile(), true, 'UTF-8') + + then: 'the closure form is marked so that callers keep dispatching it dynamically' Review Comment: Both fixed. The label now says both forms are described by name, and the blank line is in. ########## grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/TagLibraryMetaUtils.groovy: ########## @@ -199,6 +220,30 @@ class TagLibraryMetaUtils { existingMethod instanceof CachedMethod } + /** + * Whether an argument list is one a tag can be called with. + * + * <p>A tag takes attributes, a body, or both, which is none, one, or two arguments whose first is + * a Map. Anything else the switch below reduces to a call with no attributes and no body, silently + * dropping what was written - so a name that is both a tag and an ordinary overload, a tag + * {@code foo(Map)} beside a helper {@code foo(String, String)}, would run the tag with nothing. + * Such a call is left to the method lookup further down, which finds the overload. + * + * @param args the arguments the call was made with + * @return true when the call can be treated as a tag invocation + */ + private static boolean matchesTagShape(Object[] args) { Review Comment: Pinned as you suggested, in 79a69b3. `matchesTagShape` now states that overlapping shapes resolve to the tag deliberately, with your reasoning for why preferring the overload would be worse, and `TagLibraryInvokerDispatchSpec` has a row for the one-argument case — `format(String)` beside the tag, asserting the tag runs and its output is captured. -- 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]
