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

Changing the display of fatal warnings #948

Merged
merged 2 commits into from Mar 8, 2017

Conversation

Projects
None yet
3 participants
@sliquister
Copy link
Contributor

commented Dec 5, 2016

This pull request changes the display of warnings that are errors, for instance from:

File "a.ml", line 1, characters 6-7:
Warning 27: unused variable x.
File "tmp.ml", line 1:
Error: Some fatal warnings were triggered (1 occurrences)

to

File "a.ml", line 1, characters 6-7:
Error: Warning 27: unused variable x.

The main motivation is that the message about fatal warnings has a location that does not respect the line directives in the source file, which is my case means random filenames show up in errors.

This change also means one can tell from the output what warnings are errors, which could be useful either for people or for parsing the output.

Finally, it makes for a shorter error message in the common case where the code is not full of warnings.

(errorf ~loc:(in_file !input_name)
"Some fatal warnings were triggered (%d occurrences)" n)

| Warnings.Errors _ -> Some non_displayed_error

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Dec 6, 2016

Contributor

This seems a bit of a hack. What about capturing Warning.Errors directly in Location.report_exception (and just do nothing)?

This comment has been minimized.

Copy link
@sliquister

sliquister Dec 7, 2016

Author Contributor

I started this way but there are two things that were a bit awkward:

  • it doesn't allow to define more exceptions that don't need to be displayed
  • Location.error_of_exn is exposed

ocamldoc can receive Warnings.Error but it uses Location.error_of_exn for instance, so it'd report some weird errors. But in fact, it doesn't even report warnings in mli because it's missing a call to Typecore.force_delayed_checks, and it doesn't stop on fatal warnings in ml because it's missing a call to Warnings.check_fatal (why is it even reporting warnings anyway?).

Looking at the uses of Location.error_of_exn, I think they are all worse than using Location.report_exception, so I guess I'll change that, and then it'd make sense to do what you say.

This comment has been minimized.

Copy link
@sliquister

sliquister Dec 7, 2016

Author Contributor

I think Ast_mapper.ext_of_exn would behave weirdly if I did as you say. It would rethrow the exception, and the exception would be caught at the further down, instead of being converted into an error and sent to the compiler as the other exceptions.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Was about the following: drop the Warnings.Error exception constructor altogether, have Warnings.check_fatal returns a boolean, and explicitly adapt all driver code to behave as they do currently (either exiting the process, or continuing in the case of the toplevel) when fatal warnings have been reported? It's a much more invasive change, but it will also paves the way to a nicer treatment of reporting multiple errors in general (e.g. adapting the type-checker to report errors without failing necessarily at the first one).

This comment has been minimized.

Copy link
@sliquister

sliquister Jan 5, 2017

Author Contributor

I'd rather have the compiler behave more like a library and less like a standalone program, so I don't really want to add calls to exit.
I don't see how the refactoring you suggest would help with reporting multiple type errors: it sounds like reporting such errors the same way as warning would work fine.

If the comparison with the empty string is what you want to remove, maybe renaming Warnings.Errors to Location.Already_displayed and changing Location.error_of_exn to have type exn -> [ Ok of error | Already_displayed ] option would work. This way Ast_mapper.ext_of_exn can serialize the Already_displayed case, and rethrow on the other side, and Location.report_exception would behave as you wrote earlier.

This comment has been minimized.

Copy link
@sliquister

sliquister Jan 9, 2017

Author Contributor

Any opinion on that last change, where I did what I described above?

; R (Location.warning_printer , warning_printer)
; R (Location.error_reporter , error_reporter )
[ R (Location.formatter_for_warnings , ppf)
; R (Location.printer , print_loc)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Dec 6, 2016

Contributor

Do we want to keep those old hooks?

This comment has been minimized.

Copy link
@sliquister

sliquister Dec 7, 2016

Author Contributor

The hooks were added in #171, so I don't think this refactoring means we can remove them.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Feb 22, 2017

Contributor

Ok.

@sliquister sliquister force-pushed the sliquister:fatal-warnings branch from b2346f4 to faf13a5 Dec 7, 2016

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

@alainfrisch I think we're waiting on your approval as to @sliquister 's latest comments.

@@ -104,9 +107,6 @@ type error =

exception Error of error

val print_error_prefix: formatter -> unit -> unit

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Jan 3, 2017

Contributor

I'm not against removing this function from the API, but is this related to the current proposal?

This comment has been minimized.

Copy link
@sliquister

sliquister Jan 4, 2017

Author Contributor

I removed the only use of this (in the expect test tool), so it seems appropriate to stop exposing it too.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

Error: Warning 27:

The multiple colons do not look super nice. What about Fatal warning 27: ..., or Error (warning 27): ....

@sliquister sliquister force-pushed the sliquister:fatal-warnings branch from faf13a5 to 735a431 Jan 4, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

Fatal warning 27: .. was the first thing I tried, but I don't really like it: the "fatal" in fatal warning and fatal error don't mean the same thing so it's awkward. If it's an error, I'd rather print "Error". Error (warning 27): sounds fine though, so I used that.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

@alainfrisch Are you happy with this patch now that the colon issue has been fixed?
@sliquister Please rebase and add a Changes entry.

@sliquister sliquister force-pushed the sliquister:fatal-warnings branch from 41337b6 to b03ba0c Feb 22, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

@mshinwell I made the changes.

I would squash the four middle commits too, but I'll wait until no one is asking for further changes.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2017

Silence from @alainfrisch taken as assent, given the nature of this patch, and previous comments.
@sliquister Please rebase again (sorry) and ping me to merge.

@sliquister sliquister force-pushed the sliquister:fatal-warnings branch from b03ba0c to d3b628c Mar 8, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

@mshinwell I rebased and squashed.

@mshinwell mshinwell merged commit 356d8a9 into ocaml:trunk Mar 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sliquister sliquister deleted the sliquister:fatal-warnings branch Sep 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.