RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]

[Maurizio Cimadamore]

On Thu, 12 Jan 2023 21:28:12 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> My point is about who puts ThisRef in the set to begin with. It seems to me that ThisRef is put there at the start of a method analysis. After which, there's several code points where we say "if there's a direct ThisRef in the set, do this, otherwise, if there's an indirect ThisRef in the set, do that". But the ThisRef (whether indirect or direct) seems set once and for all (when we start the analysis, and then inside visitApply). 
>> 
>> There is also this bit in `visitReference`:
>> 
>> 
>> case SUPER:
>>             if (this.refs.contains(ThisRef.direct()))
>>                 receiverRefs.add(ThisRef.direct());
>>             if (this.refs.contains(ThisRef.indirect()))
>>                 receiverRefs.add(ThisRef.indirect());
>>             break;
>> 
>> 
>> But this doesn't change what I'm saying - there seems to be a general property when we execute this analysis which says whether the current execution context has a direct `this` or not. This seems to me better captured with a boolean, which is then fed back to all the other various downstream ref creation.
>> 
>> The main observation, at least for me, is that the code unifies everything under refs, when in reality there are different aspects:
>> 
>> * the set of variables that can point to this, whether directly or indirectly (this is truly a set)
>> * whether the current context has a direct or indirect this (this seems more a flag to me)
>> * whether the expression on top of stack is direct/indirect this reference or not (again, 3 possible values here) - but granted, there `depth` here to take into account, so you probably end up with a set (given that we don't want to model a scope with its own set)
>> 
>> When reading the code, seeing set expression like containment checks or removals for things that doesn't seem to be truly sets (unless I'm missing something) was genuinely confusing me.
>
> I get what you're saying - it seems silly to model what is essentially a fixed, boolean property with the membership of a singleton in a set field, rather than with a simple boolean field.
> 
> There is a conceptual trade-off though... a lot of the code relates to converting `Ref`'s from one type to another. For example, as pointed out above, a method invocation might convert a `ExprRef` to a `ThisRef`, then to a `ReturnRef`'s, etc. Having these things all be considered part of the same family helps conceptually. The fact that a direct `ThisRef` is a singleton is just a coincidence in this way of looking at things.
> 
> The benefit is the simplicity of being able to think of the data model as "just a set of references".
> 
> For example, methods like `RefSet.replaceExprs()` become less elegant (or basically impossible) if there have to be special cases for each type of reference, whereas currently we can do clean stuff like this:
> 
> 
>     @Override
>     public void visitReturn(JCReturn tree) {
>         scan(tree.expr);
>         refs.replaceExprs(depth, ReturnRef::new);
>     }
> 
> 
> But I'm also realizing now that several places can be cleaned up by taking this event further. E.g., we should replace this code:
> 
>             if (refs.contains(ThisRef.direct()))
>                 outerRefs.add(OuterRef.direct());
>             if (refs.contains(ThisRef.indirect()))
>                 outerRefs.add(OuterRef.indirect());
> 
> with something like this:
> 
>             refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> 
> 
> I will go through and refactor to do that and clean things up a little more.
> 
>> When reading the code, seeing set expression like containment checks or removals for things that doesn't seem to be truly sets (unless I'm missing something) was genuinely confusing me.
> 
> Probably my fault for not providing better documentation of the overall "set of references" conceptual approach. FWIW I added a little bit more in f83a9cf0.

> ```java
> ```java
>  if (refs.contains(ThisRef.direct()))
>                 outerRefs.add(OuterRef.direct());
>             if (refs.contains(ThisRef.indirect()))
>                 outerRefs.add(OuterRef.indirect());
> ```
> 
> 
>     
>       
>     
> 
>       
>     
> 
>     
>   
> with something like this:
> ```java
>             refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> ```
> ```

This sounds like a good idea, that idiom is quite widespread, so it should help avoiding repetition.

-------------

PR: https://git.openjdk.org/jdk/pull/11874