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

parser: better locations for type constraints #1850

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

commented Jun 20, 2018

No description provided.

@trefis trefis requested review from gasche and let-def Jun 20, 2018

@trefis trefis force-pushed the trefis:better-locs branch from 954a759 to 794cb3c Jun 20, 2018

package_type:
module_type { package_type_of_module_type $1 }
package_type_desc { ghtyp $1 }

This comment has been minimized.

Copy link
@let-def

let-def Jun 20, 2018

Contributor

Why does the location have to be ghost?

This comment has been minimized.

Copy link
@let-def

let-def Jun 20, 2018

Contributor

Since the location is now correct, it might be the time to unghost it :).

This comment has been minimized.

Copy link
@trefis

trefis Jun 20, 2018

Author Contributor

I believe you're right: we can unghost this. I used ghtyp because all the callers did so, I think they were all incorrect.

@gasche
Copy link
Member

left a comment

Seems nice to me, see minor comments.

match loc with
| None -> symbol_gloc ()
| Some l -> { l with loc_ghost = true }
in

This comment has been minimized.

Copy link
@gasche

gasche Jun 20, 2018

Member

Can you factorize this match into a ghost ?loc () function?

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

Will do.

{ loc_start = rhs_start_pos 1
; loc_end = rhs_end_pos 5
; loc_ghost = true }
in

This comment has been minimized.

Copy link
@gasche

gasche Jun 20, 2018

Member

This level of expliciteness works for me, but note that a rhs_interval helper that would take two numbers and not set the ghost boolean would also work well here -- given that the ghost aspect is set by the gh* functions below anyway.

But do these really need to be ghost locations here? Is the idea that ident : a b . ty is not a syntactically valid pattern, so this should be ghost? Because intuitively it seems a valid location for a "pattern" that maps to a real part of the program that the user wrote. Same for typloc, which seems reasonable to me.

(I have forgotten what is the implicit contract of non-ghost locations.)

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

I added rhs_interval alongside rhs_loc in Location.

As for the ghosting I have some opinion on the matter, but I'm unwilling to take any action at this point. I'd rather keep this PR simpler and have a look at that at a later point.
As was mentioned in some email, there is a plan of adding a pvb_type field to Parsetree.value_binding instead of synthesizing constraints on the pattern and sometimes the expression.

This comment has been minimized.

Copy link
@gasche

gasche Jun 21, 2018

Member

Even if you want to keep these locations ghost in the produced AST for simplicity, I was pointing out that I think the with loc_ghost = true part (in the new patchset) is useless, because the ~loc argument of ghtyp/ghpat is going to get ghosted within the function anyway.

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

Ah indeed! I'll simplify that.

@@ -2294,13 +2329,16 @@ simple_core_type2:
{ mktyp(Ptyp_variant(List.rev $3, Closed, Some [])) }
| LBRACKETLESS opt_bar row_field_list GREATER name_tag_list RBRACKET
{ mktyp(Ptyp_variant(List.rev $3, Closed, Some (List.rev $5))) }
| LPAREN MODULE ext_attributes package_type RPAREN
{ mktyp_attrs (Ptyp_package $4) $3 }
| LPAREN MODULE ext_attributes package_type_desc RPAREN

This comment has been minimized.

Copy link
@gasche

gasche Jun 20, 2018

Member

I'm not fond of having _desc show up in a production, personally I think that it makes the production slightly less readable (_desc should only be internal wrapping). I would rather use package_type everywhere and wrap_typ_attrs in the semantic action here.

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

I had missed that wrap_typ_attrs, thanks a lot!

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

Actually, that change makes the locations slightly worse.
For:

type t =
  (module%foo[@foo] M)

I get this diff on the parsetree:

           Some
             core_type ([2,2]..[2,22]) ghost
               Ptyp_extension "foo"
-              core_type ([2,2]..[2,22])
+              core_type ([2,20]..[2,21])
                 attribute "foo"
                   []
                 Ptyp_package "M" ([2,20]..[2,21])

This comment has been minimized.

Copy link
@trefis

trefis Jun 21, 2018

Author Contributor

I added a call to reloc_typ which fixed that issue.

This comment has been minimized.

Copy link
@let-def

let-def Jun 21, 2018

Contributor

I tend to agree, though it is not really uniform in the rest of the parser.
And the other rules in simple_core_type2 also call mktyp to make a type from a desc, so to me it looks like _desc might be the right construction.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@gasche: I believe I addressed all your comments, are you happy with the current state of the PR?

@gasche

gasche approved these changes Jun 21, 2018

Copy link
Member

left a comment

Good to go when the CI passes -- see a remaining minor comment.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

Also, I think that we should eventually create a Changes entry to keep track of the improvements to the yacc parser we are doing collectively (#1844, #1846 and this one #1850 for now). This is non-trivial work.

@trefis trefis force-pushed the trefis:better-locs branch from 1ca292e to 0d872d1 Jun 21, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

I rebased and squashed all the commits. I'll merge this now.

@trefis trefis merged commit 86ed685 into ocaml:trunk Jun 21, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:better-locs branch Jun 21, 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.