Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add message format cli option #3491

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

W95Psp
Copy link
Contributor

@W95Psp W95Psp commented Sep 21, 2024

This PR adds a --message_format option to the CLI that can be human or JSON, defaulting to human.

When --message_format is set to JSON, error reports and profiling messages are done in JSON.

@W95Psp W95Psp force-pushed the add-message_format-cli-option branch from 304e68c to c13b9ae Compare September 21, 2024 09:13
@mtzguido
Copy link
Member

Hi Lucas, this looks great! I'll take a deeper look soon (I'm phone-only right now) and merge it.

Wondering, is this for the Hax playground?

@W95Psp W95Psp force-pushed the add-message_format-cli-option branch from c13b9ae to d4a7b2f Compare September 24, 2024 06:05
@W95Psp
Copy link
Contributor Author

W95Psp commented Sep 24, 2024

Thanks! I've actually just forced push this, made it a little better and I added support for JSON-output for profiling-related messages.

Ah :) this is both for the playground and for something I've wanted to make for such a long time: a build system & package manager for F* 😄

@mtzguido
Copy link
Member

Hi Lucas, took another look and this seems perfect to me. (I want to add a jsonable class which would be useful here btw.)

Just one thing, I noticed this also changes the format for the output of profiling info.

$ ./bin/fstar.exe X.fst --message_format json --profile X --profile_component '*'
{"tag":"Prims","counter":{"id":"FStar.Universal.extend_tcenv","total_time":5,"running":false,"undercount":false}}
{"tag":"Prims","counter":{"id":"FStar.CheckedFiles","total_time":1,"running":false,"undercount":false}}
{"tag":"Prims","counter":{"id":"FStar.Parser.Dep.discover","total_time":0,"running":false,"undercount":false}}
{"tag":"Prims","counter":{"id":"FStar.Parser.Dep.topological_dependences_of","total_time":0,"running":false,"undercount":false}}
{"tag":"FStar.Pervasives.Native","counter":{"id":"FStar.Universal.extend_tcenv","total_time":14,"running":false,"undercount":false}}
{"tag":"FStar.Pervasives.Native","counter":{"id":"FStar.CheckedFiles","total_time":10,"running":false,"undercount":false}}
{"tag":"FStar.Pervasives","counter":{"id":"FStar.Universal.extend_tcenv","total_time":18,"running":false,"undercount":false}}
{"tag":"FStar.Pervasives","counter":{"id":"FStar.CheckedFiles","total_time":6,"running":false,"undercount":false}}
{"msg":["Expected expression of type Prims.int\ngot expression ()\nof type Prims.unit"],"level":"Error","range":{"def":{"file_name":"X.fst","start_pos":{"line":3,"col":12},"end_pos":{"line":3,"col":14}},"use":{"file_name":"X.fst","start_pos":{"line":3,"col":12},"end_pos":{"line":3,"col":14}}},"number":189,"ctx":["While typechecking the top-level declaration `let uu___0`"]}
1 error was reported (see above)

Would a field like kind in each json message be useful to distinguish what each line represents? I guess clients can look at the presence of "counter" or "level" instead. Maybe that less-structured convention is fine in the json world? : )

@W95Psp
Copy link
Contributor Author

W95Psp commented Sep 26, 2024

Ah, I wanted to make a jsonable class as well!

Same thing, I was thinking about that and was thinking having unstructured JSON data was pretty bad.
I was a bit too shy to make big changes, but I was roughly thinking about the following design:

  • introduce a FStar.Diagnostics module
  • in that module have a diagnostic type where each variant is a kind of diagnostic (profiling, error, "proof done message", etc.)
  • the variants of diagnostic should carry the data that represent the message, not the message itself: rendering of messages should be made by FStar.Diagnostics, not by anything else
  • have one function print (or something) of type diagnostics -> unit, that prints the error and takes care of the JSON/human logic

Things would be much nicer, we could move from mostly string/document based errors and messages to well structured ADT messages. What do you think?

@mtzguido
Copy link
Member

mtzguido commented Sep 26, 2024

Yeah I had in mind something very similar to that, modularizing all output into a single module that takes care of formatting, coloring, etc. But I think that's for another PR... probably pretty invasive.

I pushed a patch here to also try --message_format json in tests/error-messages. If you think this looks good, I can rebase and merge it, and then we can brainstorm about improvements.

@W95Psp
Copy link
Contributor Author

W95Psp commented Sep 26, 2024

Yes, let's do a follow up PR, but indeed, would love to see all those ad-hoc reports go away in favor of a single ADT the stores all the information.

Great, thank you, that's a great idea, that looks good :) let's merge then!

@mtzguido mtzguido force-pushed the add-message_format-cli-option branch from ff227f4 to 0434212 Compare September 26, 2024 14:35
@mtzguido mtzguido enabled auto-merge (rebase) September 26, 2024 14:37
@mtzguido mtzguido merged commit 87a9ba9 into FStarLang:master Sep 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants