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

[Maurizio Cimadamore]

On Thu, 12 Jan 2023 17:33:48 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 875:
>> 
>>> 873:         // Reference to this?
>>> 874:         if (tree.name == names._this || tree.name == names._super) {
>>> 875:             if (this.refs.contains(ThisRef.direct()))
>> 
>> This idiom occurs quite a lot. If I'm correct, this basically amounts at asking as to whether the receiver of the method we're currently evaluating is direct or not (which is an invariant, given a method body - e.g. for a given method this "fact" should stay the same). If that's the case, perhaps capturing this in a flag could be better - then you could have just have a single method e.g. `XYZRef.create(boolean direct)`, and remove the branching (here and elsewhere).
>
> The code you quoted has nothing specifically to do with method invocations. This code is simply handing the evaluation of the expressions `this` and `super`. For example, `this` could just be a parameter we're passing to another method.
> 
> When `this` or `super` expressions are evaluated, the thing left on top of the stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when the there is a direct/indirect reference of type `ThisRef` in the current reference set.
> 
> In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top of Java stack) becomes the method receiver, and when the method is "invoked" it turns back into a `ThisRef` (see earlier question).
> 
> Regarding the optimization you mention, in light of the above I'm not sure it would still work.

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.

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

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