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

[Archie L. Cobbs]

On Thu, 12 Jan 2023 19:24:50 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> This patch passes all tests:
>> 
>> 
>> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> index 9d35c2fbc0a..e755c54d0c8 100644
>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> @@ -451,7 +451,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>              }
>>  
>>              // If the expression type is incompatible with 'this', discard it
>> -            if (type != null && !this.isSubtype(this.targetClass.sym.type, type))
>> +            if (type != null && !this.targetClass.sym.isSubClass(type.tsym, types))
>>                  this.refs.remove(ExprRef.direct(this.depth));
>>          }
>>      }
>> @@ -672,7 +672,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>          if (explicitOuterThis != null) {
>>              this.scan(explicitOuterThis);
>>              this.refs.removeExprs(this.depth, direct -> outerRefs.add(new OuterRef(direct)));
>> -        } else if (this.types.hasOuterClass(type, this.methodClass.type)) {
>> +        } else if (type.tsym.isEnclosedBy(this.methodClass.sym)) {
>>              if (this.refs.contains(ThisRef.direct()))
>>                  outerRefs.add(OuterRef.direct());
>>              if (this.refs.contains(ThisRef.indirect()))
>> @@ -801,9 +801,8 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>          // Explicit outer 'this' reference?
>>          final Type selectedType = this.types.erasure(tree.selected.type);
>>          if (selectedType.hasTag(CLASS)) {
>> -            final Type.ClassType selectedClassType = (Type.ClassType)selectedType;
>>              if (tree.name == this.names._this &&
>> -                this.types.hasOuterClass(currentClassType, selectedClassType)) {
>> +                currentClassType.tsym.isEnclosedBy((ClassSymbol)selectedType.tsym)) {
>>                  if (this.refs.contains(OuterRef.direct()))
>>                      this.refs.add(ExprRef.direct(this.depth));
>>                  if (this.refs.contains(OuterRef.indirect()))
>> @@ -895,9 +894,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>              final MethodSymbol sym = (MethodSymbol)tree.sym;
>>  
>>              // Check for implicit 'this' reference
>> -            final Type.ClassType currentClassType = (Type.ClassType)this.methodClass.sym.type;
>> -            final Type methodOwnerType = sym.owner.type;
>> -            if (this.isSubtype(currentClassType, methodOwnerType)) {
>> +            if (methodClass.sym.isSubClass(sym.owner, types)) {
>>                  if (this.refs.contains(ThisRef.direct()))
>>                      this.refs.add(ExprRef.direct(this.depth));
>>                  if (this.refs.contains(ThisRef.indirect()))
>> @@ -906,7 +903,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>              }
>>  
>>              // Check for implicit outer 'this' reference
>> -            if (this.types.hasOuterClass(currentClassType, methodOwnerType)) {
>> +            if (methodClass.sym.isEnclosedBy((ClassSymbol)sym.owner)) {
>>                  if (this.refs.contains(OuterRef.direct()))
>>                      this.refs.add(ExprRef.direct(this.depth));
>> 
>> 
>> Btw, I believe a similar trick can be used in TreeInfo.isExplicitThisReference. If I'm correct, `hasOuterClass` should probably be removed as it duplicates already existing functionality. 
>> 
>> Since I'm bringing this up, as TreeInfo.isExplicitThisReference is only used by the new analyzer, it would be cleaner if it was defined there, at least until it's clear it might be needed by some other client.
>
> Weird. I don't get that build failure.
> 
> Neither does github... I have been relying on the github build "Actions" succeeding to determine if "the build works" and according to that it is building.
> 
> I will add the `DISABLED_WARNINGS` in any case.

Thanks for the patch!

The semantics of `hasOuterClass()` returns false if A and B are the same class, while `isEnclosedBy()` returns true if A and B are the same class.

However, I don't think it would actually matter in practice...

Regardless, I'll add the extra equality comparison and apply your patch and also the suggestions to integrate `isExplicitThisReference()` and eliminate `hasOuterClass()`.

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

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