implementation of record constructor auto-initialization [was Re: instance initializer]

[Maurizio Cimadamore]

(moving to amber-dev, since I want to focus on implementation issues here)

Vicente,
it is true that assessing whether a field is DA at the end of the 
constructor is a complex task. That said, we have machinery to do it in 
the compiler (Flow class).

My feeling here is that, perhaps, the compiler is attempting to do 
things too early in the pipeline; if the initialization code is added 
say, at Enter/TypeEnter, there's no way for the compiler to see what the 
initialization state of the fields actually is.

I think the best approach is to wait it out - that is, send the 
constructor unmodified through Attr, and then define some special logic 
in Flow that:

* issue errors if, at the end of record constructor there is some field 
that is neither DA nor DU
* do not issue errors at the end of a record constructor for DU fields
* keep a list of the DU fields for later

Then in Lower, we can look at the list, and add the missing initializers 
for the previously discovered DU fields. So, that cover the first part 
of your concerns.

The second, and more meaty part, is that if you have stuff like:

record R(int i, int j) {
     public R {  // compact constructor
         if (i < 0) {
             this.i = -i;
         } else {
             this.j = j;
         }
     }
}

You might be induced to think that the compiler has to put an 
initializer inside the if branch, and another inside the else (one for 
this.j, the other for this.i). But this is not actually the case; in 
this example, both this.i and this.j are neither DA nor DU at the end of 
the constructor -> error.

In other words, I think the rules stated by Brian already take into 
account the fact that the constructor can be messy, and that the only 
thing that auto-initialization is allowed to do, is to add a bunch of 
initializers _at the end_ of the constructor; if that doesn't work - 
error (because that means that there were some fields initialized in 
some paths but not in others).

Makes sense?

Maurizio

On 06/09/2019 00:47, Vicente Romero wrote:
>
>
> On 9/5/19 6:14 PM, forax at univ-mlv.fr wrote:
>>
>>
>> ------------------------------------------------------------------------
>>
>>     *De: *"Brian Goetz" <brian.goetz at oracle.com>
>>     *À: *"Tagir Valeev" <amaembo at gmail.com>, "Remi Forax"
>>     <forax at univ-mlv.fr>
>>     *Cc: *"amber-spec-experts" <amber-spec-experts at openjdk.java.net>
>>     *Envoyé: *Jeudi 5 Septembre 2019 23:26:34
>>     *Objet: *Re: Draft JLS spec for records
>>
>>
>>
>>         I don't think so. A compact constructor (or require
>>         initializer, as you propose) could be not the only
>>         constructor. An instance initializer is convenient because
>>         it's added to every constructor, regardless of whether it's
>>         compact or not. So the new thing doesn't supersede the
>>         instance initializer and I see no good reason to explicitly
>>         disable it.
>>
>>
>>     Vicente offered another reason why we might want to prohibit the
>>     instance initializer, if only out of expediency; it complicates
>>     the analysis of which fields are DA on all paths through the
>>     constructor (and therefore, do not need to be automatically
>>     initialized.) If you have a record:
>>
>>         record Foo(int i) {
>>             { this.i = 0; }
>>         }
>>
>>     then the canonical constructor has to see that `i` is always
>>     initialized by the static init, and therefore should be not
>>     initialized.  Worse, if we have:
>>
>>         record Foo(int i) {
>>             { if (tuesday) this.i = 0; }
>>         }
>>
>>     then we have to issue a compilation error, since we have fields
>>     that are neither DA nor DU at the end of the initializer.
>>
>>     None of this is impossible to do, of course; it's just not clear
>>     whether it's worth it, given the limited utility of instance
>>     initializers in records (because we've already banned instance
>>     fields.)
>>
>>
>> If we want to fix that with an the analysis, it will have to work on 
>> user-written code mixed with generated code,
>> it doesn't seem to be a good idea.
>
> right I was thinking about the case:
>
> record R(int i, int j) {
>     public R {  // compact constructor
>         if (i < 0) {
>             this.i = -i;
>         } else {
>             this.j = j;
>         }
>     }
> }
>
> which doesn't have instance initializer blocks. The thing is where to 
> draw between cases where the compiler will just issue an error and 
> cases in which it will generate the missing initializations. In this 
> particular case the compiler will have to be very clever and generate:
>
> public R {  // compact constructor
>         if (i < 0) {
>             this.i = -i;
>             this.j = j;  // automatic code
>         } else {
>             this.i = i; // automatic code
>             this.j = j;
>         }
>     }
>
> we can do it, it will be complex though, but do we want to do it? 
> Where to draw the line?
>
> Thanks,
> Vicente
>>
>> so it's Ok to not support instance initializers in record.
>>
>> Rémi
>>
>>
>>
>>
>