Skip to content

feat(oxcaml): support parameter implementation#12392

Merged
shonfeder merged 24 commits intomainfrom
maiste/library_param_impl
Sep 9, 2025
Merged

feat(oxcaml): support parameter implementation#12392
shonfeder merged 24 commits intomainfrom
maiste/library_param_impl

Conversation

@shonfeder
Copy link
Copy Markdown
Member

@shonfeder shonfeder commented Sep 4, 2025

Closes #12085
Supersedes #12002

This PR is the second of a set of four PRs related to the introduction of Parameterized Libraries in Dune.

⚠️ This feature is only available with OxCaml until it is upstream to the compiler.

It contains multiples changes:

  • It adds support for parameter in the implements field.
  • It provides some tests to make sure the functionality act according to the specification. They are still guarded by the oxcaml variable.
  • It extends the documentation with our case. More explanation about the feature will be addressed in future PRs, specific to the documentation.

TODO

@shonfeder shonfeder force-pushed the maiste/library_param_impl branch from f7c52d8 to 08b368d Compare September 4, 2025 02:53
shonfeder
shonfeder commented Sep 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member Author

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Copying over the outstanding change requests from #12002

@shonfeder shonfeder mentioned this pull request Sep 4, 2025
Closed
3 tasks
maiste
maiste reviewed Sep 4, 2025
View reviewed changes
@art-w art-w force-pushed the maiste/library_param_impl branch 2 times, most recently from a9b566d to dcb3793 Compare September 4, 2025 10:58
@art-w art-w mentioned this pull request Sep 4, 2025
Merged
@shonfeder
Copy link
Copy Markdown
Member Author

shonfeder commented Sep 4, 2025

Thanks for the fixes, @art-w! I'll let you take this from here :D

IIUC, this is ready for review, so I will mark it as such.

@shonfeder shonfeder marked this pull request as ready for review September 4, 2025 23:53
rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg requested changes Sep 5, 2025
View reviewed changes
src/dune_rules/virtual_rules.ml Outdated
(Lib_name.to_string implements)
];
(match Lib_info.kind info with
| Parameter | Virtual -> ()
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Sep 5, 2025

Choose a reason for hiding this comment

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

The Parameter case should be "assert false" I believe

Copy link
Copy Markdown
Collaborator

@art-w art-w Sep 5, 2025

Choose a reason for hiding this comment

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

Hm in the current implementation the Parameter case is useful, as a library that has an implements still get turned into a "vlib" which associates the library with the (virtual or parameter) lib it implements. I suppose we could avoid reusing parts of the virtual logic however, with a new sum type above. WDYT?

Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Sep 5, 2025

Choose a reason for hiding this comment

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

I think it's confusing indeed then. Turning it into a sum type (or a record with a kind field) would clarify this.

Copy link
Copy Markdown
Collaborator

@art-w art-w Sep 8, 2025

Choose a reason for hiding this comment

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

I pushed a refactoring of Virtual_rules to distinguish between the three implements possibilities: no implements, implementing a virtual module or implementing a parameter. I think virtual_rules.ml could be further renamed to implements.ml, if the name "virtual" has too much connotation? :)

rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
rgrinberg
rgrinberg requested changes Sep 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Just finished a round of review.

@art-w
Copy link
Copy Markdown
Collaborator

art-w commented Sep 5, 2025

Thanks for the quick review! I've marked the simpler issues as resolved by the last commit, but I've left open the ones where I would appreciate your input :)

rgrinberg
rgrinberg reviewed Sep 5, 2025
View reviewed changes
src/dune_rules/module_compilation.ml
in
match argument with
| None -> []
| Some parameter -> [ "-as-argument-for"; Module_name.to_string parameter ])
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Sep 5, 2025

Choose a reason for hiding this comment

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

Could you remind where we add a dependency on the cmi of the parameter? Is it added a library dependency somewhere?

Copy link
Copy Markdown
Collaborator

@art-w art-w Sep 8, 2025

Choose a reason for hiding this comment

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

It looks like the dependency to the parameter is added by the pre-existing code for vlibs:

dune/src/dune_rules/lib.ml

Lines 1025 to 1034 in 07d0a93

let* requires =
Memo.return
(let open Resolve.O in
let* requires = requires in
match implements with
| None -> Resolve.return requires
| Some impl ->
let+ impl = impl in
impl :: requires)
in

@maiste @art-w
chore: set the parametized to wrapped
6a9a212 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
feat(tmp): Singleton working version of parameterized library
8905390 
This version works only in the singleton case at it aggressively
add the module implements to all the module. In the case of the
singleton it will break.

Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
feat(tmp): support other library kind for parameterized library
5d6b869 
This commit extends the support to find the root module from
the library name which must always exists locally because we
declare the implementation.

Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
fix: rebase remove useless print
010c233 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test: add implementation tests
c457847 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test: check both private and public lib work
c78c9b2 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
fix: change parameter public lib behavior
60ba6e7 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
chore(fmt): apply dune formatter
78b9abf 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
maiste and others added 15 commits September 8, 2025 09:57
@maiste @art-w
doc: add implementation documentation
5d67d9a 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
chore: fix error message for impossible case
74e9733 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test(promote): now export the main module (unique module)
c598b26 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
chore: add explnation comment for change
5d7bf1f 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
chore(review): apply @art-w review
bda4be2 
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test: check for nonexistent parameter
1f71be2 
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test: implement a non parameter library
741095e 
Signed-off-by: Etienne Marais <self@maiste.fr>
@maiste @art-w
test: apply promotion
0812eb6 
Signed-off-by: Etienne Marais <self@maiste.fr>
@shonfeder @art-w
remove [Library.is_parameter]
7fff187 
Signed-off-by: Shon Feder <shon.feder@gmail.com>
@shonfeder @art-w
documentation and comment fixes
e04a928 
Signed-off-by: Shon Feder <shon.feder@gmail.com>
@art-w
test: add more edge cases
85da315 
Signed-off-by: ArthurW <arthur@tarides.com>
@art-w
refactor: move module implements to compilation context
1f393fa 
Signed-off-by: ArthurW <arthur@tarides.com>
@art-w
refactor: replace usage of is_parameter by match kind
2049059 
Signed-off-by: ArthurW <arthur@tarides.com>
@art-w
fix after review
6874948 
Signed-off-by: ArthurW <arthur@tarides.com>
@art-w
refactor(virtual_rules): distinguish between vimpl and parameters
c9fa595 
Signed-off-by: ArthurW <arthur@tarides.com>
@art-w art-w force-pushed the maiste/library_param_impl branch from 3e40fdc to c9fa595 Compare September 8, 2025 11:15
art-w
art-w reviewed Sep 8, 2025
View reviewed changes
src/dune_rules/virtual_rules.ml
open Import
open Memo.O

let setup_copy_rules_for_impl ~sctx ~dir vimpl =
Copy link
Copy Markdown
Collaborator

@art-w art-w Sep 8, 2025

Choose a reason for hiding this comment

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

This was moved (unchanged) to Vimpl.setup_copy_rules: https://github.com/ocaml/dune/pull/12392/files#diff-654c4360f33ebd2b51644332b9d26dcf9d2e39b1a2125ae4bc9f6400a7cc412cR74

art-w
art-w reviewed Sep 8, 2025
View reviewed changes
src/dune_rules/virtual_rules.ml
"Library %s isn't virtual and cannot be implemented"
(Lib_name.to_string implements)
];
let+ vlib_modules, vlib_foreign_objects =
Copy link
Copy Markdown
Collaborator

@art-w art-w Sep 8, 2025

Choose a reason for hiding this comment

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

This was also moved (unchanged) to Vimpl.make: https://github.com/ocaml/dune/pull/12392/files#diff-654c4360f33ebd2b51644332b9d26dcf9d2e39b1a2125ae4bc9f6400a7cc412cR20

@shonfeder shonfeder requested a review from rgrinberg September 8, 2025 15:14
@art-w
Copy link
Copy Markdown
Collaborator

art-w commented Sep 8, 2025

I think this is ready for another review :) . I had to move large chunks of code in the last commit to clarify the implements field handling of virtual modules and library parameter, which makes this PR looks bigger than it really is -- I would be happy to submit that last commit as a follow-up PR if it's easier to review.

@shonfeder shonfeder enabled auto-merge (squash) September 9, 2025 14:54
@shonfeder shonfeder merged commit 2e0a649 into main Sep 9, 2025
1 of 2 checks passed
@shonfeder shonfeder deleted the maiste/library_param_impl branch September 9, 2025 14:54
@Sudha247 Sudha247 mentioned this pull request Oct 6, 2025
Closed
4 tasks
@art-w art-w mentioned this pull request Oct 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@art-w art-w art-w left review comments

@maiste maiste maiste left review comments

@rgrinberg rgrinberg rgrinberg approved these changes

@Alizter Alizter Awaiting requested review from Alizter

@ElectreAAS ElectreAAS Awaiting requested review from ElectreAAS

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[OxCaml] Parameterized libraries: parameter implementation

4 participants

@shonfeder @art-w @rgrinberg @maiste