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

[Maurizio Cimadamore]

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

>>> * On the Java stack
>>>   (a) The current 'this' instance
>>>   (b) A method parameter
>>>   (c) A local variable
>>>   (d) A temporary value that is part of the current expression being evaluated
>>>   (e) The return value from a method that just returned
>>>   (f) A caught exception
>>> 
>>> * In a field of some object...
>>>   (a) A normal field
>>>   (b) An outer 'this' instance
>>>   (c) Other synthetic field (e.g., captured free variable)
>>> 
>>> * As an element in a reference array
>>> 
>>> * In native code as a native reference
>> 
>> Thanks for the classification. This is helpful.
>> 
>> I'm not sure what you mean by (1f). You mean `this` can be embedded in an exception being thrown? Is that different from (2)?
>> 
>> Also, it seems to me that (3) is a special case of (2) - in the sense that you can imagine a reference array as a big object that has many fields as there are elements - so the same reasoning applies.
>> 
>> When looking at (2d) and (2e) the fact that Javac's AST is not in SSA form means that, yes, we need to track _expressions_ not just variables (e.g. for chains of methods calls, ternary operators and such).
>
>> I'm not sure what you mean by (1f). You mean this can be embedded in an exception being thrown? Is that different from (2)?
> 
> Yes, this would be a different case from any other that you'd have to handle in the code if you wanted to deal with it. 
> 
> An example of how this could happen would be:
> 
> public class ThrownThis extends RuntimeException {
> 
>     public ThrownThis(Object obj) {
>         try {
>             this.validate(obj);
>         } catch (RuntimeException e) {
>             e.mightLeak();  // LEAK HERE
>         }
>     }
> 
>     private void validate(Object obj) {
>         if (obj.hashCode() != 123)
>             throw this;
>     }
> 
>     public void mightLeak() {
>     }
> }
> 
> Of course, that's an absurd example and the likelihood that any random piece of actually code does that is negligible.
> 
> Regardless, I did briefly consider including handling for thrown exceptions but quickly decided it couldn't possibly be worth the trouble.
> 
> As a result, if you compile that example with `-Xlint:this-escape` you don't get a warning. No major loss!
> 
>> Also, it seems to me that (3) is a special case of (2) - in the sense that you can imagine a reference array as a big object that has many fields as there are elements - so the same reasoning applies.
> 
> Yes it's effectively the same thing, but fields and array elements are accessed by different mechanisms in Java, so from the point of the analyzer they have to be handled separately for that reason, which is why I broke them out.

> Yes, this would be a different case from any other that you'd have to handle in the code if you wanted to deal with it.
> 
> An example of how this could happen would be:
> 
> ```java
> public class ThrownThis extends RuntimeException {
> 
>     public ThrownThis(Object obj) {
>         try {
>             this.validate(obj);
>         } catch (RuntimeException e) {
>             e.mightLeak();  // LEAK HERE
>         }
>     }
> 
>     private void validate(Object obj) {
>         if (obj.hashCode() != 123)
>             throw this;
>     }
> 
>     public void mightLeak() {
>     }
> }
> ```
> 
Interesting example - I thought you might have been referring to a case where the class being analyzed was itself an exception.

Question - shouldn't we conclude that `this` leak when we see `throw this` ? E.g. what if the constructor did not have a `catch` (or if the catch was of a different type) ?
> Of course, that's an absurd example and the likelihood that any random piece of actually code does that is negligible.
> 
> Regardless, I did briefly consider including handling for thrown exceptions but quickly decided it couldn't possibly be worth the trouble.
> 
> As a result, if you compile that example with `-Xlint:this-escape` you don't get a warning. No major loss!
> 
> > Also, it seems to me that (3) is a special case of (2) - in the sense that you can imagine a reference array as a big object that has many fields as there are elements - so the same reasoning applies.
> 
> Yes it's effectively the same thing, but fields and array elements are accessed by different mechanisms in Java, so from the point of the analyzer they have to be handled separately for that reason, which is why I broke them out.

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

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