Attach location to constants #12968
Conversation
Julow
force-pushed
the
parsing_constant_loc
branch
from
d532d97 to
57f715c
Compare
Constants don't have a location in interval patterns. This adds the location information on every constants. Co-authored-by: Guillaume Petiot <guillaume@tarides.com>
This uses a more precise location when one of the bounds is not a
character:
File "./test.ml", line 3, characters 9-10:
3 | | '1'..2 -> true
^
Error: Only character intervals are supported in patterns.
The entire pattern is highlighted when both are not characters:
File "./test.ml", line 3, characters 4-12:
3 | | "1".."2" -> true
^^^^^^^^
Error: Only character intervals are supported in patterns.
Julow
force-pushed
the
parsing_constant_loc
branch
from
57f715c to
381b664
Compare
gasche
added
the
parsetree-change
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
Sounds reasonable to me, but it will cause a lot of churn in the syntax-processing ecosystem (ppxlib and others).
| | {pconst_loc; _}, {pconst_desc = Pconst_char _; _} -> | ||
| pconst_loc | ||
| | _, _ -> loc | ||
| in |
There was a problem hiding this comment.
I find this approach a bit meh. I would define a function get_bound that takes a constant, returns the character if it is a Pconst_char, and fails with Invalid_interval otherwise.
There was a problem hiding this comment.
Much better like this!
| type constant = { | ||
| pconst_desc : constant_desc; | ||
| pconst_loc : Location.t; | ||
| } |
There was a problem hiding this comment.
The house style is to define the record first and the _desc type second.
There was a problem hiding this comment.
Changed to house style.
| | "-", Pexp_constant({pconst_desc = Pconst_integer (n,m); _}) -> | ||
| Pexp_constant(mkconst ~loc:sloc (Pconst_integer(neg_string n, m))) | ||
| | ("-" | "-."), Pexp_constant({pconst_desc = Pconst_float (f, m); _}) -> | ||
| Pexp_constant(mkconst ~loc:sloc (Pconst_float(neg_string f, m))) |
There was a problem hiding this comment.
Here are with mkuplus below, I think that the record pattern on the constant record should list all fields. Otherwise if we add new fields later (say to support attributes) the new fields will be silently dropped.
In fact there is already a bug related to dropping information in these functions today, -(1[@foo]) will drop the attribute.
There was a problem hiding this comment.
Changed! I can have a look to the dropped attribute in an other PR.
|
Thanks for the review!
I plan to work with a ppxlib maintainer into updating ppxlib. Unfortunately, that wouldn't help other users of the parsetree, though it changed a lot recently and didn't seem to have caused problems ? |
More readable interval typing code and styles changes.
Julow
force-pushed
the
parsing_constant_loc
branch
from
91bd338 to
dcb5b46
Compare
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
I'm happy with the current code and found the justification for the change reasonable -- this causes churn, but it brings the AST in the right direction, and is useful for ocamlformat.
I am planning to wait a few days to let other people chime in if they want to. @Julow, feel free to ping me again after that as I will inevitably have forgotten.
The special prefix operators [-], [-.], [+] and [+.] could rewrite:
-(1 [@foo])
into the constant:
-1
which drops the attribute. This adds the attribute check, the above
example is now parsed to the correct AST:
~-(1 [@foo])
An alternative implementation would be to drop the rewrite from the
parser and instead to perform it during a later phase.
Spotted by @gasche in ocaml#12968 (comment)
|
What about merging this? :)
|
|
Good idea, done. |
|
I'd like to chime in with two different comments/questions. I hope that's ok! The first one is a clarification: @Julow, about
Thank you! Having the author of a parsing change write the parsetree migration (or help writing it), is highly appreciated. I'd like to clarify one thing, though: Writing the migration is only one of the two ways in which a parsetree change impacts the PPX ecosystem. The second one is the following: Some/many PPXs will break when ppxlib bumps the exposed versioned parsetree to a version containing the change. As you've mentioned, @Julow, the parsing has changed a lot in 5.1 and 5.2 already. However, we haven't bumped the ppxlib parsetree, yet (for concrete reasons I could explain). So we can only hope that all the authors of the PPXs that will be affected once we do so, will adapt to the changes without too much resistance/complaints. That being said, I know that these kinds of parsing changes are important, so I'm not arguing against them. I just hope it's clear what we can expect about the impact of the sum of all these kinds of changes on the PPX ecosystem. And talking about "the sum of all these kinds of changes" brings me to my second point. You've mentioned
I expect that most of those patches bring the parsetree/AST closer to the original syntax, right? I remember at some point hearing ideas around adding a concrete syntax tree (CST) as a first abstraction to OCaml. So, we'd have: Syntax -> CST -> AST -> expanded AST (via PPXs) -> Typedtree. From what I understand, the CST would be very useful in different contexts handling syntax, such as formatting, refactoring etc. And for ppxlib and the PPX ecosystem, adding that extra first layer of abstraction would have the advantage that the necessary parsing changes would be absorbed by the CST, leaving the AST as is. @gasche , do you happen to have any info about that? Is that something that has been / might be discussed among the compiler maintainers, or is it just something I heard somewhere around? Or is the idea to make the AST itself more and more concrete step by step? (Ok, the PR has been merged 1 min ago, hope this comment still gets read :)) |
|
Indeed, it's a bit unfortunate that I merged at the same time you provided your (much-appreciated) feedback, sorry about that. Personally I am not at all opposed to the idea of having a CST/AST separation in the compiler, but I don't know any of the current heavy contributors who is planning to work on this in the medium term. If this effort were to be started, it would have to be by someone doing the heavy lifting. Also, I suspect that this would result in a substantial overhaul of the AST definition (larger than the present change) to regularize it, at least if the goal is to get a pleasant final design. |
|
Thanks for the clarification. I'll help as much as I can but if these changes are too annoying to the PPX maintainers, I'll stop proposing more. The CST idea was indeed a way to make changes to the parser that couldn't make sense to upstream into the AST (eg. arrow types as lists, But we also have changes that make total sense in the AST too and would benefit to other users: added locations, removal of elaborated nodes. I can publish these changes into a list (with the most useful/less intrusive first) into an issue. |
|
Gabriel Scherer (2024/03/11 08:16 -0700):
Indeed, it's a bit unfortunate that I merged at the same time you
provided your (much-appreciated) feedback, sorry about that.
My bad. I suggested to merge and only once I did do that it occurred to
me I could mention the PR to Sonja :(
|
Not at all, @shindere. Thanks for mentioning the PR to me, I would have missed it!
As mentioned, we (the PPX maintainers) really want changes, that are important for other tools or the compiler itself, to happen. The PPX situation shouldn't be blocking for an improving parsetree and surrounding tooling etc. We just need to be aware that those changes have more impact beyond writing the migration. And we'll need to observe how the PPX authors will react. Btw, as mentioned, we haven't bumped the ppxlib parsetree since 5.0 yet. IIUC, it seems very likely that 5.3 will contain a syntax addition reflected in the parsetree (effects syntax). If that's the case, for 5.3 we'll need to bump it for sure. So if you're planning to add more parsetree changes similar to this one, doing them before 5.3 would be better for the PPX ecosystem than after 5.3.
That sounds great. Thank you! Thank you, both @gasche and @Julow , for clarifying about the CST/AST separation!
Right, @gasche , this is a very good point that I had missed!
Interesting. @Julow , we seem to have different understandings about this. It makes sense what you're saying. If the CST ever gets approached, let's discuss it. If we end up having a CST, it'd be cool if it can be beneficial for as many things as possible. |
The special prefix operators [-], [-.], [+] and [+.] could rewrite:
-(1 [@foo])
into the constant:
-1
which drops the attribute. This adds the attribute check, the above
example is now parsed to the correct AST:
~-(1 [@foo])
An alternative implementation would be to drop the rewrite from the
parser and instead to perform it during a later phase.
Spotted by @gasche in ocaml#12968 (comment)
The special prefix operators [-], [-.], [+] and [+.] could rewrite:
-(1 [@foo])
into the constant:
-1
which drops the attribute. This adds the attribute check, the above
example is now parsed to the correct AST:
~-(1 [@foo])
An alternative implementation would be to drop the rewrite from the
parser and instead to perform it during a later phase.
Spotted by @gasche in ocaml#12968 (comment)
4 participants
This is a retry of #10544
In OCamlformat, we use a patched version of the parser as we need locations on every nodes among other things.
We would like to propose some of our patches in the hope that they are useful to the compiler and others and to help OCamlformat maintenance.
The second commit finds a use for the new location information and improves the unsupported interval error: