[
https://issues.apache.org/jira/browse/GROOVY-12015?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18081374#comment-18081374
]
ASF GitHub Bot commented on GROOVY-12015:
-----------------------------------------
Copilot commented on code in PR #2537:
URL: https://github.com/apache/groovy/pull/2537#discussion_r3252618987
##########
src/main/java/org/codehaus/groovy/transform/RecordTypeASTTransformation.java:
##########
@@ -479,8 +480,12 @@ private void createCopyWith(ClassNode cNode,
List<PropertyNode> pList) {
namedParam.addMember("required", constX(false, true));
mapParam.addAnnotation(namedParam);
}
- Statement body = returnS(ctorX(cNode.getPlainNodeReference(), args));
+ BlockStatement body = new BlockStatement();
+ body.addStatement(CopyWithUtils.nestedFlattenStmt(NAMED_ARGS));
+ body.addStatement(returnS(ctorX(cNode.getPlainNodeReference(), args)));
addGeneratedMethod(cNode, COPY_WITH, ACC_PUBLIC | ACC_FINAL,
cNode.getPlainNodeReference(), params(mapParam), ClassNode.EMPTY_ARRAY, body);
+
+ CopyWithUtils.addCopyWithBlockMethod(cNode);
}
Review Comment:
The generated `@RecordType` copyWith(Map) always returns a new instance
(constructor call) even when the map is empty/contains only unknown keys or
when all supplied values are identical to the current ones. This appears to
contradict the RecordOptions javadoc (“if the values would not change the
object, then the original object is returned”) and also prevents the “unchanged
graph -> original root” identity behavior described for nested copyWith.
Consider adding a dirty-check and returning `this` when no effective changes
occur (similar to @Immutable’s copyWith implementation).
##########
src/main/java/org/apache/groovy/transform/copywith/NestedCopyWithSupport.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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 org.apache.groovy.transform.copywith;
+
+import groovy.lang.Closure;
+import groovy.lang.GroovyRuntimeException;
+import org.apache.groovy.lang.annotation.Incubating;
+import org.codehaus.groovy.runtime.InvokerHelper;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+/**
+ * Runtime support for nested-path {@code copyWith}. Resolves dotted keys such
+ * as {@code 'address.city'} into a flat map of top-level property to its
+ * replacement value, by recursively applying {@code copyWith} to the affected
+ * nested nodes. Plain (non-dotted) keys pass through unchanged.
+ * <p>
+ * A nested node whose type does not provide {@code copyWith(Map)} fails with
+ * a clear, specific error rather than silent or partial behaviour. Identity
+ * is preserved transitively: an unchanged nested node yields its original
+ * reference, so an unchanged graph yields the original root.
+ *
+ * @since 6.0.0
+ */
+@Incubating
+public final class NestedCopyWithSupport {
+
+ private NestedCopyWithSupport() {
+ }
+
+ /**
+ * Transactional-block form. Runs {@code block} against a recording
+ * delegate that captures plain assignments, nested-path navigation, and
+ * {@code prop.modify { old -> ... }} updates, then delegates to the
+ * (nested-aware) {@code copyWith(Map)}. The block is thus pure sugar over
+ * the map form, inheriting its closed-type-domain and identity guarantees.
+ */
+ public static Object applyBlock(final Object self, final Closure<?> block)
{
+ Map<Object, Object> sink = new LinkedHashMap<>();
+ Closure<?> c = (Closure<?>) block.clone();
+ c.setResolveStrategy(Closure.DELEGATE_FIRST);
+ c.setDelegate(new CopyWithRecorder(self, "", sink));
+ c.call();
+ if (sink.isEmpty()) return self;
+ return InvokerHelper.invokeMethod(self, "copyWith", new
Object[]{sink});
+ }
+
+ @SuppressWarnings("unchecked")
+ public static Map<Object, Object> flatten(final Object self, final
Map<Object, Object> raw) {
+ if (raw == null) return new LinkedHashMap<>();
+ Map<Object, Object> flat = new LinkedHashMap<>();
+ Map<String, Map<Object, Object>> nested = new LinkedHashMap<>();
+
+ for (Map.Entry<Object, Object> e : raw.entrySet()) {
+ Object key = e.getKey();
+ String k = key == null ? null : key.toString();
+ int dot = k == null ? -1 : k.indexOf('.');
+ if (dot < 0) {
+ if (nested.containsKey(k)) {
+ throw conflict(k);
+ }
+ flat.put(key, e.getValue());
+ } else {
+ String head = k.substring(0, dot);
+ String rest = k.substring(dot + 1);
+ if (flat.containsKey(head)) {
+ throw conflict(head);
+ }
+ nested.computeIfAbsent(head, h -> new
LinkedHashMap<>()).put(rest, e.getValue());
+ }
+ }
+
+ for (Map.Entry<String, Map<Object, Object>> e : nested.entrySet()) {
+ String head = e.getKey();
+ Object current = InvokerHelper.getProperty(self, head);
+ if (current == null) {
+ throw new GroovyRuntimeException("copyWith: cannot apply a
nested update to '"
+ + head + "' because its current value is null");
+ }
+ // A nested node must itself expose copyWith(Map); fail clearly
otherwise.
+ boolean supported = !InvokerHelper.getMetaClass(current)
+ .respondsTo(current, "copyWith").isEmpty();
+ if (!supported) {
+ throw new GroovyRuntimeException("copyWith: nested update of
'" + head
+ + "' requires its type (" +
current.getClass().getName()
+ + ") to provide a copyWith(Map) method — e.g. an
@Immutable/@RecordType "
+ + "declared with copyWith=true. This type is outside
the supported "
+ + "nested-copyWith domain.");
+ }
+ Object updated = InvokerHelper.invokeMethod(current, "copyWith",
new Object[]{e.getValue()});
Review Comment:
In flatten(), the "supported" check uses MetaClass.respondsTo(current,
"copyWith") without verifying the Map-based overload. If a nested value type
defines copyWith() or copyWith(Closure) but not copyWith(Map), this check will
pass and the subsequent invokeMethod(current, "copyWith", Map) will throw a
MissingMethodException instead of the intended clear GroovyRuntimeException.
Consider checking respondsTo with a Map argument (or explicitly for a
single-arg Map signature) so the error path is deterministic and matches the
documented requirement of copyWith(Map).
> Provide a nested copyWith capability
> ------------------------------------
>
> Key: GROOVY-12015
> URL: https://issues.apache.org/jira/browse/GROOVY-12015
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h3. Summary
> Extends {{copyWith}} (generated for {{@Immutable}} and {{@RecordType}}
> classes declared with {{{}copyWith=true{}}}) with nested-path support and a
> transactional block form, for ergonomic updates of immutable object graphs.
> h3. Nested-path map form
> A {{copyWith}} key may now be a dotted path; the affected nested nodes are
> rebuilt recursively and untouched branches are reused.
> {code:groovy}
> def q = p.copyWith('address.city': 'NYC', 'address.zip': '10001')
> {code}
> * Works for arbitrarily deep paths and heterogeneous {{{}@Immutable{}}}/
> {{@RecordType}} graphs (record-in-record included).
> * Plain (non-dotted) keys behave exactly as before.
> * Identity is preserved transitively: if nothing changes, the original
> instance is returned.
> * Every node on a nested path must itself provide {{{}copyWith(Map){}}};
> otherwise a clear, specific error is raised.
> h3. Transactional block form
> A {{copyWith(Closure)}} overload is also generated:
> {code:groovy}
> def q = p.copyWith {
> name = 'Bob' // plain set
> address.city = old.address.city.reverse() // derive from the original
> fullName = old.first + ' ' + old.last // cross-field derivation
> loginCount.modify { it + 1 } // single-field shorthand
> }
> {code}
> * {{old}} resolves to the original (pre-state) object, consistent with
> {{old}} in {{{}@Ensures{}}}/{{{}@Contract{}}}, and may be used to derive any
> new
> value, including across fields.
> * {{prop.modify\{ \} }} is a shorthand for the common
> transform-this-same-field case.
> * To invoke a real same-named {{modify}} method on a value type (or simply
> for readability), use the {{old}} form: x = old.x.modify { ... \}.
> * An empty or no-op block returns the original instance.
> h3. Compatibility
> * A no-arg {{copyWith()}} is generated, returning the same instance
> (preserving the historical zero-argument behaviour).
> * Existing single-level and Map-based {{copyWith}} usage is unchanged.
> * Shared generation logic is used by both the {{@Immutable}} and
> {{@RecordType}} transforms so behaviour stays identical across the two.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
