Record copy()/with()

[Remi Forax]

----- Mail original -----
> De: "Tagir Valeev" <amaembo at gmail.com>
> À: "Amber Expert Group Observers" <amber-spec-observers at openjdk.java.net>
> Cc: "amber-spec-experts" <amber-spec-experts at openjdk.java.net>
> Envoyé: Samedi 23 Mai 2020 17:30:45
> Objet: Re: Record copy()/with()

> Hello, Sergei!
> 
> I see a number of problems with your proposed approach.
> 
> 1. Naming. Java never specified any kind of derived identifiers using
> prefixes/suffixes/etc. And, in my opinion, it's for good, because such
> things make the spec dirty. You assume that every record component is
> named in plain English but Java allows any language in identifiers.
> The method name like with年齢 or withВозраст would not look right. Also,
> it would be necessary to specify how uppercasing is performed. Not
> every character valid in Java identifier has an uppercase equivalent.
> Also, what if the record component name is already uppercase? What if
> there are two record components that differ in the first letter case
> only? Should we prohibit such records? What if the record component
> name is a Turkish word that starts with lowercase 'i'? Should we apply
> tr locale for changing the case yielding grammatically correct dotted
> İ (but making the code generation locale-dependent) or should we use
> root locale always producing grammatically incorrect dot-less I?
> Unlike some other languages, Java is known for very strict spec, this
> is the strong part of Java. Having all of these corner cases specified
> makes me sad. Btw, such constructs add a pain for IDE developers as
> well. Imagine that a user starts a rename refactoring at the wither
> call.
> 
> 2. Assuming that record components could be validated in the
> constructor, not every record could be created using step-by-step
> builders. Imagine if you have a record Point(int x, int y) with a
> restriction that components must have an equal sign. You can create
> from the scratch new Point(1, 1) or new Point(-1, -1), but new
> Point(1, 1).withX(-1).withY(-1) would not work.
> 
> 3. Record components could also be normalized in the constructor. E.g.
> assume record Fraction(int numerator, int denominator) that normalizes
> the components using GCD in the constructor. Using withers would
> produce a weird result based on the previous value.

A record is transparent (same API externally and internally) so as a user i expect that if i set a record component to 3 using a constructor and then ask its value, the result will be 3,
so i don't think normalizing values of a record in the constructor is a good idea.
This issue is independent of with/copy, calling a constructor with the results of some accessors of an already constructed gcd will produce weird results. 

> 
> 4. Points 2 and 3 may lead to the conclusion that not every record
> actually needs copying. In fact, I believe, only a few of them would
> need it. Adding them automatically would pollute the API and people
> may accidentally use them. I believe, if any automatic copying
> mechanism will be added, it should be explicitly enabled for specific
> records.

with/copy calls the canonical constructor at the end, it's not something that provide a new behavior, but more a syntactic sugar you provide because updating few fields of a record declaring a dozen of components by calling the canonical constructor explicitly involve a lot of boilerplate code that may hide stupid bugs like the values of two components can be swapped because the code called the accessors in the wrong order.

> 
> With best regards,
> Tagir Valeev.

Rémi

> 
> On Sat, May 23, 2020 at 10:06 PM Sergei Egorov <bsideup at gmail.com> wrote:
>>
>> Hi Remi,
>>
>> Thanks for raising this important topic!
>>
>> I wonder if Records can provide the good old
>> withName()/withAge()/withWhatever() methods and the compiler will either
>> merge them or we let EA do the rest?
>>
>> On Sat, May 23, 2020 at 1:36 PM Remi Forax <forax at univ-mlv.fr> wrote:
>>
>> > I've spent a little time to see how we can provide a with()/copy() method,
>> > that creates a new instance from an existing record changing the value of
>> > several components in the process.
>> >
>> > It's a feature that was requested several times while i was presenting how
>> > record works and Scala, Kotlin and C# all provide an equivalent.
>> > And there is a way to add it without introducing a new syntax.
>> >
>> > Syntax in Scala or Kotlin:
>> >   val otherPerson = person.copy(name = "John", age = 17)
>> >
>> > Syntax in C#:
>> >   var otherPerson = person with { Name = "John", Age = 17 };
>> >
>> > One can note that the syntax in C# doesn't reuse the named argument syntax
>> > of C#,
>> > with the named arguments syntax, it should be
>> >   var otherPerson = person.copy(name: "John", age: 17);
>> >
>> >
>> > For Java, one solution is to re-use the same trick used to by
>> > MethodHandle.invoke*()/VarHandle.*, have a special syntax that is very
>> > close to the actual syntax so the feature is nicely integrated with the
>> > rest of the language. Here, the last time we discuss this, we stumble
>> > because unlike with MethodHandle.invoke*(), the arguments are a key/value
>> > pairs and there is no existing syntax for that currently in Java.
>> >
>> > I believe there possible trick here, use Object... as Map.of() does.
>> > the idea is to add a method 'Record with(Object... componentValuePairs)'
>> > in java.lang.Record, and ask the compiler to verify that the even arguments
>> > (0, 2, 4, etc) are constant strings
>> >
>> > Proposed syntax:
>> >  var otherPerson = person.with("name", "John", "age", 17);
>> >
>> > Then the compiler translates the method call to an invokedynamic to a
>> > bootstrap method
>> >   public static CallSite bsm(Lookup lookup, String name, MethodType type,
>> > String[] componentNames)
>> > the componentNames being "name" and "age" in the example. The methodType
>> > has the record as first parameter type and return type, the other
>> > parameters are the types of the record components corresponding to the
>> > component names.
>> >
>> > In term of separate compilation, the BSM should verify that the component
>> > names are valid record component and that the method type parameters has
>> > the same type as the corresponding record component.
>> > So adding a new record component is a compatible change, removing a record
>> > component or changing its type is not if a method call to 'with' reference
>> > it.
>> >
>> > In term of Class.getMethod()/Lookup.findVirtual(), I propose to not see
>> > the method 'with', so it's just a compiler artifact, if someone want the
>> > method 'with' at runtime, he can call the bootstrap method.
>> >
>> > regards,
>> > Rémi
>> >
>> > PS: here is the code for the bootstrap method
>> >
>> > ---
>> >   public static CallSite bsm(Lookup lookup, String name, MethodType type,
>> > String[] componentNames) {
>> >     Objects.requireNonNull(lookup);
>> >     Objects.requireNonNull(name);
>> >     if (type.parameterCount() == 0 || type.returnType() !=
>> > type.parameterType(0)) {
>> >       throw new IllegalArgumentException("invalid method type " + type);
>> >     }
>> >     if (componentNames.length != type.parameterCount() - 1) { // implicit
>> > null check
>> >       throw new IllegalArgumentException("wrong number of component names
>> > ");
>> >     }
>> >
>> >     HashMap<String, Integer> withIndexMap = new HashMap<>();
>> >     for (int i = 0; i < componentNames.length; i++) {
>> >       String componentName = Objects.requireNonNull(componentNames[i]);
>> >       Object result = withIndexMap.put(componentName, i + 1); // 'this' is
>> > at position 0
>> >       if (result != null) {
>> >         throw new IllegalArgumentException(
>> >             "component names contains twice the same name " +
>> > componentName);
>> >       }
>> >     }
>> >
>> >     Class<?> recordType = type.returnType();
>> >     RecordComponent[] components = recordType.getRecordComponents();
>> >     if (components == null) {
>> >       throw new IllegalArgumentException("the return type is not a record
>> > " + recordType.getName());
>> >     }
>> >
>> >     int length = components.length;
>> >     Class<?>[] constructorParameterTypes = new Class<?>[length];
>> >     int[] reorder = new int[length];
>> >     MethodHandle[] filters = new MethodHandle[length];
>> >     for (int i = 0; i < length; i++) {
>> >       RecordComponent component = components[i];
>> >       String componentName = component.getName();
>> >       Class<?> componentType = component.getType();
>> >       constructorParameterTypes[i] = componentType;
>> >       int withIndex = withIndexMap.getOrDefault(componentName, -1);
>> >       // a record component value either comes from the arguments or from
>> > this + accessor call
>> >       if (withIndex == -1) { // it comes from this + accessor
>> >         try {
>> >           filters[i] = lookup.unreflect(component.getAccessor());
>> >         } catch (IllegalAccessException e) {
>> >           throw (IllegalAccessError) new IllegalAccessError().initCause(e);
>> >         }
>> >         // and reorder[i] == 0
>> >       } else { // it comes from the arguments
>> >         reorder[i] = withIndex;
>> >         if (type.parameterType(withIndex) != componentType) {
>> >           throw new IncompatibleClassChangeError(
>> >               "invalid parameter type "
>> >                   + componentType
>> >                   + " at "
>> >                   + withIndex
>> >                   + " for component name "
>> >                   + componentName);
>> >         }
>> >         withIndexMap.remove(componentName); // mark that the component
>> > name has been visited
>> >         // and filter[i] == null
>> >       }
>> >     }
>> >
>> >     if (!withIndexMap.isEmpty()) { // some component names do not exist
>> >       throw new IncompatibleClassChangeError("invalid component names " +
>> > withIndexMap.keySet());
>> >     }
>> >
>> >     MethodHandle constructor;
>> >     try {
>> >       constructor = lookup.findConstructor(recordType,
>> > MethodType.methodType(void.class, constructorParameterTypes));
>> >     } catch (NoSuchMethodException e) {
>> >       throw (NoSuchMethodError) new NoSuchMethodError().initCause(e);
>> >     } catch (IllegalAccessException e) {
>> >       throw (IllegalAccessError) new IllegalAccessError().initCause(e);
>> >     }
>> >     MethodHandle filtered = MethodHandles.filterArguments(constructor, 0,
>> > filters);
>> >     MethodHandle target = MethodHandles.permuteArguments(filtered, type,
>> > reorder);
>> >     return new ConstantCallSite(target);
>> >   }
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >