RFR: 8359493: Refactor how aggregated mandatory warnings are handled in the compiler

[Maurizio Cimadamore]

On Mon, 23 Jun 2025 13:48:06 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java line 736:
>> 
>>> 734:     private MandatoryWarningAggregator aggregatorFor(LintCategory lc) {
>>> 735:         return switch (lc) {
>>> 736:         case PREVIEW -> aggregators.computeIfAbsent(lc, c -> new MandatoryWarningAggregator(this, Source.instance(context), c));
>> 
>> Can we save `Source` as an instance variable? I'm mildly worried about using the `context` instance field (which is used for lazy init of lint) more generally. (In fact, I was even thinking if it would make sense to have a `Supplier<Lint>` field that we create at construction, so that we don't have to save the `context` field at all.
>
>> Can we save `Source` as an instance variable? I'm mildly worried about using the `context` instance field (which is used for lazy init of lint) more generally. (In fact, I was even thinking if it would make sense to have a `Supplier<Lint>` field that we create at construction, so that we don't have to save the `context` field at all.
> 
> I guess I don't understand the issue - isn't this kind of usage the whole point of having a `Context` singleton?
> 
> We can certainly grab the reference to `Source` eagerly (which means sometimes we would be grabbing it unnecessarily - i.e., any time there are no preview warnings) but beyond that I'm not sure what additional changes would buy us.
> 
> Put another way: what are you trying to optimize for with that question? How would `Supplier<Lint>` be better than a `Context` (plus `Lint.instance()`) which is the more standard way to achieve the same thing?

What I mean is that, typically, javac components only use `context` as a constructor parameter, and they initialize all the context-dependent stuff they need from there. It is rather strange to see classes to "hold onto" their context parameter and use it later. Sometimes (as in this case) this is unavoidable (as initializing too early would lead to cycles) -- but I believe the status quo is to do this only where strictly necessary, and the `Source.instance(context)` does not feel necessary to me. (separately, each call to `instance` does a lookup on a map, so there's also an efficiency angle to try and limit this where possible).

`Supplier` is not better -- but it avoids the `context` field, which in turns avoid the "temptation" to use it when not necessary. It is not mandatory you change the code to use a supplier though, as we don't have precedents for that kind of thing -- but limiting context lookups as much as possible is defo how javac code generally works -- and the main thing I'm bringing up in this comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25810#discussion_r2161759855