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

Incorrect tracking of identifier through inlcude module type #1848

Open
liam923 opened this issue Sep 30, 2024 · 3 comments · May be fixed by #1857
Open

Incorrect tracking of identifier through inlcude module type #1848

liam923 opened this issue Sep 30, 2024 · 3 comments · May be fixed by #1857
Labels

Comments

@liam923
Copy link
Contributor

liam923 commented Sep 30, 2024

Here is a cram test of the issue:

Create a module with an mli file
  $ cat > foo.ml << EOF
  > type t = Foo
  > module Bar = struct
  >   type t = Bar
  > end
  > EOF

  $ cat > foo.mli << EOF
  > module Bar : sig
  >   type t
  > end
  > type t
  > EOF

  $ $OCAMLC -c -bin-annot foo.mli
  $ $OCAMLC -c -bin-annot foo.ml

Locate the Bar on line 4
  $ cat > test1.ml << EOF
  > module type Foo = sig
  >   include module type of Foo
  >   module Bar : sig
  >     include module type of Bar
  >   end
  > end
  > EOF
The expected location is 2:7 of foo.ml, but it instead goes to 1:9, which is the
constructor Foo
  $ $MERLIN single locate -position 4:28 -look-for ml -filename test1.ml < test1.ml | jq .value
  {
    "file": "$TESTCASE_ROOT/foo.ml",
    "pos": {
      "line": 1,
      "col": 9
    }
  }

Locate the Bar on line 3
  $ cat > test2.ml << EOF
  > include Foo
  > module Bar = struct
  >   include Bar
  > end
  warning: here-document at line 1 delimited by end-of-file (wanted `EOF')
Correctly returns 2:7
  $ $MERLIN single locate -position 3:12 -look-for ml -filename test2.ml < test2.ml | jq .value
  {
    "file": "$TESTCASE_ROOT/foo.ml",
    "pos": {
      "line": 2,
      "col": 7
    }
  }

Locate the Foo.Bar on line 1
  $ cat > test3.ml << EOF
  > include module type of Foo.Bar
  > EOF
Correctly returns 2:7
  $ $MERLIN single locate -position 1:28 -look-for ml -filename test3.ml < test3.ml | jq .value
  {
    "file": "$TESTCASE_ROOT/foo.ml",
    "pos": {
      "line": 2,
      "col": 7
    }
  }

Locating the Bar originating from an include module type of Foogoes to the wrong location. Looking at the logs for the failing case, we see:

# 0.01 env-lookup - env_lookup
  found: 'Bar/279[1]' in namespace module with decl_uid Foo.1
  at loc File "foo.mli", line 1, characters 0-29
  # 0.01 locate - shape_of_path
  initial: <Foo.1>
  # 0.01 locate - shape_of_path
  reduced: Resolved: Foo.1

Foo.1 is correct in the sense that Bar is Foo.1 according to foo.mli, but Foo.1 in foo.ml is the constructor Foo, which is what Merlin incorrectly returns.

Compare this to the logs in the successful cases:

# 0.01 env-lookup - env_lookup
  found: 'Bar/279[1]' in namespace module with decl_uid Foo.1
  at loc File "foo.mli", line 1, characters 0-29
  # 0.01 locate - shape_of_path
  initial: CU Foo . "Bar"[module]
  # 0.01 locate - read_unit_shape
  inspecting Foo
  # 0.01 stat_cache - reuse cache
  $TESTCASE_ROOT
  # 0.01 File_cache(Exists_in_directory) - read
  reading "$TESTCASE_ROOT" from disk
  # 0.01 stat_cache - reuse cache
  $TESTCASE_ROOT/foo.cmt
  # 0.01 File_cache(Cmt_cache) - read
  reusing "$TESTCASE_ROOT/foo.cmt"
  # 0.01 locate - File_switching.move_to
  file: $TESTCASE_ROOT/foo.cmt
  digest: 4fd51ef55d4eb3719a2531fcdd6bff07
  # 0.01 locate - File_switching.move_to
  file: foo.ml
  digest: 4fd51ef55d4eb3719a2531fcdd6bff07
  # 0.01 locate - read_unit_shape
  shapes loaded for Foo
  # 0.01 locate - shape_of_path
  reduced: Resolved: Foo.4
@voodoos
Copy link
Collaborator

voodoos commented Oct 1, 2024

Thanks for the very thorough report ! I will have a look.

@voodoos
Copy link
Collaborator

voodoos commented Oct 8, 2024

Foo.1 is correct in the sense that Bar is Foo.1 according to foo.mli, but Foo.1 in foo.ml is the constructor Foo, which is what Merlin incorrectly returns.

This is a case of "the uid doesn't indicate if it comes from the implementation or the interface" and we should be able to provide a robust fix soon thanks to ocaml/ocaml#13286

voodoos added a commit to voodoos/merlin that referenced this issue Oct 8, 2024
@voodoos
Copy link
Collaborator

voodoos commented Oct 8, 2024

Given how locate works right now, I would expect the correct result of locating Bar in the failing test to be 1:9 in foo.mli since only the module type of Foo was included and not the module implementation itself.

We might, however, be more clever at some point by looking at the new information linking declaration together in 5.3 ocaml/ocaml#13308

voodoos added a commit to voodoos/merlin that referenced this issue Oct 15, 2024
voodoos added a commit to voodoos/merlin that referenced this issue Oct 15, 2024
voodoos added a commit to voodoos/merlin that referenced this issue Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants