On Tue, 9 Apr 2024 15:36:27 GMT, Archie Cobbs <aco...@openjdk.org> wrote:
>> Please review several fixes and improvements to the `this-escape` lint
>> warning analyzer.
>>
>> The goal here is to apply some relatively simple logical fixes that improve
>> the precision and accuracy of the analyzer, and capture the remaining
>> low-hanging fruit so we can consider the analyzer relatively complete with
>> respect to what's feasible with its current design.
>>
>> Although the changes are small from a logical point of view, they generate a
>> fairly large patch due to impact of refactoring (sorry!). Most of the patch
>> derives from the first two changes listed below.
>>
>> The changes are summarized here:
>>
>> #### 1. Generalize how we categorize references
>>
>> The `Ref` class hierarchy models the various ways in which, at any point
>> during the execution of a constructor or some other method/constructor that
>> it invokes, there can be live references to the original object under
>> construction lying around. We then look for places where one of these
>> `Ref`'s might be passed to a subclass method. In other words, the analyzer
>> keeps track of these references and watches what happens to them as the code
>> executes so it can catch them trying to "escape".
>>
>> Previously the `Ref` categories were:
>> * `ThisRef` - The current instance of the (non-static) method or constructor
>> being analyzed
>> * `OuterRef` - The current outer instance of the (non-static) method or
>> constructor being analyzed
>> * `VarRef` - A local variable or method parameter currently in scope
>> * `ExprRef` - An object reference sitting on top of the Java execution stack
>> * `YieldRef` - The current switch expression's yield value(s)
>> * `ReturnRef` - The current method's return value(s)
>>
>> For each of those types, we further classified the "indirection" of the
>> reference, i.e., whether the reference was direct (from the thing itself) or
>> indirect (from something the thing referenced).
>>
>> The problem with that hierarchy is that we could only track outer instance
>> references that happened to be associated with the current instance. So we
>> might know that `this` had an outer instance reference, but if we said `var
>> x = this` we wouldn't know that `x` had an outer instance reference.
>>
>> In other words, we should be treating "via an outer instance" as just
>> another flavor of indirection along with "direct" and "indirect".
>>
>> As a result, with this patch the `OuterRef` class goes away and a new
>> `Indirection` enum has been created, with values `DIRECT`, `INDIRECT`, and
>> `OUTER`.
>>
>> #### 2. Track the types...
>
> Archie Cobbs has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into JDK-8317376
> - Merge branch 'master' into JDK-8317376
> - Merge branch 'master' into JDK-8317376
> - Merge branch 'master' into JDK-8317376
> - Merge branch 'master' into JDK-8317376
> - Javadoc++
> - Merge branch 'master' into JDK-8317376
> - Several improvements to the 'this' escape analyzer.
>
> - Track direct, indirect, and outer references for all Ref types.
> - Keep type information about all references to improve tracking precision.
> - Track enhanced for() invocations of iterator(), hasNext(), and next().
> - Don't report an escape of a non-public outer instances as a leak.
> - Fix omitted tracking of references from newly instantiated instances.
> - Fix omitted tracking of leaks via lambda return values.
> - Remove unneccesary suppressions of this-escape lint warning.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 1343:
> 1341: * but also NOT an enclosing outer class of 'currentClass'.
> 1342: */
> 1343: private boolean isExplicitThisReference(Types types, Type.ClassType
> currentClass, JCFieldAccess select) {
suggestion: why not moving this method to TreeInfo already in preparation for
the changes coming with flexible constructor bodies?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16208#discussion_r1567757547
