On Mon, 16 Oct 2023 22:08:53 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 of all references > > By keeping track of the actual type of... Hummm... I know a few (two) places in `java.net.http` where we deliberately let `this` escape. One place is where we set up the double final link between an `HttpClientImpl` instance and its facade (set at construction time where the facade has a final field pointing at its implementation and the implementation has a final `WeakRefeference` pointing at its facade, all set up within their constructors). Another place is with the `SSLflowDelegate` where `connect`, a protected method implemented on the `SSLTube.SSLTubeFlowDelegate` subclass is called from within the super class constructor. I assume these were the reason why `this-escape` analysis had been disabled on `java.net.http`, and I expect the reason why the analysis can be reenabled by default is because of point 3 above? ------------- PR Comment: https://git.openjdk.org/jdk/pull/16208#issuecomment-1864102081
