-
Notifications
You must be signed in to change notification settings - Fork 99
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
PoC: support OCaml 5.3's keywords
entry in OCAMLPARAM
#535
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,7 @@ | ||
val is_keyword : string -> bool | ||
(** Check if a string is an OCaml keyword. *) | ||
|
||
val apply_keyword_edition : cli:string option -> unit -> unit | ||
(** Processes any keywords= sections from the OCAMLPARAM environment variable | ||
and CLI option and initialises the compiler's lexer with the correct keyword | ||
set. *) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
let () = Ppxlib.Driver.standalone () |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
(executable | ||
(name driver) | ||
(enabled_if | ||
(>= %{ocaml_version} "5.3")) | ||
(libraries ppxlib)) | ||
|
||
(cram | ||
(enabled_if | ||
(>= %{ocaml_version} "5.3")) | ||
(deps driver.exe)) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
This test can only work with OCaml 5.3 or higher. | ||
|
||
OCaml 5.3 introduced the new `effect` keyword. To allow old code to compile | ||
under 5.3 it also introduced a `-keyword=version+list` CLI option, allowing one to | ||
override the set of keywords. | ||
|
||
The ppxlib driver also has such an option now to properly configure the lexer before | ||
attempting to parse source code. | ||
|
||
Let's consider the following source file: | ||
|
||
$ cat > test.ml << EOF | ||
> let effect = 1 | ||
> EOF | ||
|
||
If passed to the driver as is, it will trigger a parse error: | ||
|
||
$ ./driver.exe --impl test.ml -o ignore.ml | ||
File "test.ml", line 1, characters 4-10: | ||
1 | let effect = 1 | ||
^^^^^^ | ||
Error: Syntax error | ||
[1] | ||
|
||
Now, if we use the 5.2 set of keywords, it should happily handle the file: | ||
|
||
$ ./driver.exe --keywords 5.2 --impl test.ml -o ignore.ml | ||
|
||
It can also be set using OCAMLPARAM: | ||
|
||
$ OCAMLPARAM=_,keywords=5.2 ./driver.exe --impl test.ml -o ignore.ml | ||
|
||
The priority between the CLI option and OCAMLPARAM must be respected, therefore | ||
both of the following invocation should parse: | ||
|
||
$ OCAMLPARAM=_,keywords=5.2 ./driver.exe --keywords 5.3 --impl test.ml -o ignore.ml | ||
|
||
$ OCAMLPARAM=keywords=5.3,_ ./driver.exe --keywords 5.2 --impl test.ml -o ignore.ml |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The problem is we can't make a dep on compilerlibs?
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.
I think the exposed version of this is a bit too high level and is used to parse the CLI args of a driver so it wouldn't work well for us. Maybe @dra27 can confirm?
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.
Right, it might be good to ask upstream to expose some of the internals if downstream libraries like ppxlib have to depend on that behaviour. Having just seen the issues caused in #540 by copying pieces of the compiler's internals into ppxlib, I think a long-term strategy of forcing the compiler to expose these things and help with long term stability would be nice?
Not a blocker for this PR because it seems the pieces we need are not necessarily exposed (but maybe should be? for the 5.3 release?).
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.
Yeah that's a good point! We should then at least move the bits we'd like exposed in the compiler to
Astlib
.@dra27 do you think we could reasonably expect something like
read_OCAMLPARAM
, returning a pair of args bindings lists, to be exposed at some point in the future?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.
Or maybe even
apply_keyword_edition
directly!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.
CC @dra27 any thoughts on that?
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.
It's definitely too late to be making compiler-libs changes for 5.3, but that obviously preclude doing that for 5.4.
I agree with the general concern at the duplication (I was concerned about it when hacking up the first version, too). However, it's worth remembering that it's extremely hard for OCaml itself to change the interpretation of the variable, so the details are very unlikely to change - i.e. we're duplicating a little bit of code which deals with a very non-internal detail of the compiler here.
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.
Yeah that's a good point!
Since we can't reasonably get this into 5.3, I'd say this PR is good to go! What do you think @patricoferris ?
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.
Yep! Good to go :))