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).
--
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]
