{"action":"created","comment":{"url":"https://api.github.com/repos/ocaml/ocaml/pulls/comments/208548225","pull_request_review_id":144377414,"id":208548225,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDIwODU0ODIyNQ==","diff_hunk":"@@ -1,61 +1,87 @@\n-Line 5, characters 0-1, Line 5, characters 5-7:  (3; 2;;\n-                                                 ^    ^^\n-Syntax error: ')' expected, the highlighted '(' might be unmatched\n-Line 2, characters 0-5,\n-Line 2, characters 10-12:\n+Line 5, characters 5-7,\n+Line 5, characters 0-1:","path":"testsuite/tests/parse-errors/unclosed_simple_expr.compilers.reference","position":7,"original_position":7,"commit_id":"0996b6d0f2f9012ad32345d497f8b232f8262e61","original_commit_id":"101a4ecabbd2a33ec6ce12893888d2a4f8a22778","user":{"login":"gasche","id":426238,"node_id":"MDQ6VXNlcjQyNjIzOA==","avatar_url":"https://avatars2.githubusercontent.com/u/426238?v=4","gravatar_id":"","url":"https://api.github.com/users/gasche","html_url":"https://github.com/gasche","followers_url":"https://api.github.com/users/gasche/followers","following_url":"https://api.github.com/users/gasche/following{/other_user}","gists_url":"https://api.github.com/users/gasche/gists{/gist_id}","starred_url":"https://api.github.com/users/gasche/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gasche/subscriptions","organizations_url":"https://api.github.com/users/gasche/orgs","repos_url":"https://api.github.com/users/gasche/repos","events_url":"https://api.github.com/users/gasche/events{/privacy}","received_events_url":"https://api.github.com/users/gasche/received_events","type":"User","site_admin":false},"body":"Given the constraint on this first step (not change the current code or results too much), I would think that sorting the locations before printing them textually (and highlighting them) would be slightly nicer than the current output, indeed.","created_at":"2018-08-08T11:38:27Z","updated_at":"2018-08-08T11:38:27Z","html_url":"https://github.com/ocaml/ocaml/pull/1952#discussion_r208548225","pull_request_url":"https://api.github.com/repos/ocaml/ocaml/pulls/1952","author_association":"MEMBER","_links":{"self":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/comments/208548225"},"html":{"href":"https://github.com/ocaml/ocaml/pull/1952#discussion_r208548225"},"pull_request":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/1952"}},"in_reply_to_id":208526804},"pull_request":{"url":"https://api.github.com/repos/ocaml/ocaml/pulls/1952","id":205436900,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA1NDM2OTAw","html_url":"https://github.com/ocaml/ocaml/pull/1952","diff_url":"https://github.com/ocaml/ocaml/pull/1952.diff","patch_url":"https://github.com/ocaml/ocaml/pull/1952.patch","issue_url":"https://api.github.com/repos/ocaml/ocaml/issues/1952","number":1952,"state":"open","locked":false,"title":"Location: refactor the code responsible for displaying errors and warnings","user":{"login":"Armael","id":315125,"node_id":"MDQ6VXNlcjMxNTEyNQ==","avatar_url":"https://avatars0.githubusercontent.com/u/315125?v=4","gravatar_id":"","url":"https://api.github.com/users/Armael","html_url":"https://github.com/Armael","followers_url":"https://api.github.com/users/Armael/followers","following_url":"https://api.github.com/users/Armael/following{/other_user}","gists_url":"https://api.github.com/users/Armael/gists{/gist_id}","starred_url":"https://api.github.com/users/Armael/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Armael/subscriptions","organizations_url":"https://api.github.com/users/Armael/orgs","repos_url":"https://api.github.com/users/Armael/repos","events_url":"https://api.github.com/users/Armael/events{/privacy}","received_events_url":"https://api.github.com/users/Armael/received_events","type":"User","site_admin":false},"body":"This PR is an attempt at refactoring the part of `Location` responsible for printing errors and warnings, to have a clearer separation between the structure of errors(/warnings), and backend-specific display of that structure.\r\n\r\nThere are indeed several \"backends\" on which an error can get printed, all with slightly different capabilities, in particular with respect to the highlighting of locations. I can distinguish:\r\n- \"batch mode\", i.e. the standard compiler output when called from the command-line\r\n- the toplevel, which e.g. can have the extra capability of going up and highlighting parts of the input to point at locations\r\n- merlin, which has different support from the editor to highlight code directly in the source code (currently merlin maintains a patched `location.ml`)\r\n- `expect_test` and the manual script `caml_tex.ml` also both display errors/collect locations in a custom way using the compiler-libs api.\r\n\r\nI would say that the current implementation has the following issues:\r\n- The current `Location` module exposes many functions related to the printing of errors/warnings/locations; with some of them being re-definable, and indeed re-defined in some places to allow for different printing backends. This makes the printing code entangled with the rest of the implementation, and hard to keep track of.\r\n- On the printing code side, a few quirks make it hard to print errors nicely ([example 1](https://github.com/ocaml/ocaml/blob/9ed695dfe0fb0a5c77b425f743dde436f8282d6b/parsing/location.ml#L478): there's a hack to feed a custom formatter to `errorf` which adds fake indentation to anticipate the fact that the message will be printed in front of `Error: `) ([example 2](https://github.com/ocaml/ocaml/blob/9ed695dfe0fb0a5c77b425f743dde436f8282d6b/testsuite/tests/typing-misc/pr6416.ml#L29): poor indentation in the printing of sub-locations for name conflicts)\r\n- On the error-message-writer side, it is hard to write errors that look good on all the backends, the result being that often, information that could be propagated to the backend for backend-specific display (e.g. sub-locations of an error) are instead baked in the error message.\r\n- As a corollary, trying to improve the printing of errors is hard (this was the initial motivation for this PR)\r\n\r\n## In this PR\r\n\r\nThis PR proposes to rework the current implementation, with minimal changes to its observational behavior, and pave the way for further improvements, in particular to the display of errors with several locations (typically, a main location and several sub-locations).\r\n\r\nMore specifically:\r\n- Rework the `Location.error` type. \r\n  - messages as `string`s do not compose well as soon as they are multiline (e.g. with regard to Format boxes). In this PR error messages become of the type `formatter -> unit`. This required some refactoring in some places (e.g. `printtyp.ml`) to make sure the side effects performed when crafting an error message are still triggered at the right time. We discussed a bit with @Drup and @Octachron of an alternative solution (where messages would instead be formats where all `%a`, `%t`, `%d`... have been expanded) but I leave this as a potential future improvement.\r\n  - remove the `if_highlight` field as it is intrinsically backend-dependent (it provided an alternative error message *when in the toplevel if parts of the input had been highlighted*) and is used in only one error message (\"syntax error: missing closing )\" )\r\n  - there is already a notion of sub-errors, which is used only once across the codebase (also for \"syntax error: missing closing )\"). Sub-errors were recursively error themselves; this PR changes it to a list of sub-*messages* (and *not* recursively). The rationale is that it is simpler, but hopefully expressive enough to describe somewhat faithfully the structure of errors with sub-locations. The plan in future PRs is to rework existing errors to actually use this feature, instead of baking sub-locations in the text of the error itself as it is currently done almost everywhere.\r\n- Generalize `Location.error` to a `Location.report` type, which can then be used to display warnings as well. The idea is that is gives us a more uniform printing of errors and warnings. The intent is that `Location.report` is a backend-agnostic description of an error or warning.\r\n- Define \"report printers\", which define backend-specific ways of printing a report. Also make it possible to easily reuse code between report printers (e.g. the toplevel printer is defined after the batch mode printer; expect_test and the manual backend are also defined by reusing the toplevel/batch mode backend and modifying some bits)\r\n- Limit the number of printing-related functions exposed by `Location`; the point is that everyone printing errors should emit a `Location.report`\r\n\r\nUnfortunately, because of the way the current implementation is entangled, I not manage to make these changes independently -- so they come in a big one commit, sorry for that!\r\n\r\n## Changes to the compiler output\r\n\r\nThe objective is to minimize the changes in the output; actual improvements to the way reports are printed should come in future PRs. There are nevertheless some changes:\r\n  - indentation-related, and arguably always an improvement\r\n  - there is a regression when displaying name conflicts in the terminal (there is no highlighting anymore). I plan to fix that in an upcoming PR which will turn such conflicts into proper sub-messages (I could do that here but it involves a whole bunch of uninteresting plumbing)\r\n  - changes to the \"missing closing)\" message; maybe not for the best but at least it is consistent with the rest now\r\n  - ~~one weird change that Format experts (@Drup @Octachron maybe?) may want to look at -- I point to it in a comment below.~~\r\n\r\n## Comments on the code\r\n\r\n- A `report_printer` is currently defined as a class type. The goal is to use inheritance to be able to re-use code between printers (the toplevel, expect_test and manual printers are defined that way). The currently used report printer can be changed when using the compiler-libs API. This way I that merlin can define its own report_printer (inheriting from the batch mode printer for example) and simply switch to it, instead of having to patch `location.ml`.\r\n\r\n  ~~If people are reluctant to have objects in this part of the compiler, I could rewrite the printers as a record of functions -- given the way inheritance between printers is used currently, it should not be *too* painful.~~ EDIT: printers have been rewritten as a record of functions\r\n\r\n- I expect this to cause some breakage at least in `utop`; I can send patches to fix it if needed.\r\n\r\n- The PR is best reviewed commit by commit. The two first commits move some code around and add docstrings in `location.{ml,mli}`, but do not change anything else. The third commit does the refactoring, and should probably be read starting from `location.mli`.\r\n","created_at":"2018-08-01T14:05:13Z","updated_at":"2018-08-08T11:38:27Z","closed_at":null,"merged_at":null,"merge_commit_sha":"9d41f179a7cdd7634822a608a7f58eb290522c95","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/ocaml/ocaml/pulls/1952/commits","review_comments_url":"https://api.github.com/repos/ocaml/ocaml/pulls/1952/comments","review_comment_url":"https://api.github.com/repos/ocaml/ocaml/pulls/comments{/number}","comments_url":"https://api.github.com/repos/ocaml/ocaml/issues/1952/comments","statuses_url":"https://api.github.com/repos/ocaml/ocaml/statuses/0996b6d0f2f9012ad32345d497f8b232f8262e61","head":{"label":"Armael:location-refactor","ref":"location-refactor","sha":"0996b6d0f2f9012ad32345d497f8b232f8262e61","user":{"login":"Armael","id":315125,"node_id":"MDQ6VXNlcjMxNTEyNQ==","avatar_url":"https://avatars0.githubusercontent.com/u/315125?v=4","gravatar_id":"","url":"https://api.github.com/users/Armael","html_url":"https://github.com/Armael","followers_url":"https://api.github.com/users/Armael/followers","following_url":"https://api.github.com/users/Armael/following{/other_user}","gists_url":"https://api.github.com/users/Armael/gists{/gist_id}","starred_url":"https://api.github.com/users/Armael/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Armael/subscriptions","organizations_url":"https://api.github.com/users/Armael/orgs","repos_url":"https://api.github.com/users/Armael/repos","events_url":"https://api.github.com/users/Armael/events{/privacy}","received_events_url":"https://api.github.com/users/Armael/received_events","type":"User","site_admin":false},"repo":{"id":74139961,"node_id":"MDEwOlJlcG9zaXRvcnk3NDEzOTk2MQ==","name":"ocaml","full_name":"Armael/ocaml","owner":{"login":"Armael","id":315125,"node_id":"MDQ6VXNlcjMxNTEyNQ==","avatar_url":"https://avatars0.githubusercontent.com/u/315125?v=4","gravatar_id":"","url":"https://api.github.com/users/Armael","html_url":"https://github.com/Armael","followers_url":"https://api.github.com/users/Armael/followers","following_url":"https://api.github.com/users/Armael/following{/other_user}","gists_url":"https://api.github.com/users/Armael/gists{/gist_id}","starred_url":"https://api.github.com/users/Armael/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Armael/subscriptions","organizations_url":"https://api.github.com/users/Armael/orgs","repos_url":"https://api.github.com/users/Armael/repos","events_url":"https://api.github.com/users/Armael/events{/privacy}","received_events_url":"https://api.github.com/users/Armael/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Armael/ocaml","description":"The core OCaml system: compilers, runtime system, base libraries","fork":true,"url":"https://api.github.com/repos/Armael/ocaml","forks_url":"https://api.github.com/repos/Armael/ocaml/forks","keys_url":"https://api.github.com/repos/Armael/ocaml/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Armael/ocaml/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Armael/ocaml/teams","hooks_url":"https://api.github.com/repos/Armael/ocaml/hooks","issue_events_url":"https://api.github.com/repos/Armael/ocaml/issues/events{/number}","events_url":"https://api.github.com/repos/Armael/ocaml/events","assignees_url":"https://api.github.com/repos/Armael/ocaml/assignees{/user}","branches_url":"https://api.github.com/repos/Armael/ocaml/branches{/branch}","tags_url":"https://api.github.com/repos/Armael/ocaml/tags","blobs_url":"https://api.github.com/repos/Armael/ocaml/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Armael/ocaml/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Armael/ocaml/git/refs{/sha}","trees_url":"https://api.github.com/repos/Armael/ocaml/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Armael/ocaml/statuses/{sha}","languages_url":"https://api.github.com/repos/Armael/ocaml/languages","stargazers_url":"https://api.github.com/repos/Armael/ocaml/stargazers","contributors_url":"https://api.github.com/repos/Armael/ocaml/contributors","subscribers_url":"https://api.github.com/repos/Armael/ocaml/subscribers","subscription_url":"https://api.github.com/repos/Armael/ocaml/subscription","commits_url":"https://api.github.com/repos/Armael/ocaml/commits{/sha}","git_commits_url":"https://api.github.com/repos/Armael/ocaml/git/commits{/sha}","comments_url":"https://api.github.com/repos/Armael/ocaml/comments{/number}","issue_comment_url":"https://api.github.com/repos/Armael/ocaml/issues/comments{/number}","contents_url":"https://api.github.com/repos/Armael/ocaml/contents/{+path}","compare_url":"https://api.github.com/repos/Armael/ocaml/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Armael/ocaml/merges","archive_url":"https://api.github.com/repos/Armael/ocaml/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Armael/ocaml/downloads","issues_url":"https://api.github.com/repos/Armael/ocaml/issues{/number}","pulls_url":"https://api.github.com/repos/Armael/ocaml/pulls{/number}","milestones_url":"https://api.github.com/repos/Armael/ocaml/milestones{/number}","notifications_url":"https://api.github.com/repos/Armael/ocaml/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Armael/ocaml/labels{/name}","releases_url":"https://api.github.com/repos/Armael/ocaml/releases{/id}","deployments_url":"https://api.github.com/repos/Armael/ocaml/deployments","created_at":"2016-11-18T15:07:30Z","updated_at":"2016-11-18T15:07:39Z","pushed_at":"2018-08-06T14:49:26Z","git_url":"git://github.com/Armael/ocaml.git","ssh_url":"git@github.com:Armael/ocaml.git","clone_url":"https://github.com/Armael/ocaml.git","svn_url":"https://github.com/Armael/ocaml","homepage":"","size":168734,"stargazers_count":0,"watchers_count":0,"language":"OCaml","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":{"key":"other","name":"Other","spdx_id":null,"url":null,"node_id":"MDc6TGljZW5zZTA="},"forks":0,"open_issues":0,"watchers":0,"default_branch":"trunk"}},"base":{"label":"ocaml:trunk","ref":"trunk","sha":"35218bf9dfa9452cf7192344d4a3b9ece2c50118","user":{"login":"ocaml","id":1841483,"node_id":"MDEyOk9yZ2FuaXphdGlvbjE4NDE0ODM=","avatar_url":"https://avatars3.githubusercontent.com/u/1841483?v=4","gravatar_id":"","url":"https://api.github.com/users/ocaml","html_url":"https://github.com/ocaml","followers_url":"https://api.github.com/users/ocaml/followers","following_url":"https://api.github.com/users/ocaml/following{/other_user}","gists_url":"https://api.github.com/users/ocaml/gists{/gist_id}","starred_url":"https://api.github.com/users/ocaml/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ocaml/subscriptions","organizations_url":"https://api.github.com/users/ocaml/orgs","repos_url":"https://api.github.com/users/ocaml/repos","events_url":"https://api.github.com/users/ocaml/events{/privacy}","received_events_url":"https://api.github.com/users/ocaml/received_events","type":"Organization","site_admin":false},"repo":{"id":6786166,"node_id":"MDEwOlJlcG9zaXRvcnk2Nzg2MTY2","name":"ocaml","full_name":"ocaml/ocaml","owner":{"login":"ocaml","id":1841483,"node_id":"MDEyOk9yZ2FuaXphdGlvbjE4NDE0ODM=","avatar_url":"https://avatars3.githubusercontent.com/u/1841483?v=4","gravatar_id":"","url":"https://api.github.com/users/ocaml","html_url":"https://github.com/ocaml","followers_url":"https://api.github.com/users/ocaml/followers","following_url":"https://api.github.com/users/ocaml/following{/other_user}","gists_url":"https://api.github.com/users/ocaml/gists{/gist_id}","starred_url":"https://api.github.com/users/ocaml/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ocaml/subscriptions","organizations_url":"https://api.github.com/users/ocaml/orgs","repos_url":"https://api.github.com/users/ocaml/repos","events_url":"https://api.github.com/users/ocaml/events{/privacy}","received_events_url":"https://api.github.com/users/ocaml/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/ocaml/ocaml","description":"The core OCaml system: compilers, runtime system, base libraries","fork":false,"url":"https://api.github.com/repos/ocaml/ocaml","forks_url":"https://api.github.com/repos/ocaml/ocaml/forks","keys_url":"https://api.github.com/repos/ocaml/ocaml/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ocaml/ocaml/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ocaml/ocaml/teams","hooks_url":"https://api.github.com/repos/ocaml/ocaml/hooks","issue_events_url":"https://api.github.com/repos/ocaml/ocaml/issues/events{/number}","events_url":"https://api.github.com/repos/ocaml/ocaml/events","assignees_url":"https://api.github.com/repos/ocaml/ocaml/assignees{/user}","branches_url":"https://api.github.com/repos/ocaml/ocaml/branches{/branch}","tags_url":"https://api.github.com/repos/ocaml/ocaml/tags","blobs_url":"https://api.github.com/repos/ocaml/ocaml/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ocaml/ocaml/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ocaml/ocaml/git/refs{/sha}","trees_url":"https://api.github.com/repos/ocaml/ocaml/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ocaml/ocaml/statuses/{sha}","languages_url":"https://api.github.com/repos/ocaml/ocaml/languages","stargazers_url":"https://api.github.com/repos/ocaml/ocaml/stargazers","contributors_url":"https://api.github.com/repos/ocaml/ocaml/contributors","subscribers_url":"https://api.github.com/repos/ocaml/ocaml/subscribers","subscription_url":"https://api.github.com/repos/ocaml/ocaml/subscription","commits_url":"https://api.github.com/repos/ocaml/ocaml/commits{/sha}","git_commits_url":"https://api.github.com/repos/ocaml/ocaml/git/commits{/sha}","comments_url":"https://api.github.com/repos/ocaml/ocaml/comments{/number}","issue_comment_url":"https://api.github.com/repos/ocaml/ocaml/issues/comments{/number}","contents_url":"https://api.github.com/repos/ocaml/ocaml/contents/{+path}","compare_url":"https://api.github.com/repos/ocaml/ocaml/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ocaml/ocaml/merges","archive_url":"https://api.github.com/repos/ocaml/ocaml/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ocaml/ocaml/downloads","issues_url":"https://api.github.com/repos/ocaml/ocaml/issues{/number}","pulls_url":"https://api.github.com/repos/ocaml/ocaml/pulls{/number}","milestones_url":"https://api.github.com/repos/ocaml/ocaml/milestones{/number}","notifications_url":"https://api.github.com/repos/ocaml/ocaml/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ocaml/ocaml/labels{/name}","releases_url":"https://api.github.com/repos/ocaml/ocaml/releases{/id}","deployments_url":"https://api.github.com/repos/ocaml/ocaml/deployments","created_at":"2012-11-20T22:18:22Z","updated_at":"2018-08-08T10:59:14Z","pushed_at":"2018-08-08T10:58:46Z","git_url":"git://github.com/ocaml/ocaml.git","ssh_url":"git@github.com:ocaml/ocaml.git","clone_url":"https://github.com/ocaml/ocaml.git","svn_url":"https://github.com/ocaml/ocaml","homepage":"","size":174898,"stargazers_count":1925,"watchers_count":1925,"language":"OCaml","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":475,"mirror_url":null,"archived":false,"open_issues_count":174,"license":{"key":"other","name":"Other","spdx_id":null,"url":null,"node_id":"MDc6TGljZW5zZTA="},"forks":475,"open_issues":174,"watchers":1925,"default_branch":"trunk"}},"_links":{"self":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/1952"},"html":{"href":"https://github.com/ocaml/ocaml/pull/1952"},"issue":{"href":"https://api.github.com/repos/ocaml/ocaml/issues/1952"},"comments":{"href":"https://api.github.com/repos/ocaml/ocaml/issues/1952/comments"},"review_comments":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/1952/comments"},"review_comment":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/ocaml/ocaml/pulls/1952/commits"},"statuses":{"href":"https://api.github.com/repos/ocaml/ocaml/statuses/0996b6d0f2f9012ad32345d497f8b232f8262e61"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"gasche"}}