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

makefile rule to test parser changes by comparing ASTs stored in `.ml.ast` files #2030

Merged
merged 2 commits into from Sep 8, 2018

Conversation

Projects
None yet
6 participants
@gasche
Copy link
Member

commented Sep 7, 2018

See #2029:

We need a way to test AST-preserving parser changes to get assurance that silly mistakes (sometime hard for the human eye to spot) will be caught. I don't think I can resurrect the test technique that I used during the Menhir transition, because it relies on linking two different parser modules; that would mean asking users to play patchwork with an old and new grammar, and nobody wants that.

I have an idea which is to add a Makefile rule: %.ml.ast: %.ml, which would dump the -dparsetree output of the in-trunk compiler into the .ast file. [Then compare those files before and after parser changes]

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

Proposed (and tested) workflow:

(Edit: see below for more recent workflow.)

# save pre-change ASTs
make -j build-all-asts
find . -name '*.ml.ast' | xargs git add

# do your parser changes
# ...
make promote-menhir

# compare new ASTs
make -j build-all-asts
git diff # shows any .ml.ast difference

# remove AST files
find . -name '*.ml.ast' | xargs git reset HEAD
find . -name '*.ml.ast' | xargs rm
@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Should we be checking in the old .ast file into the repository and having a formal test? Then it would be easy to make changes. If you meant for the .ast file to change, you would of course modify it as part of your updates.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

I would rather not, because source files change frequently and I believe this would cause too much churn to be comfortable. (In the world of git, reviewing and managing the list of changing files are common operations; doubling the number of changed files would have a large usability cost.)

Makefile Outdated
@@ -1308,6 +1308,23 @@ partialclean::

beforedepend:: bytecomp/opcodes.ml

# Testing the parser -- see parsing/HACKING.adoc

SOURCE_ML_FILES=$(shell git ls-files | grep '\.ml$$')

This comment has been minimized.

Copy link
@nojb

nojb Sep 8, 2018

Contributor

can replace this by $(shell git ls-files *.ml)

This comment has been minimized.

Copy link
@gasche

gasche Sep 8, 2018

Author Member

I tried git ls-files *.ml in my terminal and it doesn't work, but now that you say it git ls-files '*.ml' does work -- I guess you have to present the shell from globbing the argument first. I will change this, thanks!

describing the parsed abstract syntax tree (AST) in `-dparsetree`
format.

This rule is rather slow to run, and can safely be run in paralle, so

This comment has been minimized.

Copy link
@nojb

nojb Sep 8, 2018

Contributor

parallel

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2018

I decided to extend build-all-asts to also the .mli files in the compiler distribution, and discovered that -stop-after parsing is not currently taken into account when processing .mli files; this is fixed by #2032.

@gasche gasche force-pushed the gasche:ml.ast branch 2 times, most recently from bc0db71 to e024eee Sep 8, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2018

@nojb: I applied your comments, added .mli file testing, and a list-all-asts target to make manual scripting easier for users.

@fpottier

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2018

Sounds cool, very useful!

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2018

With the list-all-asts rule, the new workflow looks as follows:

# save pre-change ASTs
make -j build-all-asts
make list-all-asts | xargs git add

# do your parser changes
# ...
make promote-menhir

# compare new ASTs
make -j build-all-asts
git diff # shows any .ml.ast difference

# remove AST files from the index
make list-all-asts | xargs git reset HEAD

# remove the files (if no further parser change planned)
make list-all-asts | xargs rm

SOURCE_FILES=$(shell \
git ls-files '*.ml' '*.mli' \
| grep -v '^experimental/')

This comment has been minimized.

Copy link
@nojb

nojb Sep 8, 2018

Contributor

Off-topic in this PR, but I wonder if there is a good reason to keep this in the tree nowadays.

This comment has been minimized.

Copy link
@gasche

gasche Sep 8, 2018

Author Member

I will send a PR to remove it. (Would you by chance 'approve' of the present one?)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 23, 2018

Member

IIRC @shindere tried to remove things from experimental but some of the owners of those files pushed back. Maybe it's time to apply more pressure.

This comment has been minimized.

Copy link
@nojb

nojb Nov 23, 2018

Contributor

In fact, experimental was completely removed in da5c22a

@nojb

nojb approved these changes Sep 8, 2018

Copy link
Contributor

left a comment

LGTM

@gasche gasche merged commit cbae94c into ocaml:trunk Sep 8, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2018

Thanks! Merged.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

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.