[
https://issues.apache.org/jira/browse/GROOVY-12030?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18082996#comment-18082996
]
ASF GitHub Bot commented on GROOVY-12030:
-----------------------------------------
Copilot commented on code in PR #2554:
URL: https://github.com/apache/groovy/pull/2554#discussion_r3291653286
##########
src/test/groovy/groovy/transform/options/PropertyHandlerTest.groovy:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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
+ *
+ * http://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 groovy.transform.options
+
+import org.codehaus.groovy.ast.AnnotationNode
+import org.codehaus.groovy.ast.ClassNode
+import org.codehaus.groovy.ast.Parameter
+import org.codehaus.groovy.ast.PropertyNode
+import org.codehaus.groovy.ast.stmt.Statement
+import org.codehaus.groovy.transform.AbstractASTTransformation
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
+import static org.codehaus.groovy.ast.tools.GeneralUtils.assignS
+import static org.codehaus.groovy.ast.tools.GeneralUtils.constX
+import static org.codehaus.groovy.ast.tools.GeneralUtils.plusX
+import static org.codehaus.groovy.ast.tools.GeneralUtils.propX
+import static org.codehaus.groovy.ast.tools.GeneralUtils.varX
+
+/**
+ * Locks in the public contract of {@link PropertyHandler}: that user-supplied
+ * handlers wired in via {@code @PropertyOptions(propertyHandler = …)} are
+ * loaded, validated and consulted at compile time.
+ */
+final class PropertyHandlerTest {
+
+ @Test
+ void testCustomHandlerControlsPropertyInitialisation() {
+ assertScript '''
+ import groovy.transform.PropertyOptions
+ import groovy.transform.TupleConstructor
+ import
groovy.transform.options.PropertyHandlerTest.PrefixingPropertyHandler
+
+ @PropertyOptions(propertyHandler = PrefixingPropertyHandler)
+ @TupleConstructor
+ class Foo {
+ String name
+ }
+
+ assert new Foo('hello').name == 'X_hello'
+ '''
+ }
+
+ @Test
+ void testHandlerValidateAttributesCanRejectAnAttribute() {
+ // Exercises a list-valued attribute (`includes = [...]`); detection
relies on
+ // PropertyHandler.isValidAttribute using AnnotationNode.getMember
rather than
+ // the scalar-only AbstractASTTransformation.getMemberValue.
Review Comment:
The comment describing `AbstractASTTransformation.getMemberValue` as
"scalar-only" is a bit misleading; the key limitation is that it only returns
values for `ConstantExpression` members (and returns null for `ListExpression`,
closures, etc.). Tweaking the wording will make the test rationale more precise.
##########
src/main/java/groovy/transform/options/PropertyHandler.java:
##########
@@ -72,15 +82,18 @@ public boolean validateProperties(final
AbstractASTTransformation xform, final B
}
/**
- * Create a statement that will initialize the property including any
defensive copying. Null if no statement should be added.
+ * Create a statement that will initialize the property including any
defensive copying.
+ * Return {@code null} to indicate that no initialization statement should
be emitted
+ * for this property; this is distinct from returning an empty block and
skips the
+ * corresponding entry in the generated constructor body altogether.
*
* @param xform the transform being processed
* @param anno the '@ImmutableBase' annotation node
* @param cNode the classnode containing the property
* @param pNode the property node to initialize
- * @param namedArgMap an "args" Map if the property value should come from
a named arg map or null if not
+ * @param namedArgsMap an "args" Map if the property value should come
from a named arg map or null if not
Review Comment:
The `createPropInit` Javadoc still says `anno` is the "'@ImmutableBase'
annotation node", but this hook is used by multiple transforms (e.g.
`@TupleConstructor`, `@MapConstructor`, `@ImmutableBase`, `@RecordType`). Since
this PR is graduating `PropertyHandler` to a stable API, the parameter docs
should be accurate/generic to avoid misleading implementers.
> Graduate PropertyHandler from incubating to stable
> --------------------------------------------------
>
> Key: GROOVY-12030
> URL: https://issues.apache.org/jira/browse/GROOVY-12030
> Project: Groovy
> Issue Type: Task
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Summary
> Graduates {{groovy.transform.options.PropertyHandler}} from {{@Incubating}}
> to stable, documents the previously-implicit contract, corrects a latent bug
> in {{isValidAttribute}}, and adds a custom-handler test to lock the public
> extension contract in place.
> h3. Changes
> * *Graduation*: removed {{@Incubating}} from {{PropertyHandler}}. The
> concrete subclasses ({{DefaultPropertyHandler}},
> {{ImmutablePropertyHandler}}, {{LegacyHashMapPropertyHandler}}) and the
> activation annotation {{@PropertyOptions}} were already unmarked; the
> abstract base was the lone outlier.
> * *Contract documented*: javadoc on the base class now records the lifecycle
> ordering ({{validateAttributes}} → {{validateProperties}} →
> {{createPropInit}}/{{createPropGetter}}/{{createPropSetter}}), the public
> no-argument constructor required of custom handlers, the null-return
> semantics of {{createPropInit}}, the rejection-on-presence semantics of
> {{isValidAttribute}}, and the error behaviour of {{createPropertyHandler}}
> when a handler cannot be loaded.
> * *Parameter rename*: {{createPropInit}}'s abstract parameter renamed from
> {{namedArgMap}} to {{namedArgsMap}} to match all three concrete subclasses.
> * *Dead code*: stale commented-out call removed from
> {{DefaultPropertyHandler.validateAttributes}}.
> * *isValidAttribute correction*: switched the presence check from
> {{xform.getMemberValue(anno, memberName) != null}} to
> {{anno.getMember(memberName) != null}}.
> {{AbstractASTTransformation.getMemberValue}} is documented as returning
> {{null}} for any member that is not a {{ConstantExpression}}, so list-,
> class-, nested-annotation-, and closure-valued attributes were silently
> passing validation. The only in-tree caller ({{ImmutablePropertyHandler}}
> rejecting the boolean {{useSuper}}) is unaffected.
> * *Test*: added {{groovy/transform/options/PropertyHandlerTest}} covering
> custom-handler initialisation, attribute rejection via a list-valued
> attribute (which also locks the correction above in place), and the
> no-{{@PropertyOptions}} fallback path.
> h3. Compatibility
> * Parameter rename is source- and binary-compatible (Java does not require
> parameter-name agreement between abstract methods and their overrides).
> * The {{isValidAttribute}} correction widens detection; no in-tree consumer
> relies on the previous silent-pass behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
