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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brendanzab
Copy link

@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.

Signed-off-by: brendanzab <bjzaba@yahoo.com.au>
@brendanzab brendanzab force-pushed the test_menhir__qualified_parsers branch from eb60772 to 658fcfa Compare November 18, 2024 12:18
@@ -0,0 +1,2 @@
(menhir
(modules parser))
Copy link
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
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
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
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
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
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!

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.

2 participants