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

Improved error reporting api (WIP) #2190

Closed
wants to merge 7 commits into from
Closed

Improved error reporting api (WIP) #2190

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2019

The current error reporting API is a bit complex, we have too many functions to report user errors and many error messages are not cut properly and contains very long lines.

This PR proposes a new simpler API that makes it easier to construct well formatted error messages.

All error reporting functions are replaced by only two: User_error.raise and Code_error.raise. At this point, this PR only partially updates Stdune and a couple of call sites in src. I'd like to get some feedback on the new API before refactoring the whole code base.

user errors

User errors are errors that users need to fix themselves in order to make progress. Since these errors are read by users, they should be simple to understand for people who are not familiar with the dune codebase.

val User_error.raise : ?loc:Loc.t -> Style.t Pp.t list -> _

This functions takes an optional location as well a list of paragraphs. Each paragraph starts on a new line and the first one is prefixed by Error: .

One noticeable point is the use of Pp.t rather Format format strings. I personally find it difficult to produce well formatted error messages using the Format or even Fmt API. In practice, grepping test/blackbox-tests/test-cases reveals that many error messages contain lines that are longer than 80 characters even though Format uses a margin of 80 characters by default, so I'm guessing that this is a general problem.

On the other hand, the Pp API makes it easier to construct arbitrarily complex messages programmatically that are well formatted.

The Style.t type indicate the that parts of the message might be styled with styles such as Error, Details, ...

code errors

Code errors are errors that are due to programming errors and should be reported upstream. Since they are aimed at Dune developers, these don't need to be human readable and can leak internal details of the Dune codebase, such as the representation of internal data structures.

val Code_error.raise : string -> (string * Dyn.t) list -> _

The API is the same as before, except that we now use Dyn.t rather than Sexp.t.

@ghost ghost requested review from emillon and rgrinberg as code owners May 23, 2019 07:44
@rgrinberg
Copy link
Member

The proposal looks good to me. I think the Sexp.t to Dyn.t change should be done in a separate PR however.

@ghost
Copy link
Author

ghost commented May 23, 2019

Seems reasonable

@aalekseyev
Copy link
Collaborator

I'm happy to start learning how to use Pp much more than to start learning Fmt. :-)

So the change seems good to me!

@ghost ghost mentioned this pull request Jun 5, 2019
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost ghost mentioned this pull request Jun 6, 2019
@mlasson mlasson mentioned this pull request Jun 25, 2019
@ghost
Copy link
Author

ghost commented Jul 3, 2019

This PR has been split in various PRs that have all been merged. The underlying implementation of a few modules could still be improved, but at least the way to report errors in now completely formalised.

@ghost ghost closed this Jul 3, 2019
This pull request was closed.
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.

3 participants