<i18n dev> RFR: 8015831: Add lint check for calling overridable methods from a constructor

[Maurizio Cimadamore]

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.

I agree with @AlanBateman. When we introduced similar warnings in the past, we have enabled support in javac (with some test) in a separate PR, and then followed up with small dedicated PR for each of the various areas (enabling new warnings one by one).

See this great umbrella JBS issue (created by @asotona) which has details on all the issues that were filed when we added an extra Lint warning for lossy conversions:

https://bugs.openjdk.org/browse/JDK-8286374

They all refer to this:

https://bugs.openjdk.org/browse/JDK-8244681

Which was submitted as a separate javac-only PR:
https://github.com/openjdk/jdk/pull/8599

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

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