Skip to content

Use open_in_bin not open_in for merlinpp#1725

Merged
voodoos merged 1 commit into
ocaml:masterfrom
jonahbeckford:fix-file-open-win32
Jan 31, 2024
Merged

Use open_in_bin not open_in for merlinpp#1725
voodoos merged 1 commit into
ocaml:masterfrom
jonahbeckford:fix-file-open-win32

Conversation

@jonahbeckford
Copy link
Copy Markdown
Contributor

@jonahbeckford jonahbeckford commented Jan 30, 2024 •
edited
Loading

Fixes #1724 thanks to @nojb spotting the problem in ocaml-ppx/ppxlib#465.

@jonahbeckford
Use open_in_bin not open_in for merlinpp
1c43591 
Signed-off-by: Jonah Beckford <71855677+jonahbeckford@users.noreply.github.com>
@jonahbeckford
Copy link
Copy Markdown
Contributor Author

jonahbeckford commented Jan 30, 2024

There are also two problematic other uses of open_in.

First is https://github.com/ocaml/merlin/blob/master/src/utils/misc.ml#L520-L537

Second is https://github.com/ocaml/merlin/blob/master/src/utils/std.ml#L822-LL839

Both of these are the same and are not used in the Merlin code base. I assume they are helpers for consumers of Merlin (but why is it duplicated?). Regardless, I think those should be changed as well, but that could break Merlin consumers.

nojb
nojb approved these changes Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

cc @voodoos who has been approving and merging these PRs lately :)

@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Jan 31, 2024

There are also two problematic other uses of open_in.

First is https://github.com/ocaml/merlin/blob/master/src/utils/misc.ml#L520-L537

Second is https://github.com/ocaml/merlin/blob/master/src/utils/std.ml#L822-LL839

Both of these are the same and are not used in the Merlin code base. I assume they are helpers for consumers of Merlin (but why is it duplicated?). Regardless, I think those should be changed as well, but that could break Merlin consumers.

They might be leftovers since it's not even part of the upstream Misc module.

voodoos
voodoos approved these changes Jan 31, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you @jonahbeckford

@voodoos voodoos merged commit 8404f96 into ocaml:master Jan 31, 2024
voodoos added a commit to voodoos/merlin that referenced this pull request Feb 20, 2024
@voodoos
Use open_in_bin not open_in for merlinpp (ocaml#1725)
c1894f6 
from jonahbeckford/fix-file-open-win32
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin (3 packages) (4.14-414)
1e53e68 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.14-500)
7905af8 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin (3 packages) (4.14-501)
613d86e 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Feb 22, 2024
@voodoos
[new release] merlin-lib, merlin (4.14-501/500/414)
6b881a5 
CHANGES:

Thu Feb 22 14:00:42 CET 2024

  + merlin binary
    - Add a "heap_mbytes" field to Merlin server responses to report heap usage (ocaml/merlin#1717)
    - Add cache stats to telemetry (ocaml/merlin#1711)
    - Add new SyntaxDocument command to find information about the node under the cursor (ocaml/merlin#1706)
    - Fix `FLG -pp ppx.exe -as-pp/-dump-ast` use of invalid shell redirection when
    direct process launch on Windows. (ocaml/merlin#1723, fixes ocaml/merlin#1722)
    - Add a query_num field to the `ocamlmerlin` responses to detect server crashes (ocaml/merlin#1716)
    - Jump to cases within a match statement (ocaml/merlin#1726)
    - Jump to `module-type` (ocaml/merlin#1728, partially fixes ocaml/merlin#1656)
    - Exposes stable functions for configuration handling and pattern variable
      destruction. (ocaml/merlin#1730)
  + editor modes
    - vim: load merlin under the ocamlinterface and ocamllex filetypes (ocaml/merlin#1340)
    - Fix merlinpp not using binary file open (ocaml/merlin#1725, fixes ocaml/merlin#1724)
@voodoos voodoos mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@voodoos voodoos voodoos approved these changes

+1 more reviewer

@nojb nojb nojb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Backports for 5.(x-1)
Status: 4.14-500

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invalid arg from Marshal.from_bytes

3 participants

@jonahbeckford @voodoos @nojb