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

Fix GPR#1204 #1329

Merged
merged 5 commits into from Sep 16, 2017

Conversation

Projects
None yet
2 participants
@garrigue
Copy link
Contributor

commented Sep 13, 2017

Another way to fix #1204, by setting row_name in Typedecl, as suggested by @lpw25.

@lpw25

lpw25 approved these changes Sep 13, 2017

Copy link
Contributor

left a comment

Looks good to me

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

As a side-note, with this patch the type definition

type 'a t = [`A of 'a] as 'a;;

gets printed by the top-level printer in a way that is not accepted by the type-checker:

# type 'a t = [`A of 'a] as 'a;;
type 'a t = [ `A of 'a ] constraint 'a = 'a t

# type 'a t = [ `A of 'a ] constraint 'a = 'a t;;
Characters 0-45:
  type 'a t = [ `A of 'a ] constraint 'a = 'a t;;
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: The type abbreviation t is cyclic

However, this isn't a new bug from this code, as you could already get the same thing with a slightly different type in 4.04.1:

# type 'a t = [ `A of 'a t ] as 'a;;
type 'a t = [ `A of 'a t ] constraint 'a = 'a t

# type 'a t = [ `A of 'a t ] constraint 'a = 'a t;;
Characters 0-47:
  type 'a t = [ `A of 'a t ] constraint 'a = 'a t;;
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: The type abbreviation t is cyclic
@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

Indeed, the problem does not seem directly related to this fix.

However, this fix may have some impact on tools using the internal type representation for printing, in particular if they do not call the proper functions from Printtyp.
tests/tool-ocamldoc/t01.ml failed printing the type name as right hand of the equation.
I fixed this by pasting the same code as in Printtyp.tree_of_type_declaration , but I wonder how odoc does for instance.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

odoc carefully copies the behaviour of the functions in Printtyp so it should be fine.

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

Attempted a slightly different approach, adding the row name in Env just where the descriptions for record fields and variant constructors are generated. This avoids having to modify ocamldoc (and possibly other tools too).

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

It's much less clear that this new version is correct. It looks like it will only add row names to types coming from other modules. I think it would be better to use the version you had before.

@garrigue garrigue force-pushed the garrigue:fix_gpr1204 branch from 359e55d to b47d056 Sep 13, 2017

garrigue added some commits Sep 16, 2017

@garrigue garrigue merged commit 5a04984 into ocaml:trunk Sep 16, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.