Skip to content

feature: enable (include_subdirs qualified)#6594

Merged
rgrinberg merged 1 commit intomainfrom
ps/rr/feature__enable__include_subdirs_qualified_
Dec 6, 2022
Merged

feature: enable (include_subdirs qualified)#6594
rgrinberg merged 1 commit intomainfrom
ps/rr/feature__enable__include_subdirs_qualified_

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Nov 27, 2022

(include_subdirs qualified) for OCaml libraries and executables

Aliasing

We now build an alias module for every directory. To build a module x/y/z.ml, we need to open -open X -open Y. Similarly, we can write an "interface module" for every directory:

$ ls -R
bar/
  foo.ml
  bar.ml

In this example, Bar.Foo isn't accessible unless it's re-exported by bar.ml

Ocamldep Hacks

Currently ocamldep can't give us precise deps in situations such as:

$ ls -R
bar/
  baz.ml
foo.ml
$ cat foo.ml
let _ = Bar.Baz.foo

ocamldep gives us Bar, even though we really want to know that it's Lib__Bar__Baz that we need. For now we just depend on everything in bar/. Hopefully codept will help us out here.

TODO

  • aliasing scheme
  • dune-package support
  • interface files for module groups
  • merlin support
  • ppx
  • fix describe

Out of scope (for subsequent PR's):

  • documentation
  • per module preprocessing, virtual libraries, modules without implementation, private modules
  • unwrapped libraries
  • support in bootstrap

@rgrinberg rgrinberg linked an issue Nov 27, 2022 that may be closed by this pull request
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 3 times, most recently from 8803c58 to bede0ef Compare November 27, 2022 19:23
@rgrinberg rgrinberg marked this pull request as draft November 27, 2022 19:27
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 5 times, most recently from 81b56de to 233a745 Compare November 27, 2022 23:08
Copy link
Copy Markdown
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

did a very rough first pass, take all my suggestions as optional, as I'm trying to wrap my head around the implementation

@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 5 times, most recently from 2078ace to 4e09f53 Compare November 28, 2022 03:47
@rgrinberg rgrinberg added this to the 3.7.0 milestone Nov 28, 2022
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 2 times, most recently from 7cfa498 to 12785e8 Compare November 28, 2022 05:20
@rgrinberg
Copy link
Copy Markdown
Member Author

Okay, it's "alive". Nowhere near usable but the basic examples work.

@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 3 times, most recently from 01be69c to 3414e23 Compare November 29, 2022 00:12
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 5 times, most recently from c2733f6 to 676651c Compare November 29, 2022 16:14
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 2 times, most recently from 01ef92e to 000e561 Compare December 4, 2022 17:45
@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Dec 5, 2022

The way it currently works by looking up Lib3 in parent directories. I don't think this should be allowed. Why can't merlin just copy location directives to find the path to the module?

Merlin should be made aware of copy-rules and use them to determine source file location, but it remains to be done. The current behavior was introduced to reproduce how Merlin behaved before the dune-based configuration reader. I agree that it should eventually be replaced.

@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 4 times, most recently from 5e30bf3 to a8b837a Compare December 5, 2022 21:28
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Dec 5, 2022

Alright, tests are passing.

@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch from a8b837a to 3e5885c Compare December 5, 2022 22:30
@rgrinberg rgrinberg force-pushed the ps/rr/feature__enable__include_subdirs_qualified_ branch 7 times, most recently from b75caab to 40f8494 Compare December 6, 2022 02:26
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: f0bdd789-87c5-4047-b418-47cdaf7749ae
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.

Add support for (include_subdirs qualified)

4 participants