这是indexloc提供的服务，不要输入任何密码
Skip to content

Implicit source position erasing #63

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Apr 2, 2024
Merged

Implicit source position erasing #63

merged 15 commits into from
Apr 2, 2024

Conversation

@Enoumy
Copy link

@Enoumy Enoumy commented Mar 8, 2024 •
edited
Loading

This feature updated ocamlformat to adds erasing support for implicit source code positions!

It performs the following erasures:

let ~(LABEL : [%call_pos]) () = EXPR
(* into: *)
let ?(LABEL = Lexing.dummy_pos) () = EXPR
let _ : LABEL:[%call_pos] -> CORE_TYPE = EXPR
(* into: *)
let _ : ?LABEL:Lexing.position -> CORE_TYPE = EXPR

and

let _ = [%src_pos]
(* into: *)
let _ = Lexing.dummy_pos

The approach taken in this feature is to always expands [%src_pos] to Location.dummy_pos and not - say generate the record for the source of the actual location. Other possible approaches/solutions include that might be possible to switch this to are:

  • Expanding to [%here] (assuming that ppx_here will be accessible in the code that gets erased)
  • Directly expanding to the lexing_position record in-place. (I chose not to do this in the initial version due to the "equality" sanity check potentially being harder to implement as two equal "normalized" AST's could have different AST outputs if their parse locations are slightly different.)

Testing

I added tests for this in test/passing/tests/implicit_source_position_erased.ml following the pattern of local_erased.ml tests. Please let me know if there are any other tests I should create. Thanks!

@Enoumy Enoumy force-pushed the implicit-source-position-erasing branch 3 times, most recently from e908595 to 026b5d6 Compare March 8, 2024 20:17
@Enoumy Enoumy changed the title [DRAFT] Implicit source position erasing Implicit source position erasing Mar 8, 2024
@Enoumy Enoumy requested a review from goldfirere March 8, 2024 20:38
goldfirere
goldfirere approved these changes Mar 11, 2024
View reviewed changes
@Enoumy Enoumy force-pushed the implicit-source-position-erasing branch from 2fb9869 to 61281f0 Compare March 11, 2024 21:05
@goldfirere
Copy link

goldfirere commented Mar 12, 2024

I've reviewed the latest commit on "Fully qualified Lexing.position and Lexing.dummy_pos"

@goldfirere
Copy link

goldfirere commented Mar 13, 2024

Chatted with @alanechang briefly about this today in our 1:1. He pointed out that there is a ppx somewhere to delete probes when compiled upstream. This works well for probes, as they're encoded as extensions, with the [%...] syntax. We realized that a similar approach would also work well for implicit source positions: the upstream compiler will have no trouble parsing the new syntax, so a ppx could just do the transformation encoded in ocamlformat in this patch. I don't actually think minting a new ppx for public release is the right approach here -- too much overhead, I think -- but it was something we could have considered earlier in the process. To capture this notion, do you mind @Enoumy adding a comment near the additional code suggesting a ppx as a possible alternative? That way, if somehow the approach ends up causing trouble in the future, we'll know a possible way out. Thanks!

@Enoumy
Copy link
Author

Enoumy commented Mar 13, 2024

@goldfirere Oh that approach sounds interesting. I've added a comment summarizing my understanding of that approach. Please let me know if the comment is reasonable/if I should edit it. Thanks!

goldfirere
goldfirere reviewed Mar 15, 2024
View reviewed changes
@goldfirere
Copy link

goldfirere commented Mar 26, 2024

This looks all ready for merging. So sorry about the delay here!

Enoumy added 15 commits March 26, 2024 19:41
@Enoumy
Sanity ocamlformat transformation and also added tests for implicit
257ef4e 
source position erasing

Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Erased implicit source position arguments via [Lexing.dummy_pos]
0f1fd50 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Found and fixed a bug during self review.
bc4f6e8 
The bug was that (in the context of a type):

```ocaml
src_pos:[%src_pos] -> CORE_TYPE
```

was erased to:

```ocaml
src_pos:Lexing.position -> CORE_TYPE
```

where it should instead have translated to (an optional parameter):
```ocaml
?src_pos:Lexing.position -> CORE_TYPE
```

Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Renamed src_pos -> call_pos argument
5b9954f 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Removed unnecessary change found during self review
ce34d32 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Updated out-of-place comment during self-review
f4c9235 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Removed lingering test cases
c64ca39 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Added test that does not pass erased flag.
bb699e3 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
De-duplicated duplicated implicit_source_position.ml file by changi…
353a07e 
…ng suffix to use kebab case.

Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Fully qualified Lexing.position and Lexing.dummy_pos
9be1bad 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Refactored erasing approach with at-parse-time [mly] approach.
5601bc0 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Removed unintended whitespace change
2fd98f8 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Added note with alternate approach.
4b66d42 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Added test with locals
1ac4830 
Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy
Changed extended parser.mly to perform erase check before pattern mat…
bee3cd8 
…ching deeply on the syntax.

Signed-off-by: enoumy <enoumy@gmail.com>
@Enoumy Enoumy force-pushed the implicit-source-position-erasing branch from 5af408b to bee3cd8 Compare March 26, 2024 23:41
@Enoumy Enoumy merged commit 912e075 into oxcaml:jane Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goldfirere goldfirere goldfirere approved these changes

@dkalinichenko-js dkalinichenko-js dkalinichenko-js left review comments

+1 more reviewer

@alanechang alanechang alanechang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Enoumy @goldfirere @dkalinichenko-js @alanechang