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

Fix error submessages in the toplevel: do not display dummy locations #8954

Merged
merged 2 commits into from Sep 20, 2019

Conversation

@Armael
Copy link
Member

Armael commented Sep 19, 2019

When an error sub-message is equipped with a ghost location, the error printing code should not print the location. This is done properly by the "batch mode" printer (used by ocamlc and the toplevel when TERM=dumb), but not by the printer used by the toplevel in presence of an ANSI-capable terminal.

This PR fixes the toplevel printer for ANSI terminals accordingly, and closes #8953.

I wanted to add a test, but it turns out that it is not possible using our current testing pipeline. Indeed, both inline tests and "toplevel" tests use the "batch mode" printer. To exercise the other printer, one would need to execute the tests while simulating an ANSI-capable terminal; this is not done currently anywhere as far as I'm aware.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 19, 2019

I tried to look at the Location code and convince myself that there are no other bugs of this kind, that printing is only made on non-ghost locations. I found that the implementation is in fact not convincing at all. In particular, I see no such protection in the case of the pp_loc and pp_main_loc subfunctions of batch_mode_printer, and as far as I can tell Location.print_loc{,s} are unprotected, delaying this check to their users.

It looks like the following functions are worth reviewing, to think about their intended behavior on ghost locations:

  • Location.print_loc{,s}
  • all print_loc users in Location
  • Includemod.show_loc
  • Printtyp.pp_explanation
  • typing/Stypes.print_location (which we may not care about anymore?)
  • uses of Location.print_loc in Typemod.

(Fun story time! Includemod.show_loc has its own implementation of detecting dummy/ghost/uninteresting locations.)

Also, tools/dumpobj has its own location-printing logic, but this is probably fine

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 19, 2019

I reviewed the patch and obviously it is correct, but I would prefer if we could be diligent and think about the other printing-location places before closing the issue.

@Armael

This comment has been minimized.

Copy link
Member Author

Armael commented Sep 19, 2019

I remember finding out that there are indeed several places in the code where dummy and/or ghost locations leak out and end up being printed, and that cleaning up is not always easy (I think there is at least an instance of the typechecker emitting meaningful-but-ghost locations).

My perception of the current API provided by Location is the following:

  • whatever broken locations is given to them, the printers (of type report_printer) should behave properly (this property is hopefully restored by the current PR);
  • when implementing such a printer, one should however take care of not printing broken locations (consequently, I consider it OK for pp_loc and pp_main_loc to not be protected against dummy locs, as they are part of the implementation of a printer)
  • in general, it is not possible/too late to protect against dummy locations in print_loc or pp_loc: when you are printing a location, it is typically surrounded by some text specific to the overall message (e.g. a box, spacing, a colon afterwards, etc.). If the location turns out to be dummy, you also want to disable the surrounding text, otherwise you end up with a broken message.
  • if you use Location.print_loc, you're on your own: the "safe" API is to emit an error report, that will get nicely printed by the current report_printer. Ideally, Location.print_loc should not be exported, and other parts of the compiler should not call it to hand craft their error messages (but I know that it is not the case currently).

In short:

  • the safe API is to emit error reports (of type Location.error)
  • using Location.print_loc is unsafe
  • the implementation of the safe API (values of type printer_report) has to use unsafe features, and should be reviewed carefully (e.g. the implementation of batch_mode_printer and terminfo_toplevel_printer).

With that in mind, I believe that there are indeed issues with how other parts of the compiler use the "unsafe" API, but that these are mostly orthogonal to the issue here (which is related to the implementation of the "safe" API).

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 19, 2019

when implementing such a printer, one should however take care of not printing broken locations (consequently, I consider it OK for pp_loc and pp_main_loc to not be protected against dummy locs, as they are part of the implementation of a printer)

I don't understand the logic in this paragraph. If those functions are part of the implementation of a printer, they should be careful about ghost locations, shouldn't they?

I don't have time to think deeply about this, and it looks like you have, somewhat -- and I don't expect you to spend more time on it. So let's consider that we have performed due diligence and merge the PR.

@gasche
gasche approved these changes Sep 19, 2019
@Armael

This comment has been minimized.

Copy link
Member Author

Armael commented Sep 19, 2019

I don't understand the logic in this paragraph. If those functions are part of the implementation of a printer, they should be careful about ghost locations, shouldn't they?

My reasoning is as follows:

A printer is composed of an entry point (pp) and internal functions (all the others, in particular pp_loc and pp_main_loc). (the internal functions are exposed to allow defining new printers by "inheritance", but should be treated as internal to the logic of the printer...).
pp_loc and pp_main_loc assume in precondition that the location is not dummy. This precondition must be satisfied by the caller (either pp or another internal function); it is OK for pp_loc and pp_main_loc to require a precondition without checking it dynamically because they are internal functions (and not "user facing" like pp).

Thanks for approving the PR.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 19, 2019

This sounds like something we want to cherry-pick in the 4.09 branch: it's there since 4.08 bug it's easier to observe with the new warning in 4.09. I just changed the Changes file to make it easy (by creating the section), could you put it there and then merge?

@Armael Armael force-pushed the Armael:fix-toplevel-submsg-locs branch from 7fe7050 to e5a87c7 Sep 19, 2019
@Armael

This comment has been minimized.

Copy link
Member Author

Armael commented Sep 19, 2019

Done (I think?).

@Armael

This comment has been minimized.

Copy link
Member Author

Armael commented Sep 19, 2019

(I'm not familiar with the workflow for backporting patches so I didn't merge just yet; can you confirm that you're happy with Changes as it is now?)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 20, 2019

Yes: yes.

@gasche gasche merged commit 8f7708a into ocaml:trunk Sep 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gasche added a commit that referenced this pull request Sep 20, 2019
Fix error submessages in the toplevel: do not display dummy locations

(cherry picked from commit 8f7708a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.