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.

Sign up for GitHub

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

Jump to bottom

Highlight error locations #1121

Merged
merged 1 commit into from
Aug 13, 2018
Merged

Highlight error locations #1121

merged 1 commit into from
Aug 13, 2018

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Aug 10, 2018

This is a simple implementation of an error printer that highlights the error location with tildes, for example:

File "dune", line 1, characters 14-21:
(copy_files %{read:x}/*)
              ~~~~~~~

The message is not displayed in some cases, like if the file does not exist, or if the location spans multiple lines.

@emillon emillon requested review from rgrinberg and a user August 10, 2018 10:00
@emillon emillon force-pushed the highlight-error-locations branch from 7da78b5 to 5038a42 Compare August 10, 2018 10:01
@rgrinberg
Copy link
Member

Very cool. What happens when loc spans more than one line?

@emillon
Copy link
Collaborator Author

emillon commented Aug 10, 2018

If a location spans more than one line, we're in the stop_c > String.length line case, so we don't print anything (the corresponding message looks like "line 3, characters 4-234"). We might be able to render it like this:

File "dune", line 3, characters 4-234:
3:  (something
4:      something
5:  )

But that can be improved later. Similarly, we can add colors, etc 😃 .

@rgrinberg
Copy link
Member

That would indeed be nice. I'll let you implement that at your spare time.

@rgrinberg
Copy link
Member

Btw, a bit of a nit, but I think ^^^^^^ is the more popular syntax for this purpose. See rust for example.

@emillon emillon force-pushed the highlight-error-locations branch from 5038a42 to f96966c Compare August 10, 2018 17:07
@emillon
Copy link
Collaborator Author

emillon commented Aug 10, 2018

Right, that's what elm uses as well. I updated this.

ghost
ghost suggested changes Aug 13, 2018
View reviewed changes
src/loc.ml
@@ -61,12 +61,35 @@ let of_pos (fname, lnum, cnum, enum) =
; stop = { pos with pos_cnum = enum }
}

let print ppf { start; stop } =
let file_line path n =
Io.with_file_in ~binary:false path
Copy link

Choose a reason for hiding this comment

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

This should probably be binary:true as dune files are read in binary mode. It's the lexer which accepts both kinds of line endings.

Copy link

Choose a reason for hiding this comment

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

Actually it only matters for multi-line locations, which are currently not printed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK! I'll change that when implementing multi line highlights.

@ghost
Copy link

ghost commented Aug 13, 2018

This looks nice indeed 👍

@emillon
Highlight error locations
06bc3b1 
This is a simple implementation of an error printer that highlights the
error location with squiggly lines, for example:

    File "dune", line 1, characters 14-21:
    (copy_files %{read:x}/*)
                  ^^^^^^

The message is not displayed in some cases, like if the file does not
exist, or if the location spans multiple lines.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon force-pushed the highlight-error-locations branch from f96966c to 06bc3b1 Compare August 13, 2018 12:52
@emillon emillon merged commit 7a8b86d into master Aug 13, 2018
@emillon emillon deleted the highlight-error-locations branch August 13, 2018 13:15
@emillon emillon mentioned this pull request Aug 14, 2018
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 14, 2018
@rgrinberg
[new release] dune (1.2.0)
64d5a2a 
CHANGES:

- Ignore stderr output when trying to find out the number of jobs
  available (ocaml/dune#1118, fix ocaml/dune#1116, @diml)

- Fix error message when the source directory of `copy_files` does not exist.
  (ocaml/dune#1120, fix ocaml/dune#1099, @emillon)

- Highlight error locations in error messages (ocaml/dune#1121, @emillon)

- Display actual stanza when package is ambiguous (ocaml/dune#1126, fix ocaml/dune#1123, @emillon)

- Add `dune unstable-fmt` to format `dune` files. The interface and syntax are
  still subject to change, so use with caution. (ocaml/dune#1130, fix ocaml/dune#940, @emillon)

- Improve error message for `dune utop` without a library name (ocaml/dune#1154, fix
  ocaml/dune#1149, @emillon)

- Fix parsing `ocamllex` stanza in jbuild files (ocaml/dune#1150, @rgrinberg)

- Highlight multi-line errors (ocaml/dune#1131, @anuragsoni)

- Do no try to generate shared libraries when this is not supported by
  the OS (ocaml/dune#1165, fix ocaml/dune#1051, @diml)

- Fix `Flags.write_{sexp,lines}` in configurator by avoiding the use of
  `Stdune.Path` (ocaml/dune#1175, fix ocaml/dune#1161, @rgrinberg)

- Add support for `findlib.dynload`: when linking an executable using
  `findlib.dynload`, automatically record linked in libraries and
  findlib predicates (ocaml/dune#1172, @bobot)

- Add support for promoting a selected list of files (ocaml/dune#1192, @diml)

- Add an emacs mode providing helpers to promote correction files
  (ocaml/dune#1192, @diml)

- Improve message suggesting to remove parentheses (ocaml/dune#1196, fix ocaml/dune#1173, @emillon)

- Add `(wrapped (transition "..message.."))` as an option that will generate
  wrapped modules but keep unwrapped modules with a deprecation message to
  preserve compatibility. (ocaml/dune#1188, fix ocaml/dune#985, @rgrinberg)

- Fix the flags passed to the ppx rewriter when using `staged_pps` (ocaml/dune#1218, @diml)

- Add `(env var)` to add a dependency to an environment variable.
  (ocaml/dune#1186, @emillon)

- Add a simple version of a polling mode: `dune build -w` keeps
  running and restarts the build when something change on the
  filesystem (ocaml/dune#1140, @kodek16)

- Cleanup the way we detect the library search path. We no longer call
  `opam config var lib` in the default build context (ocaml/dune#1226, @diml)

- Make test stanzas honor the -p flag. (ocaml/dune#1236, fix ocaml/dune#1231, @emillon)

- Test stanzas take an optional (action) field to customize how they run (ocaml/dune#1248,
  ocaml/dune#1195, @emillon)

- Add support for private modules via the `private_modules` field (ocaml/dune#1241, fix
  ocaml/dune#427, @rgrinberg)

- Add support for passing arguments to the OCaml compiler via a
  response file when the list of arguments is too long (ocaml/dune#1256, @diml)

- Do not print diffs by default when running inside dune (ocaml/dune#1260, @diml)

- Interpret `$ dune build dir` as building the default alias in `dir`. (ocaml/dune#1259,
  @rgrinberg)

- Make the `dynlink` library available without findlib installed (ocaml/dune#1270, fix
  ocaml/dune#1264, @rgrinberg)
@rgrinberg rgrinberg mentioned this pull request Sep 14, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emillon @rgrinberg