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

make short-paths work on more polymorphic variants #881

Merged
merged 2 commits into from Jan 2, 2017

Conversation

Projects
None yet
5 participants
@sliquister
Contributor

sliquister commented Oct 28, 2016

Compare without and with this change:

 $ ocaml -short-paths
        OCaml version 4.03.0+dev11-2015-10-19

 # module B = struct type perms = [ `A | `B ] end;;
 module B : sig type perms = [ `A | `B ] end
 # open B;;
 # let x = lazy (assert false : [< B.perms ]);;
-val x : [< B.perms ] lazy_t = <lazy>
+val x : [< perms ] lazy_t = <lazy>
 # 
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 12, 2016

Member

The proposed change seems correct to me (using args here corresponds to replacing tree_of_typlist sch tyl by tree_of_typlist sch (apply_subst s tyl)), but I don't understand the code very well. @garrigue, this is code you added in 00e7279 , do you agree that the substitution should happen here as well?

Member

gasche commented Nov 12, 2016

The proposed change seems correct to me (using args here corresponds to replacing tree_of_typlist sch tyl by tree_of_typlist sch (apply_subst s tyl)), but I don't understand the code very well. @garrigue, this is code you added in 00e7279 , do you agree that the substitution should happen here as well?

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Nov 12, 2016

Contributor

I think the change needs to be guarded by an is_nth check.

Contributor

lpw25 commented Nov 12, 2016

I think the change needs to be guarded by an is_nth check.

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Nov 15, 2016

Contributor

Ah, it looks like the path returned by best_type_path is garbage when the param_subst is Nth. I undid the change when that happens. Ideally I would put List.hd args into the AST instead, but I don't know how to do that, and it seems very unlikely to show up in practice.

Contributor

sliquister commented Nov 15, 2016

Ah, it looks like the path returned by best_type_path is garbage when the param_subst is Nth. I undid the change when that happens. Ideally I would put List.hd args into the AST instead, but I don't know how to do that, and it seems very unlikely to show up in practice.

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 5, 2016

Contributor

After looking at PR#6836, I changed the outcome tree as Garrigue described.

It's now pretty clear this change is correct: this branch is only about choosing to omit the brackets (ie simplifying [ | M.t ] into M.t), and the then case was already using the short-path'ed type contructor, so the else branch should too.

Contributor

sliquister commented Dec 5, 2016

After looking at PR#6836, I changed the outcome tree as Garrigue described.

It's now pretty clear this change is correct: this branch is only about choosing to omit the brackets (ie simplifying [ | M.t ] into M.t), and the then case was already using the short-path'ed type contructor, so the else branch should too.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 28, 2016

Contributor

@lpw25 Can you confirm this is correct now?

Contributor

mshinwell commented Dec 28, 2016

@lpw25 Can you confirm this is correct now?

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Jan 2, 2017

Contributor

It now looks correct to me.

Contributor

lpw25 commented Jan 2, 2017

It now looks correct to me.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 2, 2017

Member

According to Travis, this patchset (1) lacks a Changes entry and (2) breaks the Camlp4 build because, if I understand correctly, of the change to the outcometree type:

File "camlp4/Camlp4Top/Rprint.ml", line 176, characters 10-19:
Error: This variant pattern is expected to have type Outcometree.out_variant
       The constructor Ovar_name does not belong to type Outcometree.out_variant

It would be nice to patch camlp4's master branch to fix this, so that we can merge both at once and not break the Travis CI for everyone -- while maintaining that camlp4 checking that I think is useful.

Member

gasche commented Jan 2, 2017

According to Travis, this patchset (1) lacks a Changes entry and (2) breaks the Camlp4 build because, if I understand correctly, of the change to the outcometree type:

File "camlp4/Camlp4Top/Rprint.ml", line 176, characters 10-19:
Error: This variant pattern is expected to have type Outcometree.out_variant
       The constructor Ovar_name does not belong to type Outcometree.out_variant

It would be nice to patch camlp4's master branch to fix this, so that we can merge both at once and not break the Travis CI for everyone -- while maintaining that camlp4 checking that I think is useful.

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Jan 2, 2017

Contributor

I added a Changes entry and fixed the camlp4 build in ocaml/camlp4#118.

Contributor

sliquister commented Jan 2, 2017

I added a Changes entry and fixed the camlp4 build in ocaml/camlp4#118.

@gasche gasche merged commit fae3ed0 into ocaml:trunk Jan 2, 2017

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
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 2, 2017

Member

Thanks! (I don't really deserve a "review" mention on this one that I was largely clueless about.)

Member

gasche commented Jan 2, 2017

Thanks! (I don't really deserve a "review" mention on this one that I was largely clueless about.)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 2, 2017

Member

Thinking more about this, I think we should mention that this changes the Outcometree definition, in case other consumers (than Camlp4) are relying on it. (Thinking more about this, I'm starting to get mildly worried that this may affect too many people; I would consider reverting if it turned out to be the case, so please complain here if it breaks your code.)

P.S.: I'll write to @roglo that Camlp5 should probably be updated.

Member

gasche commented Jan 2, 2017

Thinking more about this, I think we should mention that this changes the Outcometree definition, in case other consumers (than Camlp4) are relying on it. (Thinking more about this, I'm starting to get mildly worried that this may affect too many people; I would consider reverting if it turned out to be the case, so please complain here if it breaks your code.)

P.S.: I'll write to @roglo that Camlp5 should probably be updated.

gasche added a commit that referenced this pull request Jan 2, 2017

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 2, 2017

Member

I made the Outcometree change more explicit in the Changelog: 239c054

Member

gasche commented Jan 2, 2017

I made the Outcometree change more explicit in the Changelog: 239c054

@roglo

This comment has been minimized.

Show comment
Hide comment
@roglo

roglo Jan 4, 2017

Contributor

Current git of Camlp5 updated for current trunk.

Contributor

roglo commented Jan 4, 2017

Current git of Camlp5 updated for current trunk.

@sliquister sliquister deleted the sliquister:short-path-variants branch Sep 17, 2017

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #881 from sliquister/short-path-variants
make short-paths work on more polymorphic variants

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment