Skip to content

test: Sibling modules should be accessible from qualified parsers#11118

Merged
anmonteiro merged 2 commits intoocaml:mainfrom
brendanzab:test_menhir__qualified_parsers
Dec 28, 2025
Merged

test: Sibling modules should be accessible from qualified parsers#11118
anmonteiro merged 2 commits intoocaml:mainfrom
brendanzab:test_menhir__qualified_parsers

Conversation

@brendanzab
Copy link
Copy Markdown
Contributor

@brendanzab brendanzab commented Nov 13, 2024

This PR adds a failing test regarding the current behavior of (include_subdirs qualified) when a Menhir parser attempts to refer to a sibling module, as reported in #11119.

@brendanzab brendanzab force-pushed the test_menhir__qualified_parsers branch 2 times, most recently from eb60772 to 658fcfa Compare November 18, 2024 12:18
@@ -0,0 +1,2 @@
(menhir
(modules parser))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From what I see the error message is correct, as this makes the Ast module invisible to Menhir.

Copy link
Copy Markdown
Contributor Author

@brendanzab brendanzab Nov 18, 2024

Choose a reason for hiding this comment

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

Oh, why is that? I had assumed it would work similarly to other qualified modules - where you could mention sibling modules from within a subdirectory? See the directory tree from #11119:

├── dune
├── dune-project
├── foo.ml
├── lang
│   ├── ast.ml
│   ├── dune
│   └── parser.mly
└── run.t

If this is intentional is there a workaround for this, or is this pattern supposed to be impossible with Menhir?

Copy link
Copy Markdown
Contributor Author

@brendanzab brendanzab Nov 18, 2024

Choose a reason for hiding this comment

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

I’ve also tried the following from the root dune file:

(subdir lang
 (menhir
  (modules parser)))

Which resulted in the same error.

I also tried:

(menhir
 (modules lang/parser))

Which resulted in:

Error: dune__exe__Lang/parser corresponds to an invalid module name
-> required by _build/default/.foo.eobjs/dune__exe.ml-gen
-> required by _build/default/.foo.eobjs/byte/dune__exe.cmi
-> required by _build/default/lang/parser__mock.mli.inferred
-> required by _build/default/lang/parser.ml
-> required by alias lang/all
-> required by alias default

Copy link
Copy Markdown
Contributor Author

@brendanzab brendanzab Nov 20, 2024

Choose a reason for hiding this comment

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

Would you prefer it if I used an example that used one of the approaches above, or are those also intended behavior?

I think the main reason I used a dune file in the subdirectory was because I had no other way of referencing the .mly from the root directory via the menhir stanza. And I assume using the subdir stanza is isomorphic to the approach I am currently using (but I could be mistaken on that).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I’ve also tried variations of (modules Lang.Parser), (modules Lang__Parser), (modules dune__exe__Lang__parser) to no avail in the menhir stanza.

Copy link
Copy Markdown
Contributor Author

@brendanzab brendanzab Dec 14, 2024

Choose a reason for hiding this comment

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

@Leonidas-from-XIV If it helps, it seems #8987 was merged, which seems like a case where the .mly is the “group interface” of the subdirectory, which references a sibling module:

├── dune
├── dune-project
└── group
    ├── dune         (menhir stanza is here)
    ├── group.mly    (refers to M)
    └── m.ml

https://github.com/ocaml/dune/pull/8987/files

In contrast the pattern I am after is:

├── dune
├── dune-project
└── group
    ├── dune         (menhir stanza is here)
    ├── m.ml
    └── parser.ml    (refers to M)

Not sure if that test is enough to covers this use-case case as well, as in this case the parser.mly is not the group interface? But it’d be really cool if that issue was fixed, this pattern would be supported as well!

brendanzab and others added 2 commits December 28, 2025 14:56
Signed-off-by: brendanzab <bjzaba@yahoo.com.au>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the test_menhir__qualified_parsers branch from 658fcfa to 0af5980 Compare December 28, 2025 23:26
@anmonteiro
Copy link
Copy Markdown
Collaborator

This seems worth fixing. I pushed a commit converting the test into a single file.

@anmonteiro anmonteiro merged commit 301fd9c into ocaml:main Dec 28, 2025
24 checks passed
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.

3 participants