RFR: 8267312: Resolve should use generated diagnostic factories

[Vicente Romero]

On Tue, 18 May 2021 11:07:18 GMT, Maurizio Cimadamore <mcimadamore at 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