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

[Vicente Romero]

On 9/6/19 4:04 AM, Maurizio Cimadamore wrote:
>
> (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.
>

yes I agree with this, it is in line with one of the options I was 
considering to implement this

> 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.
>

yes I think that issuing an error if the field is neither DA nor DU is 
the part I was missing. Having that rule makes the implementation 
simpler as there is no need to inline compiler generated code with user code

> 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?
>

yep, thanks

> Maurizio
>

Vicente

> 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
>>>
>>>
>>>
>>>
>>