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

Parser: fix bug #7847. #2019

Merged
merged 4 commits into from Sep 6, 2018

Conversation

Projects
None yet
3 participants
@fpottier
Copy link
Contributor

commented Sep 5, 2018

A change in the grammar removes the need for raising Parsing.Parse_error in the semantic action, and should fix bug #7847. This PR also passes --no-stdlib to Menhir, for greater robustness in the face of changes in Menhir's standard library.

fpottier added some commits Sep 4, 2018

Parser: introduce a generic definition of [reversed_separated_nonempt…
…y_list]

and use it to give a more concise definition of [core_type_comma_list]. This
should make no difference whatsoever: the generated LALR(1) automaton should
be the same (up to some possible differences in naming).
Parser: in the definition of [simple_core_type], replace
[core_type_comma_list] with [core_type], as this is what is really intended:
allow a single type. This removes the need for raising [Parsing.Parse_error]
in the semantic action, and should fix bug #7847. This change alone would
create a conflict in the grammar, which is avoided by replacing
[core_type_comma_list] with [inline_core_type_comma_list] in two places.
Show resolved Hide resolved parsing/parser.mly
Show resolved Hide resolved Makefile.menhir Outdated
@trefis

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

@fpottier could you add a regression test (and a changelog entry)?

@gasche

gasche approved these changes Sep 5, 2018

Copy link
Member

left a comment

This PR comes from an email discussion with @let-def, @fpottier and myself. I have reviewed it, believe it is correct and (less obvious) understand why it fixes the issue -- the problem comes from a bad interaction between Menhir's execution loop and the trick of raising a parser-internal exception from a semantic action, here raise Parsing.Parse_error, now removed from simple_core_type.

I approve the PR, modulo clarity/presentation questions discussed with @fpottier and @trefis. We should merge once we have converged to something that people like.

@fpottier, could you also include a Changes entry? Cite the MPR and the GPR number, the reporter is "Stefan Muenzel" -- I would put Thomas, Frédéric and myself as reviewers.

Show resolved Hide resolved parsing/parser.mly
Show resolved Hide resolved Makefile.menhir Outdated
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

The regression test could be

external x : unit -> (int,int)`A.t = "x"

from MPR#7847. You could create a new file pr7847.ml in testsuite/tests/parse-errors, and list it in the ocamltests file in that directory. (from testsuite, use make one DIR=tests/parse-errors and check that it is listed in the test results.) The usual protocol for expect tests is to use an empty [%%expect{| |}] expect block when you write the test, run it, and complete from the failure message (or: make promote DIR=tests/parse-errors).

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@gasche: using lists that return elements in the right order and removing List.rev at the use site is indeed the right thing to do in the long run, but (I think) this should be done in another pull request whose focus is on improving the style of the grammar.

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@gasche: the Changes file already has an entry "GPR#292: use Menhir as the parser generator for the OCaml parser". The change proposed here fixes a bug that was never exposed in a release, so I don't think it deserves its own entry, does it?

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Having a change entry in the "Bug fixes" section is low-profile and it is also a way to credit your work and the work of the bug reporter.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I see your point about having a separate PR to reverse lists, that sounds reasonable to me.

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

I have added a Changes entry and a regression test.

@trefis

trefis approved these changes Sep 5, 2018

Copy link
Contributor

left a comment

LGTM assuming travis is OK.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Thanks! Good to merge if CI passes. (I'm still not convinced by --no-stdlib for the reasons given but we can always remove the flag later if the need arises.)

@fpottier: if by chance you were mildly motivated by the idea of factorizing other parts of the grammar, or getting rid of these nagging List.rev that are easy to forget, please feel free to send further PRs!

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

Concerning --no-stdlib, you (@gasche) are right: I was trying to protect against future evolutions of Menhir's standard library, and there is no reason why this burden should lie upon users of Menhir. In fact, I was too pessimistic: Menhir already has a mechanism for renaming user symbols when they have the same name as a symbol in the standard library. So, I will remove this commit from the PR.

@fpottier fpottier force-pushed the fpottier:trunk branch from 2979cbf to a1d90ac Sep 5, 2018

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

Commit removed.

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@gasche: I will indeed do some cleanup work in my spare time, beginning with list forms.

A question: could you explain (and perhaps document somewhere) the distinction between boot/menhirLib.{ml,mli} and parsing/camlinternalMenhirLib.{ml,mli}?

Also, why does make test-menhir include a sed command to rename MenhirLib to CamlinternalMenhirLib, whereas make promote-menhir does not have such a command? In my limited experience, the renaming seems needed in both cases.

@gasche gasche merged commit b155f28 into ocaml:trunk Sep 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

I made the choice to store in boot/ exactly the Menhir runtime library and menhir-generated parsers, unchanged. The transition from MenhirLib to CamlinternalMenhirLib happens in the parser-related rule of Makefile (not Makefile.menhir, because these rules don't depend on having Menhir installed and are part of the core build): we rename menhirlib.ml{,i} and sed within parser.ml{,i} when moving them from boot/ to parsing/.

The promote-menhir rule copies from a temporary directory (which happens to be parsing/) to boot/ the unchanged source files. Replacement is done on the way "back": when building parser.ml, the boot/ file is more recent and is copied over.

The test-menhir boot does not affect the boot/ directory at all, it produces the parser and compiles it in place in parsing/. It must do the translation itself.

I may add a comment to explain this in Makefile.menhir.

@fpottier: note that an option in Menhir to change the name of the MenhirLib module would be convenient. (Note that starting the .mly header with an module MenhirLib = Foo alias does not work as some other code is generated before it.) The use-case is pretty arcane: it is required if you want to link two parsers generated with distinct versions of Menhir in the same program. But it may still make sense for anyone developing software that (1) uses Menhir and (2) lets user build and link arbitrary plugin (a plugin may use a different Menhir version), so at least Why3 and Coccinelle and Coq could be in the potential user base.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

I added more document comments in 5191251. Or, shall I say, "Makefile.menhir has been pottierized".

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@gasche: thanks, good suggestion, duly noted.

@fpottier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

Excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.