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

[Archie L. Cobbs]

On Fri, 6 Jan 2023 15:38:31 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>>> The associated JBS issue has been dormant for 6+ years and this is a very intrusive change affecting many, many files. Has the resurrection of this project previously been discussed somewhere?
>> 
>> Hi @dholmes-ora,
>> 
>> The work to add this warning has been guided by @briangoetz in discussions on the amber-dev mailing list. See that thread for how we came up with the current design, underlying motivation, etc.
>> 
>> Regarding intrusiveness (assuming you're referring simply to the number of files touched), as mentioned this was one of two options. The other option would be to patch to about ~30 `Java.gmk` files in `make/modules` to exclude `this-escape` from `-Xlint` during the various module builds.
>> 
>> Going this route is fine with me, but it has the downside that any new code being developed would not benefit from the new warning. This was in fact my original approach (and it was a lot easier :) but Brian rightly pointed out that adding `@SuppressWarnings` annotations was the the safer (i.e, more conservative) approach. 
>> 
>>> If you haven't done so already then you probably should socialize on compiler-dev and get agreement on the semantics and other details.
>> 
>> Hi @AlanBateman,
>> 
>> As mentioned this has been under discussion on amber-dev for a while. Happy to continue that discussion here as well.
>> 
>>> I think you will also need to separate the addition of the lint warning from the changes to the wider JDK. It might be okay to add the feature but have it disabled for the JDK build initially.
>> 
>> Sounds reasonable... so I take it you would also be in favor of patching `make/modules` instead of adding `@SuppressWarnings` annotations everywhere... is that correct?
>> 
>> If this is generally agreed as a better route then let me know and I'll update the patch.
>> 
>> Thanks for both of your comments.
>
>> Sounds reasonable... so I take it you would also be in favor of patching `make/modules` instead of adding `@SuppressWarnings` annotations everywhere... is that correct?
>> 
>> If this is generally agreed as a better route then let me know and I'll update the patch.
> 
> Yes, I think that would be better. It would remove most of the noise, 1200+ files, and 10+ mailing lists from this PR. I assume there will be at least some iteration on compiler-dev about the details and changes to javac. Once you get to the JDK changes then I suspect that some areas may want to fix issues rather than adding SW. Sadly, I see a few examples in your list where there have been attempts to avoid leaking "this" but where we backed away out of concern that 3rd party code was extending some class and overriding a method known to be invoked by the constructor. Also we have places that register themselves to cleaners. I suspect some of the suggestions to document leaking this in implNotes will need discussion too because they amount to documenting "hooks" that people will rely on, e.g. documenting in ArrayDeque that its constructor invokes addList could be read as an invite to override it.

Hi @AlanBateman,

OK that sounds like a plan. I've pushed a new commit to the `ThisEscape` branch that removes all the`@SuppressWarnings` annotations and replaces them with adjustments to build flags.

I've moved the `@SuppressWarnings` annotations onto a new branch `ThisEscapeAnnotations`. This is just for future reference in case somebody wants to add them back someday and doesn't want to start from scratch.

>  I suspect some of the suggestions to document leaking this in implNotes will need discussion too because they amount to documenting "hooks" that people will rely on, e.g. documenting in ArrayDeque that its constructor invokes addList could be read as an invite to override it.

Hmm. Hasn't that horse already left the barn? You kind of implied that when you said:

> I see a few examples in your list where there have been attempts to avoid leaking "this" but where we backed away out of concern that 3rd party code was extending some class and overriding a method known to be invoked by the constructor.

In other words, it doesn't sound like changing the behavior of these constructors is viable option at this point. And if that's the case, we might as well document and warn about the current behavior.

Of course I'd love to be wrong... in which case, we can fix these constructors.

Or, the third option - just do nothing yet. That would mean removing the warnings, which is fine.

But then the `ThisEscape.html` document is orphaned. What should we do with it? I can remove it, just leave it there, or put it somewhere else (where?).

It seems like having some documentation of the meaning of "this escape" would be helpful, because it's a subtle concept and there are multiple ways to define "escape".

Thanks.

@mcimadamore thanks for the bugs suggestion, I'll put that on the to-do list.

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

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