On Thu, 12 Jan 2023 16:20:12 GMT, Archie L. Cobbs <d...@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
