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

[Maurizio Cimadamore]

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

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 218:
>> 
>>> 216:         new TreeScanner() {
>>> 217: 
>>> 218:             private Lint lint = ThisEscapeAnalyzer.this.lint;
>> 
>> On a first look I'm not sure about the granularity of suppression here. I believe that suppressing at the class level, or at the constructor level is enough. Allowing to further annotate var declarations and non-constructor methods, while doable, might actually be counter productive - in the sense that the annotation does not occur in the constructor (where you'd want to see it) but in some other method. I think the fact that a constructor is escaping (willingly) should be a visible thing. Something to consider.
>
> Two things...
> 
> We need to support annotations on field declarations because their initializers are effectively mini-constructors. But we don't need it on local variables, parameters, etc. - too confusing.
> 
> For annotations on methods, yes it's debatable. It can be handy if you have e.g. an `init()` method that all of your constructors invoke. However, I agree it's also confusing, so I will remove.
> 
> But we should keep the notion that if a constructor invokes `this()`, and the invoked constructor has annotations suppressed, then we skip over the constructor invocation.
> 
> I will make these updates.

Yep - I guess there's a general theme of "where do we want the warnings to be reported". My general feeling is that reporting a warning on the constructor might be enough - but I see that you try to generate a warning in the exact spot where the leakage happens - which I'm not sure if it's ok, or too clever.

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 270:
>> 
>>> 268:                     final boolean analyzable = this.currentClassIsExternallyExtendable() &&
>>> 269:                         TreeInfo.isConstructor(tree) &&
>>> 270:                         !tree.sym.isPrivate() &&
>> 
>> Why aren't private constructors analyzed? If we have a class with a private constructor and public static factory invoking said constructor, and the constructor makes `this` escape, isn't that an issue we should detect?
>
>> If we have a class with a private constructor and public static factory invoking said constructor, and the constructor makes this escape, isn't that an issue we should detect?
> 
> A static factory method will not create a subclassed instance, so there's no 'this' escape problem.
> 
> But I admit I completely missed factory methods as a potential thing to worry about.
> 
> Is it possible for a leak to be missed due to the use of a factory method?
> 
> Hmm. I can't immediately think of how, but if you can come up with an example please share.

I guess what I'm thinking about:

class Foo {
    private Foo() {
        m(this);
    }
    
    public void m() { ... } // overridable
    
    static Foo makeFoo() { return new Foo(); }
}

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 294:
>> 
>>> 292:                   !(this.currentClass.sym.isSealed() && this.currentClass.permitting.isEmpty()) &&
>>> 293:                   !(this.currentClass.sym.owner.kind == MTH) &&
>>> 294:                   !this.privateOuter;
>> 
>> Here, note that is the owner of the current class symbol is a method, that covers anonymous classes too, which is a part of `privateOuter`. So the only think we need to check here is whether "currentClass" is private, which is a simple predicate. No need to carry `privateOuter` I believe
>
> Unless you explicitly declare a nested class `private`, it won't have the `ACC_PRIVATE` flag, even though it is "implicitly private" because it has a `private` enclosing class.
> 
> Example:
> 
> $ cat PrivateOuter.java 
> public class PrivateOuter {
>     private static class Inner1 {
>         static class Inner2 {
>         }
>     }
> }
> $ javap -v PrivateOuter$Inner1$Inner2
> Classfile /Users/archie/proj/jdk/flex-test/classes/PrivateOuter$Inner1$Inner2.class
>   Last modified Jan 12, 2023; size 408 bytes
>   SHA-256 checksum 51ba6d39a5e66df2a078761d6424acbea7a8e32b8451f6ca7d2af49889673b2c
>   Compiled from "PrivateOuter.java"
> class PrivateOuter$Inner1$Inner2
>   minor version: 0
>   major version: 63
>   flags: (0x0020) ACC_SUPER
>   this_class: #7                          // PrivateOuter$Inner1$Inner2
>   super_class: #2                         // java/lang/Object
>   ...
> 
> 
> So we have to keep track of this "implicit privateness" manually, which is what the `privateOuter` flag is for.

D'oh - missed the `|=` - so this keeps updating...

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

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