Match compilers dummy loc #540
Conversation
patricoferris
requested review from
ceastlund,
NathanReb,
panglesd and
pitag-ha
as code owners
|
Seems some tests need to be updated as well! |
|
Also, rebasing it will reduce the diff as the quoter tests don't print out the whole AST anymore! |
patricoferris
force-pushed
the
match-compilers-dummy-loc
branch
2 times, most recently
from
a4e858c
to
4aad4e6
Compare
|
Turns out this actually changed in OCaml compiler versions, so this is not the proper fix.
The fix might be fairly easy (using Cinaps) but the problem becomes all of the expect tests. And whilst we could in theory do the |
|
I guess we could use [%%ignore] here, similar to what I did for the let x = [%expr ...]
[%%ignore]
let () = Pp_ast.Default.expression x
[%%expect {|
...
|}]Would that work for you? |
patricoferris
force-pushed
the
match-compilers-dummy-loc
branch
2 times, most recently
from
f1bcf8d
to
5936109
Compare
In OCaml 4.08 the dummy position was changed to have a pos_lnum of 0 instead of 1. We need to match whatever the compiler is using. Signed-off-by: Patrick Ferris <[email protected]>
patricoferris
force-pushed
the
match-compilers-dummy-loc
branch
from
5936109
to
0549bd1
Compare
|
It actually turns out I don't need that @NathanReb -- the files are only enabled for new enough compilers that it doesn't matter. But it is good to have that trick up my sleeve should we need it. Happy to make that change here to help future proof these tests (but I don't think it is likely for those bits to change too much). |
There was a problem hiding this comment.
If I understood the issue correctly, the important part is to agree with the compiler on what is Location.none right?
If so then we could probably simply stick to using the compiler's functions to build none locs.
This won't work when reading AST emitted by a different compiler (which we in theory support) but even that could be fixed by versioning the none loc.
As I'm writing this I wonder if the right fix isn't the following:
- Stick to having our stable
Location.noneand choose a convention for it - Have a function that detects whether a location is a valid
noneloc from any version we support - Use the function from
2instead of poly=in our codebase
What do you think?
| @@ -52,6 +52,10 @@ val set_input_lexbuf : Lexing.lexbuf option -> unit | |||
| val none : t | |||
| (** An arbitrary value of type [t]; describes an empty ghost range. *) | |||
|
|
|||
| val dummy_pos : Lexing.position | |||
There was a problem hiding this comment.
I'm not sure we should expose this as Astlib is supposed to represent the minimum stable API we'd need from the compiler and it seems that something like in_file would be enough here wouldn't it?
| @@ -8,7 +8,7 @@ type t = location = { | |||
| } | |||
|
|
|||
| let in_file name = | |||
| let loc = { pos_fname = name; pos_lnum = 1; pos_bol = 0; pos_cnum = -1 } in | |||
| let loc = { L.dummy_pos with pos_fname = name } in | |||
There was a problem hiding this comment.
I think this is equivalent to the compiler's in_file. If we used it instead, we wouldn't need the cinaps bits for dummy_pos anymore.
A fix as per #532.
I tested reverse dependencies (I think most of them, for their latest version) and they mostly installed
--with-test. Any that didn't seemed unrelated.