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

Spurious inconsistent assumptions #1047

Merged
merged 4 commits into from
Nov 6, 2019
Merged

Spurious inconsistent assumptions #1047

merged 4 commits into from
Nov 6, 2019

Conversation

trefis
Copy link
Contributor

@trefis trefis commented Oct 25, 2019

Some time ago we removed the without_cmi thingy, which we did not need anymore with the new version of -short-paths (that is still waiting to be upstreamed).

However recently (i.e. 4.08), other parts of the compiler have been trying to load arbitrary .cmi files, that do not go through the usual "give me the best path" code, here are two I found today: Printtyp.rewrite_double_underscore_paths, Printtyp.Namespace.lookup.
There might be others.

This draft PR adds a test for the first one, I still have to add a test for the second.

As for the fix, the simplest one would be to reintroduce the without_cmi nonsense that the compiler is using.
Was there a reason to remove it other than "we don't need it"? I have the vague feeling there might have been a reason, but I can't remember.

@trefis trefis requested a review from let-def October 25, 2019 16:05
@trefis trefis marked this pull request as ready for review November 4, 2019 15:31
@trefis
Copy link
Contributor Author

trefis commented Nov 4, 2019

There were issues with without_cmis in the past, but they seem to have been fixed upstream by ocaml/ocaml#1223 .
So I reintroduced it in merlin, which fixes the issues (as shown by the test).

This is ready for review.

@trefis
Copy link
Contributor Author

trefis commented Nov 6, 2019

I'm reasonably confident this is not going to cause any issue.
So I'm merging now, feel free to complain when you come back from vacation.

@trefis trefis merged commit 40b77c9 into master Nov 6, 2019
@trefis
Copy link
Contributor Author

trefis commented Nov 14, 2019

Note: this broke the increasing of verbosity when printing expressions types; we sometimes do need to load cmis.
Right now in these situations we end up with this:

Not_found
Raised at file \"map.ml\", line 135, characters 10-25
Called from file \"src/ocaml/typing/408/env.ml\", line 1115, characters 6-28
Called from file \"src/analysis/type_utils.ml\", line 176, characters 17-39
Called from file \"src/ocaml/utils/misc.ml\", line 47, characters 8-15
Re-raised at file \"src/ocaml/utils/misc.ml\", line 59, characters 10-24
Called from file \"src/ocaml/utils/misc.ml\", line 73, characters 10-14
Re-raised at file \"src/ocaml/utils/misc.ml\", line 75, characters 38-45
Called from file \"src/ocaml/typing/408/env.ml\", line 645, characters 10-105
Called from file \"src/utils/std.ml\", line 679, characters 8-12
Re-raised at file \"src/utils/std.ml\", line 681, characters 30-39
Called from file \"src/frontend/query_commands.ml\", line 342, characters 12-132
Called from file \"list.ml\", line 96, characters 20-25
Called from file \"src/utils/local_store.ml\", line 29, characters 8-12
Re-raised at file \"src/utils/local_store.ml\", line 37, characters 4-15
Called from file \"src/kernel/mocaml.ml\", line 40, characters 8-38
Re-raised at file \"src/kernel/mocaml.ml\", line 48, characters 4-15
Called from file \"src/frontend/ocamlmerlin/new/new_commands.ml\", line 65, characters 15-53
Called from file \"src/utils/std.ml\", line 679, characters 8-12
Re-raised at file \"src/utils/std.ml\", line 681, characters 30-39
Called from file \"src/ocaml/utils/misc.ml\", line 47, characters 8-15
Re-raised at file \"src/ocaml/utils/misc.ml\", line 59, characters 10-24
Called from file \"src/frontend/ocamlmerlin/new/new_merlin.ml\", line 99, characters 18-54

Obviously at that point it is fine to load cmis, unfortunately we're already under a wrap_printing_env.
So we should either turn cmi loading back on (which Env.with_cmis doesn't actually do), or push down the call to wrap_printing_env.

@trefis
Copy link
Contributor Author

trefis commented Nov 14, 2019

More generally: type_utils.ml should be looked at in details now that we've reintroduced without_cmi.

@trefis trefis mentioned this pull request Nov 28, 2019
let-def added a commit to let-def/opam-repository that referenced this pull request Nov 29, 2019
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 added a commit to let-def/opam-repository that referenced this pull request Mar 2, 2020
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)
@trefis trefis deleted the inconsistent-assumptions branch January 29, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant