Skip to content
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

Add locations to attributes in the parsetree #1953

Merged
merged 3 commits into from Aug 6, 2018

Conversation

Projects
None yet
6 participants
@hhugo
Copy link
Contributor

commented Aug 1, 2018

Change the representation of attributes to include the location.
Note that cmis use the same representation of attributes and make the bootstrap mandatory (IIUC)

This additional location will be useful to ocamlformat when deciding where to print comments.
(also, someone had left a TODO about it in the parser: (* todo: keep exact location for the entire attribute *))

@hhugo hhugo changed the title Add locations to attributes Add locations to attributes in the parsetree Aug 1, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

I wondered, so I looked: I believe that this is complementary to #1903. #1903 is about making sure that all nodes that can carry attributes themselves have a location, while this one stores a location for the attributes only. Both sound like sensible things to have.

@hhugo would you maybe just mention why in particular you decided that you needed this feature?

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

I've updated the description.
Note that another PR is on its way to add locations to directives.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

This additional location will be useful to ocamlformat when deciding where to print comments.

I assume this is to be allow to decide whether a comment goes before, inside or after an attribute?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

We already have a location attached to attributes, but it is currently the location of the attribute identifier. I agree that generally, it is more useful to have access to the location of the whole attribute, with its payload. But do we need both? If we decided to reuse the existing location field, one would avoid changing the Parsetree definition, which is always a bit tedious for users. I'm not against the current proposal, though, just proposing a possible alternative.

Also, it would be good to provide a high-level description of the general direction in terms of adding locations. I suspect that ocamlformat would also benefit to knowing the location of keywords such as in. Will we add locations for keywords?

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

I don't have any other plan to add locations at the moment (@jberdine, anything else ?)

It's not clear that having both the locations of the identifier and the whole attribute is useful but I'm kind of uncomfortable with the idea of abusing the existing string Location.t. I would imagine that in existing occurrences of 'a Location.t, the loc is the location of 'a (or Location.none) and not an "extended" location.

@hhugo hhugo force-pushed the hhugo:attr branch from 46e7a4b to b13d22a Aug 1, 2018

@jberdine

This comment has been minimized.

Copy link

commented Aug 1, 2018

The use of locations made by ocamlformat is to determine which piece of code to which each comment refers. The current idea is that comments are written to explain something about the code, and so at this point there is no plan/desire to associate them with keywords or other punctuation, as it seems unlikely that a code author will want to comment on a keyword.

As for having the locations of both the attribute identifier and its payload, I'm not sure but isn't it conceivable that a ppx that has constraints on the form of the payload may want to issue an error to the user, at which point being able to point to the payload itself may be useful.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

I think it's best to keep existing locations on the attribute name and the payload being actually about those things, and not abuse them somehow to represent the whole attribute. Both locations are useful (the payload for obvious reasons, the identifier for typo-catching reports, such as offered by ppxlib). I also agree with @gasche's assessment of the relation to #1903.

Given all that, I think the current patch is perfectly fine. The change will not hurt anyone (yay ocaml-migrate-parsetree) and seems clearly motivated. The implementation is of the kind where the typechecker is the best reviewer, so I'm in favor.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

as it seems unlikely that a code author will want to comment on a keyword.

Understood, but consider:

let x =
   foo bar
   (* this comment refers to the "foo bar" expression above *)
in
let y =
   foo bar
in
(* this comment refer to the code below *)
....

knowing whether the comment comes before or after the in might be useful to decide whether it refers to the let-bound expression or the body.

@jberdine

This comment has been minimized.

Copy link

commented Aug 2, 2018

Agreed, definitely. It sounds like quite a significant change to the parsetree, which I don't have time to do. Having such locations would likely provide a more principled replacement implementation for some current heuristics such as checking if there is only whitespace in the concrete syntax between a comment and a phrase with a location.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

I think that the scope of the current PR is fine (well-defined and useful), and I think there is consensus to have both locations (on the attribute name and for the whole payload), but this PR needs a review if it wants to move forward. Would someone be willing to review the code carefully?

@Drup

Drup approved these changes Aug 6, 2018

Copy link
Contributor

left a comment

I forgot to go through the github thing, but I reviewed (as I said, it's mostly a patch-by-typechecker) and my last message was an Approval. :)

hhugo added some commits Jul 21, 2018

@hhugo hhugo force-pushed the hhugo:attr branch from b13d22a to c2f2a09 Aug 6, 2018

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

changelog updated, commit squash.
I'm guessing this needs to be merge manually to redo the bootstrap.

@gasche

gasche approved these changes Aug 6, 2018

Copy link
Member

left a comment

(adding the green stamp to Drup's review)

@gasche gasche merged commit 04123df into ocaml:trunk Aug 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Well now I forgot about the manual merge, I'll revert the bootstrap and redo it manually.

(This also needs a rebase of #292...)

@hhugo hhugo deleted the hhugo:attr branch Aug 6, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Previous bootstrap reverted in 795dff7, clean bootstrap in 0f9cd35.

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

Thanks

gasche added a commit to gasche/ocaml that referenced this pull request Aug 19, 2018

gasche added a commit to gasche/ocaml that referenced this pull request Aug 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.