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

Pparse: check ast invariants after preprocessing #1938

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
3 participants
@Octachron
Copy link
Contributor

commented Jul 28, 2018

This small PR proposes to check AST invariants after the preprocessing phase of the toplevel.

For instance, running a faulty ppx that generates an empty tuple with

let t = [%tuple];;

raises with this PR an error in the toplevel itself:

Error: broken invariant in parsetree: Tuples must have at least 2 components.

rather than an internal fatal error:

Fatal error: exception File "typing/typecore.ml", line 2999, characters 6-12: Assertion failed

@gasche

gasche approved these changes Jul 28, 2018

Copy link
Member

left a comment

We have discussed this before, and I agree it is a good idea. The implementation looks good.

@Octachron Octachron force-pushed the Octachron:toplevel_invariants branch from e70bbde to ad0dc2d Jul 30, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

I added a change entry (as a bug fix) since the change is potentially observable by users (or, more probably, for ppx developpers). I will merge once CI pass.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

I would placed the change entry in "Tools" (toplevel) or "Internal/compiler-libs changes" (where -ppx authors may also go look), but whatever.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Shouldn't this logic go to Pparse, to increase the likelihood that external tool also using the ppx infrastructure from compiler-libs would apply the same checks (assuming it is a good idea that they do!)?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

There already is logic in Pparse, but only in parse_file I think that the toplevel does not reuse. You're suggesting to push the checks in apply_rewriters, I suppose. I'll let @Octachron decides what he thinks is best.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Yes, exactly. On a slightly related note, the treatment of ImplementationHooks.apply_hooks seems suspicious to me: sometimes those hooks are applied before type-checking, sometimes after.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

From the point of view of external tools, I am not sure if adding an Ast_invariant check at the end of apply_rewriters always make sense, since the final consumer might not be the compiler?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

since the final consumer might not be the compiler?

It's a rather abstract discussion, but I view invariants as things that one would like to express in the Parsetree definition, but one cannot do it due to technical limitations (or it would make the code too heavy/irregular). Not particularly as expectations specific to the compiler. This is also reflected in the fact that attribute payloads are checked for invariants, even if the compiler doesn't do anything with them.

One expect in particular some kind of roundtripping property between the parser and printers (in concrete syntax), limited to Parsetree that respects the invariants. And this should hold also for "ppx" called by external tools, for debugging purposes.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

Fair enough. I am moving the invariant check to apply_rewriters.

@Octachron Octachron force-pushed the Octachron:toplevel_invariants branch 2 times, most recently from a5275cc to 20134eb Jul 30, 2018

@Octachron Octachron changed the title toplevel: check ast invariants after preprocessing Pparse: check ast invariants after preprocessing Jul 30, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

I have moved the invariants tests to Pparse.apply_rewriters and the change entry to the compiler-libs section.

@@ -183,7 +189,7 @@ let file_aux ~tool_name inputfile (type a) parse_fun invariant_fun
let ast =
Profile.record_call "-ppx" (fun () ->
apply_rewriters ~restore:false ~tool_name kind ast) in
if is_ast_file || !Clflags.all_ppx <> [] then invariant_fun ast;
if is_ast_file && !Clflags.all_ppx = [] then invariant_fun ast;

This comment has been minimized.

Copy link
@gasche

gasche Jul 30, 2018

Member

I find the logic for this line here fairly obscure. Could you just move the invariant_fun call to within the is_ast branch, so that it would test the invariants before the ppx extensions are run on the file? (This is assuming that checking the invariants is cheap, so that doing it twice is not a big deal.)

This comment has been minimized.

Copy link
@gasche

gasche Jul 30, 2018

Member

(Or if we don't want to check both before and after rewriting, you can still condition on all_ppx = [] within the branch.)

This comment has been minimized.

Copy link
@Octachron

Octachron Jul 30, 2018

Author Contributor

Maybe adding a comment on the logic and moving it to the is_ast branch is sufficient?

let ast = (input_value ic : a) in
  if !Cflags.all_ppx = [] then invariant_fun ast;
  (* if all_ppx <> [], invariant_fun will be called by apply_rewriters *)
  ast

This comment has been minimized.

Copy link
@gasche

gasche Jul 31, 2018

Member

Yes, it seems reasonable to me.

This comment has been minimized.

Copy link
@Octachron

Octachron Jul 31, 2018

Author Contributor

Done.

@Octachron Octachron force-pushed the Octachron:toplevel_invariants branch from 20134eb to 7d14788 Jul 31, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

I'm not sure whether the testsuite still passes with the changes to location-printing that may have hit trunk in between. @Octachron, could you do a rebase before I merge?

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

Sure

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

(As far as I can tell the HEAD has not changed and the CI is not rerun yet. I'm not sure if your message above means "I'll do the rebase later" or "I did the rebase but forgot to push".)

@Octachron Octachron force-pushed the Octachron:toplevel_invariants branch from 7d14788 to ae15cb0 Aug 6, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

It was the first option, the PR is now rebased.

@Octachron Octachron force-pushed the Octachron:toplevel_invariants branch from ae15cb0 to 591729b Aug 6, 2018

@gasche gasche merged commit d6ba0c9 into ocaml:trunk Aug 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.