Structured diagnostics for the compiler - #13766
Conversation
|
Note that I have tested some quick prototypes for deserializers and a bridge between the proposed compiler diagnostic and the grace library at https://github.com/Octachron/sdiag-prototypes . |
Its lovely to see grace being used here :) I look forward to playing with this |
8edb2d8 to
8e07bd5
Compare
a3ede5d to
355143d
Compare
|
I have pushed a commit that hide the change of API to the toplevel library beyond a compatibility layer. |
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
I did a first round of looking at the code, rather shallow as a review.
- I did not look at the manual yet, as I read in commit order; this was a mistake and I'll start there on my next round.
- I did not look at the new modules (diagnostics, diagnostics_history) in depth, only quickly at the interface (assessment: too hard to follow for me, I need to read the manual first). It's a lot of non-critical code and I plan to declare that it is reasonable and go by the tests mostly, instead of trying to review everything line-by-line.
- I looked at the code that adapts the compiler codebase. This is as I expected, invasive but reasonable.
|
|
||
| | List: 'a typ -> 'a list typ (** Combinators *) | ||
| | Pair: 'a typ * 'b typ -> ('a * 'b) typ | ||
| (** Specialize (2,3,4)-tuples to avoid defining tuples as heterogeneous |
There was a problem hiding this comment.
Nitpick: clearly(?!) this should be {2,3,4}-tuples rather than (2,3,4)-tuples.
| in | ||
| fprintf ppf | ||
| "@.Hint: @[\ | ||
| fprintf ppf |
There was a problem hiding this comment.
the extra spacing of fprintf looks unintentional.
|
|
||
| let print log = | ||
| log_variables log; | ||
| Log.flush log |
There was a problem hiding this comment.
I tested the new -config outputs on my machine and I observe a bug:
ocamlc -configworks exactly as beforeocamlc -log-format {json,sexp} -configwork as expected (nice!)
(nitpick: thesexpformat will sometimes fit several fields on the same line, I would rather have a vertical printing with at most one field per line)ocamlc -log-format stdout -configprints only the values and not the keys/variables, so that output is incorrect
There was a problem hiding this comment.
@Octachron points out that in fact stdout is not supposed to be the same as not setting anything, currently there is stdout and stdout-with-keys and the latter is not exposed. It would be nice if both options were exposed, and maybe they could be named stdout-light and stdout-full, or maybe stdout-values and just stdout.
| (* the GNU Lesser General Public License version 2.1, with the *) | ||
| (* special exception on linking described in the file LICENSE. *) | ||
| (* *) | ||
| (**************************************************************************) |
There was a problem hiding this comment.
The .mli would be an okay place to include some documentation of how to use diaginfo.
Help output
The documentation of -schema-format is weird.
-schema-format {json|adt}
<name> print the schema <name> in <name> format
I don't understand what it means. (Does this command expect two parameters, or just one that is either json or adt? I assume the latter.)
How to get a list of schemas?
On a first try using the tool, I failed to understand how to get a list of schemas. I think that there should be an explicit option, for example --list, to list all schemas. When I printed the --help output to write the comment about I noticed the part about *, but this does not seem to work easily on my machine:
$ ./runtime/ocamlrun ./tools/ocamldiaginfo -schema *
Unknown schema name: aclocal.m4
$ ./runtime/ocamlrun ./tools/ocamldiaginfo -schema "*"
The first comes from the fact that my shell interpolates the *, and the second produces empty output.
History output
A trailing newline seems to be missing.
$ ./runtime/ocamlrun ./tools/ocamldiaginfo -history | tail -n 5
New label backtrace, ?l structured_text
New label compiler, ?compiler
New label errors, ?l l structured_text
New label trace, ?l l structured_text
Seal[gasche@framawork github-trunk (structured_diagnostics_full)]$
| Fun.protect (fun () -> f log x) | ||
| ~finally:(fun () -> Log.flush log) | ||
|
|
||
| let dir_load = with_log V2.dir_load |
There was a problem hiding this comment.
The indentation here is confusing
b0ea420 to
31e8135
Compare
862634e to
0487e62
Compare
OlivierNicole
left a comment
OlivierNicole
left a comment
There was a problem hiding this comment.
Many of these comments are minor, because I didn’t find cause for complaining. It’s a beautiful machinery, with good tests and partly self-documenting (and the manual page is good, too).
Similarly to @gasche, I review the implementation of the new modules rather quickly, trusting the code to be overall good and the tests to catch mistakes. I spent more time on the interfaces and the changes into existing compiler code.
|
|
||
| type !'id log | ||
| type 'id t = 'id log | ||
| (** A log for the structured diagnostic with tag ['id]. *) |
There was a problem hiding this comment.
You use several times this pattern of declaring a type with a name, and then t as an alias of it. What is the gain compared to simply exporting t?
There was a problem hiding this comment.
This is in an old habits of mine to have a meaningful type name even in a context where the module has been opened. I will probably simplify remove the long name.
| log_if Compiler_diagnostic.Debug.parsetree Printast.top_phrase phrase; | ||
| log_if Compiler_diagnostic.Debug.source Pprintast.top_phrase phrase; | ||
| Toploop.execute_phrase true log phrase | ||
| Toploop.V2.execute_phrase true log phrase |
There was a problem hiding this comment.
Why was it necessary to keep Toploop backward compatible? Having these V2 around seems like a heavy price to pay.
There was a problem hiding this comment.
Toploop is one of the more user-facing of compiler-libs because it is the entry point for REPL customization and can be used in the REPL itself to programmatically execute phrase. If possible I would rather smooth the transition on those functions for a time. However, I am thinking than rather using V2 as a module name, we could use a meaningful name maybe Eval or Run?
|
|
||
| let mk_log_format f = | ||
| "-log-format", Arg.Symbol (["stdout"; "sexp"; "json"], f), | ||
| let symbols = ["stdout-full"; "stdout-light"; "sexp"; "json"] in |
There was a problem hiding this comment.
Same comment as in an earlier version: fmt-full, fmt-light would avoid the risk of confusion with the stdout, stderr terminology.
| flag | ||
| - [ocamldiaginfo -list] prints all known schema | ||
| *) | ||
|
|
There was a problem hiding this comment.
Shouldn’t these go into the .mli? It’s currently empty.
| (** dump content on log if the field was enabled *) | ||
| val dump_on_log: | ||
| 'id Log.t -> (string, 'id) Log.field -> | ||
| 'id Log.t -> (string, 'id, [`opt]) Log.field -> |
There was a problem hiding this comment.
I had the notion that non-capitalized polymorphic variants constructors were frowned upon, isn’t it the case?
4cecf90 to
ee37d75
Compare
4 participants
See also ocaml/RFCs#45 and https://icfp24.sigplan.org/details/ocaml-2024-papers/16/Structured-diagnostics-for-the-OCaml-compiler for previous discussions .
A limitation of the current compiler diagnostics (error messages, warnings, time profile information and other compiler developer debugging output) is that it cannot be parsed easily and reliably by developer tools. This creates an impedance mismatch between the compiler and those tools that tend to resort to trying to parse manually compiler error message.
This PR proposes to fix this problem by adding to the compiler a machinery for building structured compiler diagnostics before displaying them in various formats.
This PR is probably best reviewed commit by commit:
to export json-schema and adds the corresponding tests in the testsuite