-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes for 4.07 #23
Fixes for 4.07 #23
Conversation
The first patch is clearly necessary correct, and remains compatible with older OCaml versions. Thanks! The second patch indeed is worrying. We probably have to put it under a version-condition, as it will break under older systems, and it also suggests that everyone using @damiendoligez, could you split the two patches into two PRs, so that I merge the first and worry about the second one? |
I'm not very familiar with way ppx_import works, but I'm surprised the second commit is needed. By following the aliases in the typing environment one should automatically end up to |
I'm not very familiar either, but I think we miss the "following aliases in the environment" component, and that indeed (assuming it isn't already done) would be a more robust solution to the problem. For reference, the current environment-lookup code is there, but I haven't analyzed it yet. |
@gasche You most likely nailed it. It does not look like we are handling type aliasing cases when parsing the CMIs in the current environment. That's a missing feature; the second patch is not needed. |
If I read the code correctly, ppx_import doesn't type the input at all and simply assumes that in |
Without changing everything, a simple solution would be to ask the compiler libs for the initial environment, and then do the lookup in this environment rather than read the cmi file manually. |
Correct.
I believe this is the right solution. |
f1f7272
to
71ccc38
Compare
71ccc38
to
f03af27
Compare
I've removed the first commit from this PR and kept the problematic one, since the discussion about it is happening here. I've submitted #24 with the non-problematic commit. |
I made a first attempt this morning at using the type-checker to resolve paths. This doesn't completely work yet (WIP). The first attempt is the commit daf4cd5, which builds an initial typing environment when This first patch does not pass the testsuite, because not all stuff.ml:
use.ml:
Querying a member of a type signature in this way is not valid in |
In #25 I propose a more elaborate implementation that uses the type-checker in the global environment, but also supports sub-module-type names in paths as ppx_import does today. |
This can be closed now that #25 has been merged. |
For
Hashtbl
in src_tests, I'm not sure this is the best fix. You should ask @diml or the other contributors of ocaml/ocaml#1010.