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 issue 1032 #1037

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Fix issue 1032 #1037

merged 2 commits into from
Oct 17, 2019

Conversation

let-def
Copy link
Contributor

@let-def let-def commented Oct 17, 2019

This PR addresses the bug reported by @nojb in #1032

trefis
trefis requested changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

I only commented on some of them, but I don't understand why so many assertions are being commented out.

src/ocaml/preprocess/402/parser_raw.ml
@@ -27821,7 +27821,7 @@ end

# 27823 "src/ocaml/preprocess/parser_raw.ml"

# 269 "/Users/def/.opam/4.02.3/lib/menhir/standard.mly"
# 269 "<standard.mly>"
Copy link
Contributor

Choose a reason for hiding this comment

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

😮

src/ocaml/preprocess/408/parser_raw.mly
@@ -487,15 +487,15 @@ let class_of_let_bindings ~loc lbs body =
lbs.lbs_bindings
in
(* Our use of let_bindings(no_ext) guarantees the following: *)
assert (lbs.lbs_extension = None);
(* assert (lbs.lbs_extension = None); *)
Copy link
Contributor

Choose a reason for hiding this comment

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

If that assertion is incorrect, is it due to something specific in merlin, or is it something we should report upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unlikely that the parser recovery introduces an extension here, but in theory that might happen. It is ok to leave the assertion if you feel better about that.

src/ocaml/preprocess/408/parser_raw.mly Show resolved Hide resolved
src/ocaml/preprocess/408/parser_raw.mly
let psig_typesubst ((nr, ext), tys) =
assert (nr = Recursive); (* see [no_nonrec_flag] *)
let psig_typesubst ((_nr, ext), tys) =
(*assert (nr = Recursive);*) (* see [no_nonrec_flag] *)
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same remark.

@trefis
Copy link
Contributor

trefis commented Oct 17, 2019

W.r.t. all the disabled assertions, I guess the follow up question is: what happens if we do generate such a piece of parsetree during recovery? If the typechecker chokes on it, are we catching and hiding that error?

@let-def
Copy link
Contributor Author

let-def commented Oct 17, 2019

The piece of information that are checked are dropped immediately after the assertion.
The assertions are here to ensure some internal consistency of the parser rather than for the typechecker.

@trefis
Copy link
Contributor

trefis commented Oct 17, 2019

Ah! Thanks.

@trefis trefis merged commit 4b03351 into master Oct 17, 2019
@trefis trefis deleted the fix-issue-1032 branch October 17, 2019 14:12
@trefis trefis mentioned this pull request Oct 17, 2019
Closed
@let-def let-def mentioned this pull request Nov 29, 2019
Merged
let-def added a commit to let-def/opam-repository that referenced this pull request Nov 29, 2019
@let-def
[new release] merlin (3.3.3)
75ea28f 
CHANGES:

Fri Nov 29 17:35:58 CET 2019

  + backend
    - support OCaml 4.09 (ocaml/merlin#1055)
    - fix parse errors in 4.08 (ocaml/merlin#1037)
    - update 4.08 support to OCaml 4.08.1 (ocaml/merlin#1053)
    - support `without_cmis`
    - separate reading from caching in file-cache, use caching in
      `Env.check_state_consistency` (ocaml/merlin#1044)
    - simplify compiler state management (ocaml/merlin#1056, ocaml/merlin#1059)
    - fix creation of initial environment, improve compatibility with
      upstream 4.08 (ocaml/merlin#1052)
  + frontend
    - code re-organization (ocaml/merlin#1042)
    - error command: select which kind of errors to show (ocaml/merlin#995)
    - print value types in outline (ocaml/merlin#1014)
    - fix process handling in windows (ocaml/merlin#1005)
  + editor modes
    - emacs
      + bugfixes in merlin-imenu, merlin-xref (ocaml/merlin#1000, ocaml/merlin#1021, ocaml/merlin#1001)
      + show types in merlin-imenu (ocaml/merlin#1013)
      + reset buffer local configurations when resetting server (ocaml/merlin#1004)
      + remove merlin-use-tuareg-imenu
      + fix stack overflow (ocaml/merlin#1024)
      + fix merlin-occurrence (ocaml/merlin#1043)
    - vim
      + display warn-error warnings as errors (ocaml/merlin#1009)
  + testsuite
    - cover file-cache and `check_state_consistency` (ocaml/merlin#1044)
    - check inconsistent assumptions, test server versus single modes (ocaml/merlin#1047)
@let-def let-def mentioned this pull request Mar 2, 2020
Merged
let-def added a commit to let-def/opam-repository that referenced this pull request Mar 2, 2020
@let-def
[new release] merlin (3.3.4-4.10preview1)
bd38c15 
CHANGES:

Fri Nov 29 17:35:58 CET 2019

  + backend
    - support OCaml 4.09 (ocaml/merlin#1055)
    - fix parse errors in 4.08 (ocaml/merlin#1037)
    - update 4.08 support to OCaml 4.08.1 (ocaml/merlin#1053)
    - support `without_cmis`
    - separate reading from caching in file-cache, use caching in
      `Env.check_state_consistency` (ocaml/merlin#1044)
    - simplify compiler state management (ocaml/merlin#1056, ocaml/merlin#1059)
    - fix creation of initial environment, improve compatibility with
      upstream 4.08 (ocaml/merlin#1052)
  + frontend
    - code re-organization (ocaml/merlin#1042)
    - error command: select which kind of errors to show (ocaml/merlin#995)
    - print value types in outline (ocaml/merlin#1014)
    - fix process handling in windows (ocaml/merlin#1005)
  + editor modes
    - emacs
      + bugfixes in merlin-imenu, merlin-xref (ocaml/merlin#1000, ocaml/merlin#1021, ocaml/merlin#1001)
      + show types in merlin-imenu (ocaml/merlin#1013)
      + reset buffer local configurations when resetting server (ocaml/merlin#1004)
      + remove merlin-use-tuareg-imenu
      + fix stack overflow (ocaml/merlin#1024)
      + fix merlin-occurrence (ocaml/merlin#1043)
    - vim
      + display warn-error warnings as errors (ocaml/merlin#1009)
  + testsuite
    - cover file-cache and `check_state_consistency` (ocaml/merlin#1044)
    - check inconsistent assumptions, test server versus single modes (ocaml/merlin#1047)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trefis trefis trefis requested changes

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
@let-def @trefis