Insert a blank line between consecutive compiler messages by gasche · Pull Request #12024 · ocaml/ocaml

gasche · 2023-02-21T22:09:54Z

This PR is inspired by user feedback from @mimoo on compiler error messages which originated in #11539.

Currently OCaml tools will include several messages concatenated together -- several warnings, possibly followed by one error -- and it may not be obvious especially for new users to visually split the output in several reports.

This PR introduces a blank line between consecutive reports.

(cc @Octachron)

Batch compiler example

Before:

File "b_bad.ml", lines 13-14, characters 29-28:
13 | .............................function
14 |     A.X s -> print_endline s
Error (warning 8 [partial-match]): this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
Y
File "b_bad.ml", line 18, characters 11-14:
18 | let () = f A.y
                ^^^
Error: Unbound value A.y

After:

File "b_bad.ml", lines 13-14, characters 29-28:
13 | .............................function
14 |     A.X s -> print_endline s
Error (warning 8 [partial-match]): this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
Y

File "b_bad.ml", line 18, characters 11-14:
18 | let () = f A.y
                ^^^
Error: Unbound value A.y

Toplevel example

Before:

# type _ t = Int : int -> int t | Bool : bool -> bool t;;
type _ t = Int : int -> int t | Bool : bool -> bool t
# let test : int t -> int = function Int n -> n | _ -> 0;;
Warning 4 [fragile-match]: this pattern-matching is fragile.
It will remain exhaustive when constructors are added to type t.
Warning 56 [unreachable-case]: this match case is unreachable.
Consider replacing it with a refutation case '<pat> -> .'
val test : int t -> int = <fun>

After:

# type _ t = Int : int -> int t | Bool : bool -> bool t;;
type _ t = Int : int -> int t | Bool : bool -> bool t
# let test : int t -> int = function Int n -> n | _ -> 0;;
Warning 4 [fragile-match]: this pattern-matching is fragile.
It will remain exhaustive when constructors are added to type t.

Warning 56 [unreachable-case]: this match case is unreachable.
Consider replacing it with a refutation case '<pat> -> .'

val test : int t -> int = <fun>

Implementation

Most of the logic is hidden in Location, but unfortunately there are other places in the compiler that print user messages than Location.print_report, so we need to break the abstraction a bit and expose a function "I'm about to print a new message" to be called by other modules. Not super nice, but not too bad either -- and very short.

Note

In the original report, the issue was at the build-system level (the build system would concatenate several errors coming from different files); I reported this issue on dune in ocaml/dune#6158 , and this is getting fixed by @esope in ocaml/dune#6823. The present PR is about messages concatenated by the compiler itself.

parsing/location.ml

Octachron · 2023-02-22T13:47:44Z

The code change looks fine to me: it seems fine to have a function that is used to mark the start of separate user messages. I expect that if we move to a structured log for the compiler functions those function calls will be naturally subsumed by the more general API.

gasche · 2023-02-22T13:50:22Z

This reads like an approving review, without the approval stamp. What's your thought on what this PR needs next?

Octachron · 2023-02-22T13:55:07Z

I am reading the testsuite changes before giving my approval.

Octachron · 2023-02-22T14:05:57Z

testsuite/tests/warnings/deprecated_module_use.compilers.reference

  As you could guess, Deprecated_module is deprecated.
  Please use something else!

+


The second blank line here comes for the deprecation message, which suggest that this PR might slightly affect the design of deprecation message.

Octachron · 2023-02-22T14:08:43Z

testsuite/tests/warnings/deprecated_warning_specs.ml

 Use the equivalent signed form: -f-r-a-g-i-l-e-m-a-t-h.
 Hint: Enabling or disabling a warning by its mnemonic name requires a + or - prefix.
 Hint: Did you make a spelling mistake when using a mnemonic name?
+


This newline is from an empty toplevel phrase.

Ah, I guess my move to a signature option should be changed back into "is the empty signature?" to account for other ways the signature could be empty. I will do this.

I fixed this (now the toplevel-specific commit has a much shorter diff) and rebased. Thanks for the eagle eyes!

Octachron · 2023-02-22T14:49:04Z

The regexp of ocamltex's catch_warning needs to be updated to be able to ignore blank lines before the warning.

gasche · 2023-02-22T15:11:11Z

The regexp of ocamltex's catch_warning needs to be updated to be able to ignore blank lines before the warning.

Urgh, would you maybe be willing to do this part yourself? (You should have push rights on my fork.)

Octachron · 2023-02-22T15:37:56Z

The fix for ocamltex is available at Octachron/ocaml@5b8852a16979af60 .

gasche · 2023-02-22T15:39:52Z

Thanks! I added it to the PR.

gasche · 2023-02-22T21:27:02Z

There were some bits of testsuite changes left to promote, I did this and kept the fixup commits around for now for easier reviewing.

Octachron

The testsuite is passing and adding some negative spaces to the compiler messages is a good idea.

mimoo reviewed Feb 22, 2023

View reviewed changes

parsing/location.ml Outdated Show resolved Hide resolved

gasche mentioned this pull request Feb 22, 2023

Add a new line at the end of error reports (#6823 ocaml/dune#6823

Merged

gasche force-pushed the blank-line-between-messages branch 3 times, most recently from cd16d3d to 94836c9 Compare February 22, 2023 13:34

gasche force-pushed the blank-line-between-messages branch from ab541a5 to 7a31d6d Compare February 22, 2023 13:53

Octachron reviewed Feb 22, 2023

View reviewed changes

gasche force-pushed the blank-line-between-messages branch from 7a31d6d to b847d4b Compare February 22, 2023 14:58

gasche force-pushed the blank-line-between-messages branch from e2b505b to 784f86a Compare February 22, 2023 21:26

Octachron approved these changes Feb 23, 2023

View reviewed changes

gasche and others added 6 commits February 23, 2023 22:20

print a blank line between consecutive warnings/errors

165b179

update testsuite

db91381

toplevel: also separate the toplevel phrase with a blank line

fd525c8

update testsuite (2)

b435777

Changes

bc36c14

ocamltex: update regexp and reset locations

fe953a7

gasche force-pushed the blank-line-between-messages branch from 784f86a to fe953a7 Compare February 23, 2023 21:20

gasche merged commit d71ea3d into ocaml:trunk Feb 24, 2023

Octachron added a commit that referenced this pull request Mar 24, 2023

Fix no-float-array test after #12024

a42f608

dra27 mentioned this pull request Jun 9, 2023

Print end line and valid end character for multi-line locations in dumps #12297

Open

ElectreAAS mentioned this pull request Feb 9, 2024

Tracking: Upgrading the OCaml version we use in tests to 5.1.1 ocaml/dune#9954

Closed

18 tasks

		As you could guess, Deprecated_module is deprecated.
		Please use something else!

Conversation

gasche commented Feb 21, 2023

Batch compiler example

Toplevel example

Implementation

Note

Uh oh!

Uh oh!

Octachron commented Feb 22, 2023

Uh oh!

gasche commented Feb 22, 2023

Uh oh!

Octachron commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Octachron Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

Octachron Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

gasche Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

gasche Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

Octachron commented Feb 22, 2023

Uh oh!

gasche commented Feb 22, 2023

Uh oh!

Octachron commented Feb 22, 2023

Uh oh!

gasche commented Feb 22, 2023

Uh oh!

gasche commented Feb 22, 2023

Uh oh!

Octachron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Octachron commented Feb 22, 2023 •

edited

Loading