On Wed, 3 Apr 2024 07:13:13 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
>> line 1415:
>>
>>> 1413:
>>> canonicalConstructorTypes,
>>> 1414: List.nil());
>>> 1415: createNew.constructor = init;
>>
>> Maybe instead of hardcoding the constructor that is canonical at compile
>> time (in case new components get added), it might be better to go through
>> some sort of `indy` callsite like (at least when not in the same compilation
>> unit):
>>
>> DynamicCallSiteDesc[java.lang.runtime.RecordSupport::derivedConstructor(modified
>> component names...):(T extends Record, Class<?>... /* modified component
>> types */)T]
>>
>>
>> with the `derivedConstructor` bootstrap method:
>>
>> public static MethodHandle derivedConstructor(MethodHandles.Lookup lookup,
>> String unused, MethodType type, String... modifiedComponents) throws
>> ReflectiveOperationException {
>> requireNonNull(lookup);
>> requireNonNull(type);
>> // implicit null-check:
>> List<String> modifiedComponentNames = List.of(modifiedComponents);
>>
>> Class<?> rtype = type.returnType();
>> if (
>> !rtype.isRecord()
>> || type.parameterCount() != modifiedComponents.length + 1
>> || type.parameterType(0) != rtype
>> ) {
>> throw new IllegalArgumentException("...");
>> }
>>
>> Set<String> remainingComponentNames = new
>> HashSet(modifiedComponentNames);
>> if (remainingComponentNames.size() != modifiedComponentNames.size()) {
>> throw new IllegalArgumentException("Duplicate component names
>> in modifiedComponents");
>> }
>>
>> RecordComponent[] recordComponents = rtype.getRecordComponents();
>>
>> var componentTypes = new Class<?>[recordComponents.length];
>> var filters = new MethodHandle[recordComponents.length];
>> var reorder = new int[recordComponents.length];
>>
>> for (int i = 0, j = 1; i < recordComponents.length; i++) {
>> var component = recordComponents[i];
>> componentTypes[i] = component.getType();
>>
>> var getter = lookup.findVirtual(rtype, component.getName(),
>> MethodType.methodType(component.getType()));
>> if (modifiedComponentNames.contains(component.getName())) {
>> remainingComponentNames.remove(component.getName());
>> filters[i] = null;
>> reorder[i] = j++;
>> } else {
>> filters[i] = getter;
>> reorder[i] = 0;
>> }
>> }
>>
>> if (!remainingComponentNames.isEmpty()) {
>> throw new IllegalArgumentException("Components " +
>> remainingComponentNames + " are not present in the record " + rtype);...
>
> While this is very smart, I would prefer not to do that. JLS 13.4.27 talks
> about binary compatibility for record, and warns that adding or removing
> components may break record usages. So I don't think we need to provide too
> much support for adding or removing record components.
>
> Overall, I believe the current code adheres to what the JLS draft says. If
> the author of the record wants to add or remove components, they can make it
> so that the existing compiled code still works (by adding constructor
> overloads and keeping the accessors for removed component). It is better,
> IMO, to keep the behavior predictable, and leave adjustments after record
> component modifications up to the record author, than to introduce some magic.
This was once again asked about on amber‑dev:
- https://mail.openjdk.org/pipermail/amber-dev/2024-April/008748.html
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1583084422
