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

Fix superfluous break in error reporting #1432

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Fix superfluous break in error reporting #1432

merged 3 commits into from
Mar 18, 2022

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Jan 20, 2022 •
edited
Loading

Following a report by @panglesd this PR fix an issue with the formating of errors.

The following node:
[%%ocaml.error "test with several words"]

is reported by the compiler as:
Error: test with several words

but by merlin as:

test with several
words

The difference is that Merlin uses Location.print_main to produce the string while the compiler uses Location.print_report. The latter function wrap the printing of the main part of the report in a pretty-printing box while the former doesn't.
This PR wraps the output of Location.print_main in a pp box.

@voodoos voodoos requested a review from trefis January 20, 2022 11:19
voodoos
voodoos commented Jan 20, 2022
View reviewed changes
src/frontend/ocamlmerlin/query_json.ml
@@ -255,8 +255,7 @@ let json_of_error (error : Location.error) =
in
let loc = Location.loc_of_report error in
let msg =
Location.print_main Format.str_formatter error;
String.trim (Format.flush_str_formatter ())
Format.asprintf "@[%a@]" Location.print_main error |> String.trim
Copy link
Collaborator Author

@voodoos voodoos Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@trefis does the use of a box makes the trim superfluous ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have my answer: the testsuite disagrees strongly.

@voodoos
Copy link
Collaborator Author

voodoos commented Jan 25, 2022

ping @trefis ?

@trefis
Copy link
Contributor

trefis commented Jan 26, 2022

I'm a bit confused regarding how this is helping.
If the text fits on a line, why is a newline inserted in the absence of a box? Is it just that the formatter state is "dirty", leading to bad decisions when printing our string?

In any case, this seems "fine".

voodoos added 3 commits March 18, 2022 11:45
@voodoos
Add test illustrating issue
1a96232 
> There is an unnecessary linebreak between the last word of the error
@voodoos
Use a pp box to fix printing
2313934 
Promote correct result to test
@voodoos
Add CHANGES entry
719679f
@voodoos voodoos force-pushed the error-node-line-break branch from 6be9f92 to 719679f Compare March 18, 2022 14:46
@voodoos voodoos merged commit 66baa1e into ocaml:master Mar 18, 2022
voodoos added a commit to voodoos/merlin that referenced this pull request Mar 29, 2022
@voodoos
Merge pull request ocaml#1432 from voodoos/error-node-line-break
458fa0a 
Fix superfluous breaks in error reporting
voodoos added a commit to voodoos/merlin that referenced this pull request Mar 29, 2022
@voodoos
Merge pull request ocaml#1432 from voodoos/error-node-line-break
2f8d2a6 
Fix superfluous breaks in error reporting
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-413)
11d8687 
CHANGES:

Tue Apr  5 20:59:42 CEST 2020

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-412)
5d18fa3 
CHANGES:

Tue Apr  5 21:12:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - improve load path performance (ocaml/merlin#1323)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin and dot-merlin-reader (4.5-411)
64dcc60 
CHANGES:

Tue Apr 5 21:17:21 PM CET 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handling of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - update quick setup instructions for emacs (ocaml/merlin#1380, @ScriptDevil)
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Apr 5, 2022
@voodoos
[new release] merlin 4.5-414/413/412/411 and dot-merlin-reader 4.2
5940165 
CHANGES for 414:

Tue Apr  5 20:51:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
    - use the new "shapes" generated by the compiler to perform precise
      jump-to-definition (ocaml/merlin#1431)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)

CHANGES for 413:

Tue Apr  5 20:59:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - filter dups in source paths (ocaml/merlin#1218)
    - improve load path performance (ocaml/merlin#1323)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - expose all destruct exceptions in the api (ocaml/merlin#1437)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
    - cover locate calls on module aliases with and without dune
    - Add a test expliciting the interaction between locate and Dune's generated
      source files (ocaml/merlin#1444)

CHANGES for 412:

Tue Apr  5 21:12:42 CEST 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handlink of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - improve load path performance (ocaml/merlin#1323)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)

CHANGES for 411:

Tue Apr 5 21:17:21 PM CET 2022

  + merlin binary
    - don't reset the environment when running merlin in single mode so that the
      parent environement is forwarded the the child processes (ocaml/merlin#1425)
    - locate: look for original source files before looking for preprocessed
      files (ocaml/merlin#1219 by @ddickstein, fixes ocaml/merlin#894)
    - fix handling of ppx's under Windows (ocaml/merlin#1413)
    - handle `=` syntax in compiler flags (ocaml/merlin#1409)
    - fix superfluous break in error reporting (ocaml/merlin#1432)
    - recognise binding operators in locate and occurrences (ocaml/merlin#1398, @mattiase)
    - remove dependency on Result (ocaml/merlin#1441, @kit-ty-kate)
  + editor modes
    - update quick setup instructions for emacs (ocaml/merlin#1380, @ScriptDevil)
    - fix an issue in Neovim where the current line jumps to the top of the
      window on repeated calls to `MerlinTypeOf` (ocaml/merlin#1433 by @ddickstein, fixes
      ocaml/merlin#1221)
    - add module, module type, and class imenu items for emacs (ocaml/merlin#1244, @ivg)
    - add prefix argument to force or prevent opening in a new buffer in locate
      command (ocaml/merlin#1426, @panglesd)
    - add type-on-hover functionality for vim (ocaml/merlin#1439, @nilsbecker)
    - add a dedicated buffer `*merlin-errors*` containing the last viewed error
      (ocaml/merlin#1414, @panglesd)
  + test suite
    - make `merlin-wrapper` create a default `.merlin` file  only when there is
      no `dune-project` to let tests use `dune ocaml-merlin` reader. (ocaml/merlin#1425)
@voodoos voodoos mentioned this pull request Apr 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trefis trefis Awaiting requested review from trefis

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
@voodoos @trefis