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

[Archie L. Cobbs]

On Thu, 12 Jan 2023 11:09:35 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Archie L. Cobbs has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Use the more appropriate Type comparison method Types.isSameType().
>>  - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 444:
> 
>> 442:         if (referenceExpressionNode) {
>> 443: 
>> 444:             // We treat instance methods as having a "value" equal to their instance
> 
> The comment is slightly misleading - e.g. I'd suggest clarifying "having a "value" whose type is the same as that of the class in which the method is defined"

Agreed - will fix.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 454:
> 
>> 452: 
>> 453:             // If the expression type is incompatible with 'this', discard it
>> 454:             if (type != null && !this.isSubtype(this.targetClass.sym.type, type))
> 
> Instead of adding the direct reference, and then having to check if the reference needs to be removed, would it be possible not to add the reference in the first place if the types mismatch?

No because (for example) what if you cast?

The thing you're casting might be compatible, but after the cast it might become incompatible.

> This will then determine how to interpret this in the context of that method analysis - e.g. when we see a JCIdent for this, we create a direct/indirect ExprRef based on what the receiver kind was. Correct?

Yes, exactly.

When we "invoke" a method, we "preload" the set of current references that the method is going to have when it starts. That "preload" step includes any references from (a) the method receiver (non-static methods only) and (b) method parameters.

To the extent the method receiver has a direct/indirect reference, that turns into a direct/indirect `ThisRef` during method execution. You can see this happen in the very next lines after what you quoted. But of course the method invocation (the "apply") could have been applied to any arbitrary expression as the receiver.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 817:
> 
>> 815:         // Methods - the "value" of a non-static method is a reference to its instance
>> 816:         final Symbol sym = tree.sym;
>> 817:         if (sym.kind == MTH) {
> 
> This is perhaps where filtering based on the declaring class could make sense (to avoid having to filter later) ? Perhaps this could also be centralized - e.g. whenever you create an ExprRef you also pass the type for it, and if the type matches that for the current class you create it and add to the list, otherwise you skip it.

Yes but see previous comment regarding casting.

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

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

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