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

(include_subdirs qualified) leads to some spurious ocaml-lsp errors #8297

Closed
tjdevries opened this issue Jul 29, 2023 · 12 comments · Fixed by #9659 or ocaml/opam-repository#25081
Closed
Labels
Milestone

Comments

@tjdevries
Copy link

Expected Behavior

No errors from ocaml-lsp (project successfully compiled), for example this succeeds.

❯ dune build @check   

(You can see the code here: https://github.com/tjdevries/thereactagen/blob/2dfcfc8b7473f771f3f14927fc75c90bb3609131/lib/models/dune )

Actual Behavior

image

(Same error in .ml file and .mli file for lib/models/storage/custom.mli

Reproduction

  • PR with a reproducing test:
  1. I'm not sure how to reproduce outside of the project, but pretty sure it's from the qualified usage. I linked the

Specifications

  • Version of dune (output of dune --version): 3.9.1
  • Version of ocaml (output of ocamlc --version): 5.0.0
  • Operating system (distribution and version): Pop OS

(cc @anmonteiro, i can give more info if needed)

@Alizter
Copy link
Collaborator

Alizter commented Aug 2, 2023

Hi, your explanation of the reproduction looks a bit cut-off; do you have any more information? Particularly I am not sure I understood if the error is coming from ocamllsp or dune? If it is possible, could you create a minimal example of the issue, maybe inside a cram test?

p.s. love your videos :)

@tjdevries
Copy link
Author

Sorry! I didn't see this reply (and so happy to hear you like the videos!!)

This was only from the LSP, not from dune. Dune was able to successfully build, but @anmonteiro said this would be a better spot to leave an issue (or at least, mentioned it in passing on stream). I can try and make a smaller repro at some point

@psb
Copy link

psb commented Aug 21, 2023

I've come across the same problem. I posted this message about it in the Discord:

I've been working on a NextJS app which uses folders for routing. Some have suggested adding a dune file per folder and some have suggested using the stanza (include_subdirs qualified) in a parent folder so that you don't have to add a dune file for every route, which would get tedious quickly if you have lots of routes. I have tried both ways, and using (include_subdirs qualified) gives me red squiggles in my editor but the app compiles fine (no dune errors), and adding dune files to every route folder gets rid of the red squiggles.

With (include_subdirs qualified):
Screenshot 2023-08-14 at 15 10 40

With dune files in each folder:
Screenshot 2023-08-14 at 15 19 51

The code to reproduce the error is in this branch.

@jake-87
Copy link

jake-87 commented Sep 8, 2023

Also happening to me.
See this branch, lib/ for another replication: Khasmc branch.
Dune 3.10.X, opam version 2.1.5 .
I can also confirm with fairly high confidence that it is the usage of qualified - this emerged immediately after I moved said project over to include_subdirs qualified.

I happen to be getting a message at the very top of the file:
Unbound module Frontend
despite the fact that the cm* files are being generated in _build/install/default/<project specific stuff>/khasmc__Frontend.cm{i,x,t}

Out of curiosity, @psb, what's in those dune folders? For me, just empty dune folders doesn't seem to work - I'd like to see if your solution works for me too.

@rgrinberg
Copy link
Member

A great way to move this forward is for someone to create a minimal reproducible test case demonstrating the issue and add it to our test suite. Although we can't test ocamllsp, we do have tests that demonstrate we output the correct configuration for ocamllsp and merlin. One could use those to point out where the configuration is wrong.

@jake-87
Copy link

jake-87 commented Sep 8, 2023

@rgrinberg Apologies, I don't know your preferred style for adding tests and whatnot, but this is an MRE for the issue as far as I can tell: ocamllsp MRE. Code compiles fine, warnings in the editor in lib/one/onefile.ml. This is with a completely fresh install of ocamllsp and dune, so I don't think it's outside config or factors causing the issue.

@psb
Copy link

psb commented Sep 10, 2023

Also happening to me. See this branch, lib/ for another replication: Khasmc branch. Dune 3.10.X, opam version 2.1.5 . I can also confirm with fairly high confidence that it is the usage of qualified - this emerged immediately after I moved said project over to include_subdirs qualified.

I happen to be getting a message at the very top of the file: Unbound module Frontend despite the fact that the cm* files are being generated in _build/install/default/<project specific stuff>/khasmc__Frontend.cm{i,x,t}

Out of curiosity, @psb, what's in those dune folders? For me, just empty dune folders doesn't seem to work - I'd like to see if your solution works for me too.

They are just library definitions, e.g.,:

(library
  (name joke_route)
  ...

I then include this library in the src/app/api/dune file:

(library
  (name api)
  (libraries joke_route)
  ...

@jake-87
Copy link

jake-87 commented Sep 11, 2023

Right, I see. Unfortunately that doesn't work for my usecase, but it does seem to work if your directories aren't codependent.
Much appreciated nonetheless.

@jake-87
Copy link

jake-87 commented Sep 12, 2023

@rgrinberg Apologies if I wasn't clear earlier - I'm more then happy to create a testcase for dune, but I'm not sure what I'm looking for in regard to dune's generated files being correct. I've tried looking through what files are accessed while ocamllsp or merlin are running (It seems to affect both), but without knowing the underlying formats, I'm not getting far. I'd much appreciate if you would be able to point me to where I might find the documentation for these formats that dune produces, so that I might figure out what's going on and try to make a testcase?

@Tchou
Copy link

Tchou commented Oct 30, 2023

Here is a repro case. Note, that there is nothing wrong with the invocation of dune build, but it seems that either the information that gets sent to ocamllsp is wrong or ocamllsp itself is buggy in that respect (and I don't know how to test this later part).

$ tree dune_test_qualified 
dune_test_qualified
├── a
│   ├── b
│   │   ├── mod_b1.ml
│   │   └── mod_b2.ml
│   ├── dune
│   └── mod_a.ml
└── dune-project

3 directories, 5 files
$ cat dune_test_qualified/dune-project
(lang dune 3.7)
$ cat dune_test_qualified/a/dune
(include_subdirs qualified)
(library
 (name a))
$ cat dune_test_qualified/a/mod_a.ml
let f () = B.Mod_b1.f ()
$ cat dune_test_qualified/a/b/mod_b1.ml
let f () = Mod_b2.T.f ()
$ cat dune_test_qualified/a/b/mod_b2.ml
module T =
struct
  let f () = ()
end

Now doing dune build works. But after that, opening the project is VSCode gives:
image

So it seems that in the context of the mod_b2.ml tab, the scope (for ocamllsp) is the toplevel module A. And indeed, if I replace with the full path, that is:

let f () = B.Mod_b2.T.f ()

the error disappears in VSCode and f() gets resolved to the correct identifier (e.g. if I run Go to Definition the editor jumps to the definition of f in T in mod_b2.ml), but obviously this is wrong and dune build signals a cyclic dependency:

Error: Module Mod_b1 in directory _build/default/a depends on B.
This doesn't make sense to me.

I can upload a tarball somewhere to ease the reproduction if needed (but the example here is pretty small).

@rgrinberg rgrinberg added the bug label Oct 31, 2023
@AdamBrodzinski
Copy link

I'm running into this as well. I've pushed a minimal repro here:
https://github.com/AdamBrodzinski/dune-issue-8297-repro

  • dune build (no issues building)
  • vist bin/utils/calc.ml in an editor with ocaml lsp
Screenshot 2023-12-20 at 9 04 51 AM

rgrinberg added a commit that referenced this issue Jan 7, 2024
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: f85cd2ad-1da0-44b1-b5c4-aa7f220e9446 -->
rgrinberg added a commit that referenced this issue Jan 7, 2024
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: f85cd2ad-1da0-44b1-b5c4-aa7f220e9446 -->
rgrinberg added a commit that referenced this issue Jan 7, 2024
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg added a commit that referenced this issue Jan 7, 2024
Use the same -open flags that are used for compilation

Fixes #8297

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: f8c2fcc7-4268-401a-9b59-4e734fcb923c -->
rgrinberg added a commit that referenced this issue Jan 7, 2024
Use the same -open flags that are used for compilation

Fixes #8297

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: f8c2fcc7-4268-401a-9b59-4e734fcb923c -->
@rgrinberg rgrinberg added this to the 3.13.0 milestone Jan 7, 2024
@rgrinberg
Copy link
Member

Should be fixed in #9659. Thanks for the detailed reports everyone.

rgrinberg added a commit that referenced this issue Jan 8, 2024
Use the same -open flags that are used for compilation

Fixes #8297

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
emillon added a commit to emillon/opam-repository that referenced this issue Jan 12, 2024
CHANGES:

- Do not ignore `(formatting ..)` settings in context or workspace files
  (ocaml/dune#8447, @rgrinberg)

- Add command `dune cache clear` to completely delete all traces of the Dune
  cache. (ocaml/dune#8975, @nojb)

- Fixed a bug where Dune was incorrectly parsing the output of coqdep when it
  was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter)

- Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg)

- Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace
  file unless language version is at least 3.2.

- [coq] Fix bug in computation of flags when composed with boot theories.
  (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego)

- Fixed a bug where the `(select)` field of the `(libraries)` field of the
  `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter)

- Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego)

- Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An
  intermediate directory called `NAME` is no longer created if `PATH` is
  supplied, so `dune init proj my_project .` will now initialize a project in
  the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder)

- Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file
  through the `env` stanza. (ocaml/dune#9449, @rgrinberg)

- Experimental doc rules: Correctly handle the case when a package depends upon
  its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam)

- Resolve various public binaries to their build location, rather than to where
  they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908,
  @rgrinberg).

- Menhir: generate `.conflicts` file by default. Add new field to the
  `(menhir)` stanza to control the generation of this file: `(explain <blang
  expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the
  `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under
  a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb)

- Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg)

- Directory targets can now be caches. (ocaml/dune#9535, @rleshchinskiy)

- Remove warning 30 from default set for projects where dune lang is at least
  3.13 (ocaml/dune#9568, @gasche)

- It is now possible to use special forms such as `(:include)` and variables
  `%{read-lines:}` in `(modules)` and similar fields. Note that the
  dependencies introduced in this way (ie the files being read) must live in a
  different directory than the stanza making use of them. (ocaml/dune#9578, @nojb)

- Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517,
  @PoorlyDefinedBehaviour)

- Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659,
  fixes ocaml/dune#8297, @rgrinberg)

- Fix handling of `enabled_if` in binary install stanzas. Previously, we'd
  ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707,
  @rgrinberg)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- ctypes: fix an error where `(ctypes)` with no `(function_description)` would
  cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302,
  fix ocaml/dune#9300, @emillon)
emillon added a commit to emillon/opam-repository that referenced this issue Jan 16, 2024
CHANGES:

### Added

- Add command `dune cache clear` to completely delete all traces of the Dune
  cache. (ocaml/dune#8975, @nojb)

- Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego)

- Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file
  through the `env` stanza. (ocaml/dune#9449, @rgrinberg)

- Menhir: generate `.conflicts` file by default. Add new field to the
  `(menhir)` stanza to control the generation of this file: `(explain <blang
  expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the
  `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under
  a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb)

- Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy)

- It is now possible to use special forms such as `(:include)` and variables
  `%{read-lines:}` in `(modules)` and similar fields. Note that the
  dependencies introduced in this way (ie the files being read) must live in a
  different directory than the stanza making use of them. (ocaml/dune#9578, @nojb)

- Remove warning 30 from default set for projects where dune lang is at least
  3.13 (ocaml/dune#9568, @gasche)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- ctypes: fix an error where `(ctypes)` with no `(function_description)` would
  cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302,
  fix ocaml/dune#9300, @emillon)

### Changed

- Check that package names in `(depends)` and related fields in `dune-project`
  are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS)

### Fixed

- Do not ignore `(formatting ..)` settings in context or workspace files
  (ocaml/dune#8447, @rgrinberg)

- Fixed a bug where Dune was incorrectly parsing the output of coqdep when it
  was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter)

- Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg)

- Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace
  file unless language version is at least 3.2.

- [coq] Fix bug in computation of flags when composed with boot theories.
  (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego)

- Fixed a bug where the `(select)` field of the `(libraries)` field of the
  `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter)

- Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An
  intermediate directory called `NAME` is no longer created if `PATH` is
  supplied, so `dune init proj my_project .` will now initialize a project in
  the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder)

- Experimental doc rules: Correctly handle the case when a package depends upon
  its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam)

- Resolve various public binaries to their build location, rather than to where
  they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908,
  @rgrinberg).

- Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg)

- Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517,
  @PoorlyDefinedBehaviour)

- Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659,
  fixes ocaml/dune#8297, @rgrinberg)

- Fix handling of `enabled_if` in binary install stanzas. Previously, we'd
  ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707,
  @rgrinberg)
nberth pushed a commit to nberth/opam-repository that referenced this issue Jun 18, 2024
CHANGES:

### Added

- Add command `dune cache clear` to completely delete all traces of the Dune
  cache. (ocaml/dune#8975, @nojb)

- Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego)

- Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file
  through the `env` stanza. (ocaml/dune#9449, @rgrinberg)

- Menhir: generate `.conflicts` file by default. Add new field to the
  `(menhir)` stanza to control the generation of this file: `(explain <blang
  expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the
  `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under
  a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb)

- Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy)

- It is now possible to use special forms such as `(:include)` and variables
  `%{read-lines:}` in `(modules)` and similar fields. Note that the
  dependencies introduced in this way (ie the files being read) must live in a
  different directory than the stanza making use of them. (ocaml/dune#9578, @nojb)

- Remove warning 30 from default set for projects where dune lang is at least
  3.13 (ocaml/dune#9568, @gasche)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- ctypes: fix an error where `(ctypes)` with no `(function_description)` would
  cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302,
  fix ocaml/dune#9300, @emillon)

### Changed

- Check that package names in `(depends)` and related fields in `dune-project`
  are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS)

### Fixed

- Do not ignore `(formatting ..)` settings in context or workspace files
  (ocaml/dune#8447, @rgrinberg)

- Fixed a bug where Dune was incorrectly parsing the output of coqdep when it
  was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter)

- Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg)

- Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace
  file unless language version is at least 3.2.

- [coq] Fix bug in computation of flags when composed with boot theories.
  (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego)

- Fixed a bug where the `(select)` field of the `(libraries)` field of the
  `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter)

- Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An
  intermediate directory called `NAME` is no longer created if `PATH` is
  supplied, so `dune init proj my_project .` will now initialize a project in
  the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder)

- Experimental doc rules: Correctly handle the case when a package depends upon
  its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam)

- Resolve various public binaries to their build location, rather than to where
  they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908,
  @rgrinberg).

- Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg)

- Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517,
  @PoorlyDefinedBehaviour)

- Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659,
  fixes ocaml/dune#8297, @rgrinberg)

- Fix handling of `enabled_if` in binary install stanzas. Previously, we'd
  ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707,
  @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment