On Tue, 18 May 2021 11:07:18 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
> This patch allows Resolve to use more static diagnostic factories. Resolution
> errors support generation of diagnostics. This generation is very flexible
> and allows creating either a toplevel (error or warning) diagnostic, or a
> nested (fragment) diagostic. To support this, many resolution diagnostics in
> javac define some "overloads" in compiler.properties - e.g.
>
>
> # 0: message segment
> compiler.err.prob.found.req=...
> # 0: message segment
> compiler.misc.prob.found.req=...
> # 0: message segment, 1: type, 2: type
> compiler.warn.prob.found.req=...
>
>
> To support switching from one diagnostic kind to another, this patch adds a
> new method on `DiagnosticInfo`, namely `toType(DiagnosticType)`. The default
> implementation of this method will simply check that the type is identical to
> that of the current diagnostic info, and throw otherwise.
>
> This patch modifies the build-time step to construct diagnostic factories, so
> that, whenever a diagnostic overload is detected, the `toType` method is
> overridden, and the right constants/factories are returned/called depending
> on the requested diagnostic type. For instance, the factory for the
> `prob.found.req` key will look as follows in the generated code:
>
>
> /**
> * compiler.err.prob.found.req=\
> * incompatible types: {0}
> */
> public static Error ProbFoundReq(Fragment arg0) {
> return new Error("compiler", "prob.found.req", arg0) {
> public JCDiagnostic.DiagnosticInfo
> toType(JCDiagnostic.DiagnosticType kind) {
> return switch (kind) {
> case ERROR -> this;
> case WARNING -> throw new AssertionError("Unsupported
> kind: " + kind);
> case FRAGMENT -> Fragments.ProbFoundReq(arg0);
> case NOTE -> throw new AssertionError("Unsupported
> kind: " + kind);
> };
> }
> };
> }
>
>
> As you can see, the build time process correctly detects all diagnostic
> overloads and generate some code to convert one diagnostic info to another.
> Note that in this case, only two overloads are detected (`err` and `misc`),
> given that `warn` has a different type comment so not, technically speaking,
> an overload.
>
> With this support it is relatively easy to go back to Resolve and update most
> of the resolution errors to use the generated factories.
>
> The only case I have not dealt with is the "cannot.resolve" diagnostic, which
> has many variants: `{ var, method, generic method } x { location, no location
> }`. To use the factories effectively here a change in the way the diagnostic
> is structured is required, but doing so, while trivial, will cause many
> change in the golden test files, so I held off these changes for now, and
> will come back later to this.
Some general comments about the generated code. I wonder if it would make sense
to change the implementation of the toType() method which will fold common
cases in the switch expression into a default case or generate a case label
like: `case Type1, Type2 -> sameAction`. Another thing I found is that the
generated code has "mirror" code I mean we have for example:
public static final Fragment UncheckedCastToType = new
Fragment("compiler", "unchecked.cast.to.type") {
public JCDiagnostic.DiagnosticInfo
toType(JCDiagnostic.DiagnosticType kind) {
return switch (kind) {
case ERROR -> throw new AssertionError("Unsupported kind: "
+ kind);
case WARNING -> Warnings.UncheckedCastToType;
case FRAGMENT -> this;
case NOTE -> throw new AssertionError("Unsupported kind: "
+ kind);
};
}
};
and
public static final Warning UncheckedCastToType = new
Warning("compiler", "unchecked.cast.to.type") {
public JCDiagnostic.DiagnosticInfo
toType(JCDiagnostic.DiagnosticType kind) {
return switch (kind) {
case ERROR -> throw new AssertionError("Unsupported kind: "
+ kind);
case WARNING -> this;
case FRAGMENT -> Fragments.UncheckedCastToType;
case NOTE -> throw new AssertionError("Unsupported kind: "
+ kind);
};
}
};
I wonder if what we really want to model is one factory that can fold both
implementations into one. I know this is generated code which should be ready
to use, but just thinking aloud, not sure if there are some abstractions that
could be useful from the client code perspective. I wonder if we could build on
method DiagnosticInfo::of to define one stop factories. But I guess that you
already considered this option.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4089
