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

Highlight source locations when reporting errors #214

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

Armael
Copy link
Contributor

@Armael Armael commented May 31, 2022

This PR adds support for reporting source location when displaying errors, similar to the error reports in OCaml >= 4.08. This relies on the pp_loc library.
The easy part was plugging in the library.
The harder part was to fix the locations produced by gospel. The solution was twofold:

  • modify the preprocessor to carefully preserve column alignment of the payload within (*@ *) blocs (by emitting newlines within the [@@@gospel ...] where appropriate), using line directives to make up for the extra lines introduced
  • carefully handle and "repair" the locations at the time of displaying the error, to only rely on line and column numbers, but not byte offsets from the start of the file (which happen to refer to the file after preprocessing rather than before preprocessing as one might hope).

Most of the tricky location handling logic is factored out as helpers in pp_loc, so here most of the code is about modifying the preprocessor.

The result of the new error messages can be seen in the last commit, which updates the .expected files of the testsuite.

Copy link
Contributor

@pascutto pascutto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great, thank you!
I'm surprised (in a good way) that the locations were not too broken besides the preprocessing part.
Could you also add a changelog entry under "Improved"?
You're also missing a dune build @fmt --auto-promote.
And Lexing.set_filename is not supported before 4.11 unfortunately.

@pascutto
Copy link
Contributor

pascutto commented Jun 1, 2022

I just pushed a change in the test format to allow this. Expected results are now displayed in

(* {gospel_expected|
   ...
   |gospel_expected} *)

Let me know what you think @Armael :)

@pascutto
Copy link
Contributor

pascutto commented Jun 1, 2022

It looks like pp_loc outputs a different message depending on the OCaml version (with a change in 4.11). Is that a known behaviour?
The locations seem wrong on < 4.11

@pascutto
Copy link
Contributor

pascutto commented Jun 1, 2022

I believe I found the issue.
ppxlib actually does not stabilize the OCaml parser, so it parses the code with the current OCaml's parser, then converts the AST to the stabilized version (which we use).
As a consequence, if an AST node did not previously contain a location, the translation will use a dummy (or approximate) translation in the stabilized translation.
In our case, the locations added in ocaml/ocaml@abc53d1, which you now use, were not present prior to 4.11, and therefore cause the issues in this PR.

To me that would be a sufficient argument to upgrade to 4.11 (which is already two years old) and avoid this.

@Armael
Copy link
Contributor Author

Armael commented Jun 1, 2022

ah, I see. Upgrading to 4.11 sounds good to me.

@pascutto pascutto mentioned this pull request Jun 1, 2022
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