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

#8610: toplevel printing, consistent deduplicated name for types #8613

Merged
merged 6 commits into from Apr 15, 2019

Conversation

@Octachron
Copy link
Contributor

commented Apr 12, 2019

This PR fixes the final state of Printtyp.print_items. Without this fix, a final type declaration in a phrase is not registered in the environment before the next call to Printtyp.print_items.

This make the disambiguation of type name inconsistent between

type int
type u ;;
0;;

_ : int/2

and

type float;;
0.;;

_ : float

With this PR, the two examples are rendered in the same way

type float;;
0.;;

_ : float/2

Copy link
Member

left a comment

I must say that the treatment of printing state in this file is fairly horrifying -- it takes courage to think of a best solution for a fix in this context.

Your change in print_items does two new things at the end of a group: it cleans the protected names, and it resets the printing environment. I understand why it's the right conceptual choice (it mirrors the logic in still_in_type_group), but could you explain which of the changes are necessary to fix this bug? One, the other, or both?

Also, why is the other user of still_in_type_group, namely tree_of_signature_rec, not changed in the same way? It is only called from the outside under a wrap_env so in a sense resetting the env is already done, is that the reason? (I would prefer to have both users follow the same protocol.)

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I have updated tree_of_signature_rec to mirror print_items: the difference between the two was merely that the faulty state of tree_of_signature_rec is harder to observe.

Concerning the change, just clearing the protected names would fix this particular printing error, but keeping the environment in sync seemed clearer.

Another important point is that fixing this bug unearthed a bug with the printing of some erroneous private row type:

type 'a t = private [> 'x ] as 'a;;

Error: Type declarations do not match:
type 'a t = private [> `x ] constraint 'a = 'a t
is not included in
type 'a t
Their constraints differ.

The last commit fix this issue by being more cautious when printing isolated type declaration.

@trefis trefis dismissed their stale review Apr 12, 2019

diff changed

@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@Octachron could you add a test for type 'a t = private [> 'x ] as 'a;; before your changes, so we can see the difference?

@Octachron Octachron force-pushed the Octachron:fix_toplevel_name_disambiguation branch from a934378 to b37bf3e Apr 12, 2019
@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Sure, here is the test before this PR: 60c5462 .

@gasche
gasche approved these changes Apr 12, 2019
Copy link
Member

left a comment

Thanks! This looks good to me as well, but you should add a Changes entry.

When we have time, I hope that we will be able to revisit this file together to better encapsulate the statefulness, as @Octachron just did with with_hidden.

Changes Outdated Show resolved Hide resolved
@Octachron Octachron force-pushed the Octachron:fix_toplevel_name_disambiguation branch from 8060972 to e0d07a4 Apr 12, 2019
@Octachron Octachron merged commit b83d473 into ocaml:trunk Apr 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
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
Projects
None yet
3 participants
You can’t perform that action at this time.