On Fri, 6 Jan 2023 15:38:31 GMT, Alan Bateman <al...@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
