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

[Archie Cobbs]

On Mon, 23 Jun 2025 14:21:44 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> 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.

Thinking about this, I realized I have had this assumption in my head: "There is a certain set of types which are compiler singletons (`Resolve`, `Source`, etc.). We assume that for any `Context` instance, there is only one associated singleton of any of those types. So while it would not necessarily be correct for random objects in the compiler to hold `Context` references, it's OK for any of these singletons to hold on to `Context` references because they are all part of the same singleton blob/cluster."

By that logic, `Log` holding a `Context` reference and then using it later is OK. But I agree there's no need to gratuitously do this when it's not necessary.

However, it turns out it's necessary for both `Source` and `Lint` to avoid dependencies. E.g., for `Source` we get this error if we try to acquire it eagerly:

java.lang.AssertionError: ignored flag: --source
        at jdk.compiler.interim/com.sun.tools.javac.util.Assert.error(Assert.java:162)
        at jdk.compiler.interim/com.sun.tools.javac.util.Assert.check(Assert.java:104)
        at jdk.compiler.interim/com.sun.tools.javac.util.Options.lambda$computeIfReady$0(Options.java:296)
        at jdk.compiler.interim/com.sun.tools.javac.util.Options.notifyListeners(Options.java:265)
        at jdk.compiler.interim/com.sun.tools.javac.main.Arguments.processArgs(Arguments.java:367)
        at jdk.compiler.interim/com.sun.tools.javac.main.Arguments.init(Arguments.java:202)
        at jdk.compiler.interim/com.sun.tools.javac.main.Main.compile(Main.java:238)
        at jdk.compiler.interim/com.sun.tools.javac.main.Main.compile(Main.java:178)
        at jdk.compiler.interim/com.sun.tools.javac.main.JavacToolProvider.run(JavacToolProvider.java:54)
        at javacserver.server.Server.runCompiler(Server.java:239)
        at javacserver.server.Server.handleRequest(Server.java:208)
        at javacserver.server.Server.lambda$start$0(Server.java:172)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1095)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:619)
        at java.base/java.lang.Thread.run(Thread.java:1447)


This is because `Log` is needed so early in the startup process.

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

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