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

[Archie L. Cobbs]

On Thu, 12 Jan 2023 18:40:38 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch:
>> 
>> 
>> diff --git a/make/test/BuildMicrobenchmark.gmk b/make/test/BuildMicrobenchmark.gmk
>> index 1c89328a388..7c3f0293edc 100644
>> --- a/make/test/BuildMicrobenchmark.gmk
>> +++ b/make/test/BuildMicrobenchmark.gmk
>> @@ -76,7 +76,7 @@ $(eval $(call SetupJavaCompilation, BUILD_INDIFY, \
>>      TARGET_RELEASE := $(TARGET_RELEASE_BOOTJDK), \
>>      SRC := $(TOPDIR)/test/jdk/java/lang/invoke, \
>>      INCLUDE_FILES := indify/Indify.java, \
>> -    DISABLED_WARNINGS := rawtypes serial options, \
>> +    DISABLED_WARNINGS := this-escape rawtypes serial options, \
>>      BIN := $(MICROBENCHMARK_TOOLS_CLASSES), \
>>      JAVAC_FLAGS := -XDstringConcat=inline -Xprefer:newer, \
>>  ))
>> @@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation, BUILD_JDK_MICROBENCHMARK, \
>>      TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
>>      SMALL_JAVA := false, \
>>      CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
>> -    DISABLED_WARNINGS := processing rawtypes cast serial preview, \
>> +    DISABLED_WARNINGS := this-escape processing rawtypes cast serial preview, \
>>      SRC := $(MICROBENCHMARK_SRC), \
>>      BIN := $(MICROBENCHMARK_CLASSES), \
>>      JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
>> 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..4e2b1e558e7 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
>> @@ -897,6 +897,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>              // Check for implicit 'this' reference
>>              final Type.ClassType currentClassType = (Type.ClassType)this.methodClass.sym.type;
>>              final Type methodOwnerType = sym.owner.type;
>> +            //if (currentClassType.tsym.isSubClass(sym.owner, types)) {
>>              if (this.isSubtype(currentClassType, methodOwnerType)) {
>>                  if (this.refs.contains(ThisRef.direct()))
>>                      this.refs.add(ExprRef.direct(this.depth));
>> @@ -906,6 +907,7 @@ class ThisEscapeAnalyzer extends TreeScanner {
>>              }
>>  
>>              // Check for implicit outer 'this' reference
>> +            // if (currentClassType.tsym.isEnclosedBy((ClassSymbol)sym.owner)) {
>>              if (this.types.hasOuterClass(currentClassType, methodOwnerType)) {
>>                  if (this.refs.contains(OuterRef.direct()))
>>                      this.refs.add(ExprRef.direct(this.depth));
>> 
>> 
>> Fixes the build failure.
>
> 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.

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

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