----- Mail original -----
> De: "Tagir Valeev" <amae...@gmail.com>
> À: "Amber Expert Group Observers" <amber-spec-observ...@openjdk.java.net>
> Cc: "amber-spec-experts" <amber-spec-experts@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 <bsid...@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 <fo...@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);
>> > }
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
