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

[Maurizio Cimadamore]

On 06/09/2019 09:26, Brian Goetz wrote:
> This makes sense to me, with the proviso that “at the end” doesn’t 
> necessarily mean one place, but all exit paths?

Yes - that's what I meant.

Maurizio

>
>
>
> Sent from my iPad
>
> On Sep 6, 2019, at 4:04 AM, Maurizio Cimadamore 
> <maurizio.cimadamore at oracle.com 
> <mailto:maurizio.cimadamore at oracle.com>> 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.
>>
>> 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
>>>>
>>>>
>>>>
>>>>
>>>