RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v5]

[Jan Lahoda]

On Tue, 2 Apr 2024 12:50:19 GMT, ExE Boss <duke at openjdk.org> wrote:

>> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixing tests.
>
> 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);
> 	}
> 
> 	var canonicalConstructor = lookup.findConstructor(rtype, MethodType.methodType(rtype, componentTypes);
> ...

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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1549052694