-
Notifications
You must be signed in to change notification settings - Fork 98
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
Bump internal AST to 4.10 #130
Conversation
9bd921a
to
68cc0a2
Compare
@ceastlund are you happy with using |
If these are just synthetic paths used for error messages and the like, I don't have a strong opinion on how we represent them. Using the same placeholder they're declared with seems fine. |
Cool. @trefis could you confirm that you are happy with the changes to the location checks? Otherwise, I had a quick look and the rest of the changes look good to me. |
Yep, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I just pushed a fix, the CI was broken for OCaml < 4.08 |
The appveyor builds seem to fail to install cinaps for some reason. |
My guess is that it relies on a behaviour of the stdlib that was broken before 4.08. Let's drop AppVeyor testing for OCaml < 4.08. |
Maybe it's worth including #148 into this PR then! |
Seems good |
Just to make sure we're all on the same page, the tests are now only run for 4.10. As I mentioned in the last commit's message, our current test suite can't be compatible with 4.09 or lower since there are differences in how ASTs get printed. |
Yes. It seems too much of a pain to keep the testsuite compatible with multiple versions of the compiler, especially with the compiler libs behaviour changing all the time. |
One last thing to fix and then we should be good to go: the 4.10 builds on appveyor seem to fail when running cinaps, something about a permission denied:
|
Can we disable cinaps on appveyor? We only need it to run once. |
That means disabling the tests on appveyor though, not sure there's a better approach to this unfortunately. |
It is what it is. ppxlib doesn't do much that is system dependent so it's not the end of the world. And it's only until we can figure out what's going on with cinaps on Windows. |
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
Unfortunately, due to differences in how ASTs are printed between 4.09 and 4.10 we can only test 4.10. We could eventually try to make the tests compatible between those versions, I'm wondering how often this is susceptible to break. Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
CHANGES: - Bump ppxlib's AST to 4.10 (ocaml-ppx/ppxlib#130, @NathanReb) - Remove omp_config from `Expansion_context` and replace it with `tool_name` (ocaml-ppx/ppxlib#149, @NathanReb) - Change undocumented `Ppxlib.Driver.map_structure` to return a ppxlib's `structure` instead of a `Migrate_parsetree.Driver.some_structure`. (ocaml-ppx/ppxlib#153, @NathanReb)
Closes #129
Besides the straightforward changes, there are a couple things worth noting:
map_with_path
andmap_with_ctxt
classes now need to deal with module bindings and module declarations without ids, i.e. things likemodule _ = ...
. For now I choosed to add the underscore to the path in both cases so that the paths would differ between the inside and the outside of an unnamed module, ie in:x and y would have different paths. That means that now one could end up with e.g.,
A.B._.c
as a path. That's not perfect and I'm happy to change that but I think paths need to carry that information somehow. Maybe we could try to identify them using their location in the file somehow, something like:A.B.<unnamed module at line x>.c
.As a follow up I'll look into our tests and make sure they are ran with the latest versions of OCam rather than with 4.06 and 4.07 as they currently are! I ran the test locally and all seems fine! If we improve the situation with unnamed modules as mentioned above, might be worth adding tests for it though.