Extend marking to local open patterns #113

rleonid · 2016-08-19T15:52:02Z

This PR is a WIP to allow bisect_ppx to work on 4.04. The testing for the new local open module feature (ocaml/ocaml#578) isn't complete and unfortunately stalled because of this bug in the -dsource output.

Trying to fix issue #112

aantron · 2016-09-08T17:58:09Z

src/syntax/instrumentPpx.ml

@@ -208,6 +208,9 @@ let translate_pattern =
       it makes no sense to match a second time for effects such as
       exceptions. *)
    | Ppat_exception _ -> []
+    | Ppat_open (c, p') ->
+      translate mark p'
+      |> List.map (fun (marks, p'') -> marks, Pat.open_ ~loc ~attrs c p'')


The code change LGTM, but I would suggest that it be moved above the Ppat_exception case. Otherwise, it looks like the comment above applies to Ppat_open as well.

rleonid · 2016-09-19T13:54:42Z

@aantron Apologies for the delay, I was on vacation.

Updated the code per your requests and added tests. A couple of nits:

I'd suggest looking over the .reference, I did copy and paste the result and had a think about it. But this test is probably more important than the actual ppx-coverage rewriting instructions. I haven't started using these module open extensively (haven't switched to 4.04 yet), so a bit of a careful review would be helpful.
For some reason the new code doesn't get "covered" when looking at the result of make coverage... though it must be running.
I was a bit confused when my test code didn't compile (assert c = M.foo) and I couldn't figure out what was wrong. I don't know if you've had a similar experience but it felt like oUnit was being a bit obscure, but probably not worth doing anything.

aantron · 2016-09-22T22:50:25Z

No problem, hope it was a nice vacation :)

Concerning 1 and 2, we currently don't call the pattern rewriter on patterns of let expressions (we only do it for match, function, and try). So, that is why the Ppat_open case is not being executed. The reference looks right, but it's also not testing this code as per the above.

Separately, we should probably consider calling the rewriter on let, since it allows or-patterns. I guess now we are lucky that we took the repeated-matching approach (nesting a second generated match expression) instead of promoting or-patterns to cases, since let doesn't allow cases. Might want to also look through the grammar for any other uses of production pattern we may have missed.

Re. 3, I tried introducing an error, and yes it's confusing. I don't remember the output being this bad before. Perhaps open an issue for making it better?

This change breaks compilation for OCaml < 4.04, so perhaps we need to rewrite the pattern matching in translate_pattern to use ppx_tools ast_mapper_class?

rleonid · 2016-09-23T10:54:34Z

Now I understand. I couldn't help myself in extending the test case.

You make good points about the let differences. I wouldn't worry too much about 3, as long as the test-suite makes sense to you.

I'm not certain how to handle the 4.04 constraint, short of adding opam constraints on the latest bisect_ppx versions.ppx_tools isn't ready, and when I last looked at it for bisect_ppx it seemed like there were missing features, though I can't recall what they were at the moment.

aantron · 2016-09-26T16:52:20Z

Would pinning ppx_tools work for now, for development? It looks like this commit (and presumably its future back-port to 4.03) does what we want. IMO we should wait for ppx_tools to become ready, and put an opam constraint on that.

cc @alainfrisch

rleonid · 2016-09-26T17:06:17Z

Thinking through the problem a bit more ... how would using ast_mapper_class help us with translate_pattern ? Specifically, wouldn't we still have to handle each pattern (Ppat_* ) case in our code?

I'm probably missing something.

aantron · 2016-09-26T20:33:22Z

You're right, I misunderstood some ppx_tools code. We may have to add some kind of conditional compilation, or do parallel releases for >= 4.04 and < 4.04, unless ppx_tools adds something in Ast_convenience for dealing with this. Ast_convenience seems like the right way though, since we are probably not going to be the only ones with the problem.

rleonid · 2016-09-27T18:20:51Z

Ast_convenience seems like the right way though, since we are probably not going to be the only ones with the problem.

probably, but we'd have to mirror every pattern structure anyway, so why not use the AST directly?

or do parallel releases for >= 4.04 and < 4.04,

Sure. Is there something I can add to this PR ?

aantron · 2016-09-27T20:08:54Z

probably, but we'd have to mirror every pattern structure anyway, so why not use the AST directly?

If we don't use the AST directly here, it is ppx_tools which would have to do parallel releases instead of us, which ppx_tools already does anyway, due to AST incompatibilities between 4.03 and 4.02. I hope I am understanding your concern, though :)

Sure. Is there something I can add to this PR ?

I think it's probably best to open an issue on ppx_tools to see if it is likely to get Ppat_open compatibility functions implemented in Ast_convenience. I can also do a PR to ppx_tools later, in case you prefer that. Currently spending a lot of my open source attention on Lwt :/ Should probably put this PR on hold until after the ppx_tools discussion.

rleonid · 2016-09-27T20:22:04Z

put this PR on hold until after the ppx_tools discussion.

Fine with me, this is your wheelhouse.

aantron · 2016-11-01T13:48:50Z

This will take a while to resolve on the ppx_tools side (ocaml-ppx/ppx_tools#54 (comment)). So, I think we should resolve the compatibility problem temporarily within Bisect_ppx, then switch to ppx_tools' solution once it becomes available.

I suppose you could adapt/inline the proposal in ocaml-ppx/ppx_tools@1a192b0 and ocaml-ppx/ppx_tools@367ca8b, or solve it any other way (e.g. temporarily depending on cppo).

I added testing on 4.04 to Travis (https://travis-ci.org/aantron/bisect_ppx/builds/172287998), so you should have that if you rebase.

rleonid · 2016-11-07T04:16:52Z

@aantron Sorry for the delay on this one. I ended up using cppo to support multiple versions, and some hacking in the test suite but I think this would be a fine solution until the ppx libraries are ready.

aantron

Besides inline comments,

Squashing.. :)
I think we should edit the opam files of existing releases and add a constraint for compiler < "4.04". Care to do the honors? :)

aantron · 2016-11-07T17:06:09Z

tests/instrument/expr_match.ml

@@ -127,3 +127,4 @@ let f x =
 let f x =
  match x with
  | `Foo x | `Bar x -> x
+


Nit: is this adding a stray extra line?

aantron · 2016-11-07T17:16:19Z

tests/404_or_greater/expr_match.ml

+end
+
+let f () =
+  let M.(s) = M.(Foo) in assert (s = M.Foo);


I believe the only fully useful tests right now would be match expressions, where at least one of the cases has an or-pattern, i.e. M.(Foo | Bar), M.{i = 3 | 4; _}, etc.:

We aren't running the pattern rewriter on let expressions at all (Rewrite or patterns in let expressions #115).

If the rewriter generates only one case (because there are no or-patterns), we throw the case away and don't generate a nested match expression (

bisect_ppx/src/syntax/instrumentPpx.ml

Line 255 in d587132

| [marks, _] ->

).

True.
I'll add

let s = M.Bar in match s with | M.(Foo | Bar) -> assert true | _ -> assert false

Do you want me to get rid off all the other stuff? Though we don't rewrite the pattern I can make a hand-wavy argument that it is nice to have since one could still write that type of code.

I'd definitely convert all the syntax variants (M.{}, M.[], M.[||]) to match, so that we are actually testing those, and insert an or-pattern in more of them. I'd personally delay adding let cases until #155... but they don't really hurt, besides making the future diff for #155 slightly less clear. But nobody is going to read that diff anyway after it's in master, so up to you :)

aantron · 2016-11-07T17:19:53Z

tests/test_helpers.ml

@@ -58,6 +58,24 @@ let _run_int command =
  | Unix.WEXITED v -> v
  | _ -> _command_failed command

+let _read_lines_from ~max_lines command =


I think it's possible to use Sys.ocaml_version for this.

Learn something new everyday. I can't remember the reason why I've had to parse that output before.

aantron · 2016-11-07T17:24:22Z

tests/404_or_greater/test_404_or_greater.ml

+open Test_helpers
+
+let tests =
+  compile_compare (fun () -> with_bisect () ^ " -w +A-32-4") "404_or_greater"


Nit: I think we should name the directory and test set as "test_ppat_open" or "test_open_in_patterns", or something like that.

From the 4.04 feature list, it seems like the only new thing to deal with.

If something else gets added in 4.05 or later, it won't go into 404_or_greater.

rleonid · 2016-11-07T19:06:30Z

I'll squash past last comment about which tests to keep.
Regarding patching opam, might be easiest to do in PR to opam for a new version? I'd be glad to do it, but are there other things besides tagging that we follow to release a bisect_ppx ?

aantron · 2016-11-07T19:43:42Z

Nope, pretty much just write changelog and tag (I wrote RELEASE.md to remind myself :p ).

Since this release would include some slightly messy feature removals by me, would you like to split it up this way: I'll take care of the release notes/tag, and you can do the PR to OPAM, with both the new release and the updates to older opam files, as you propose?

On a tangent, what do you think about stopping to update the CHANGES file? Just put a note in there, saying to see the GitHub releaes page?

We use CPPO to support multiple OCaml versions. A special case, version dependent is added in the ppat_open directory.

rleonid · 2016-11-07T20:19:10Z

Squashed.

Since this release would include some slightly messy feature removals by me, would you like to split it up this way: I'll take care of the release notes/tag, and you can do the PR to OPAM, with both the new release and the updates to older opam files, as you propose?

Sure, I'll look out for the tag.

CHANGES.

I'm not too wedded to the file. But there is something nice to having the actual log be part of the source as opposed to being a github object.

aantron · 2016-11-08T18:35:29Z

Thanks.

Tag is pushed.

And reasonable – let's keep CHANGES, then.

aantron reviewed Sep 8, 2016
View reviewed changes

aantron mentioned this pull request Sep 26, 2016

Rewrite or patterns in let expressions #115

Closed

aantron mentioned this pull request Sep 28, 2016

Compatibility helpers for Ppat_open in Ast_convenience ocaml-ppx/ppx_tools#53

Closed

aantron mentioned this pull request Oct 29, 2016

Compatibility module for Parsetree.pattern_desc. ocaml-ppx/ppx_tools#54

Closed

rleonid force-pushed the 404 branch from 9b8e6d4 to 3fca4ea Compare November 6, 2016 23:54

aantron reviewed Nov 7, 2016

View reviewed changes

Extend marking to local open patterns.

3043caa

We use CPPO to support multiple OCaml versions. A special case, version dependent is added in the ppat_open directory.

rleonid force-pushed the 404 branch from ca8a904 to 3043caa Compare November 7, 2016 20:14

aantron merged commit 3043caa into aantron:master Nov 8, 2016

aantron mentioned this pull request Nov 8, 2016

build failure on OCaml 4.04 #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend marking to local open patterns #113

Extend marking to local open patterns #113

rleonid commented Aug 19, 2016 •

edited

aantron Sep 8, 2016

rleonid commented Sep 19, 2016

aantron commented Sep 22, 2016

rleonid commented Sep 23, 2016

aantron commented Sep 26, 2016

rleonid commented Sep 26, 2016

aantron commented Sep 26, 2016

rleonid commented Sep 27, 2016

aantron commented Sep 27, 2016

rleonid commented Sep 27, 2016

aantron commented Nov 1, 2016

rleonid commented Nov 7, 2016

aantron left a comment

aantron Nov 7, 2016

rleonid Nov 7, 2016

aantron Nov 7, 2016

rleonid Nov 7, 2016

aantron Nov 7, 2016

aantron Nov 7, 2016

rleonid Nov 7, 2016

aantron Nov 7, 2016

rleonid Nov 7, 2016

rleonid commented Nov 7, 2016

aantron commented Nov 7, 2016

rleonid commented Nov 7, 2016

aantron commented Nov 8, 2016

Extend marking to local open patterns #113

Extend marking to local open patterns #113

Conversation

rleonid commented Aug 19, 2016 • edited

Choose a reason for hiding this comment

rleonid commented Sep 19, 2016

aantron commented Sep 22, 2016

rleonid commented Sep 23, 2016

aantron commented Sep 26, 2016

rleonid commented Sep 26, 2016

aantron commented Sep 26, 2016

rleonid commented Sep 27, 2016

aantron commented Sep 27, 2016

rleonid commented Sep 27, 2016

aantron commented Nov 1, 2016

rleonid commented Nov 7, 2016

aantron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleonid commented Nov 7, 2016

aantron commented Nov 7, 2016

rleonid commented Nov 7, 2016

aantron commented Nov 8, 2016

rleonid commented Aug 19, 2016 •

edited