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 PR8701 by expanding types in lower_contravariant #8725

Merged
merged 3 commits into from Jun 13, 2019

Conversation

@garrigue
Copy link
Contributor

commented Jun 9, 2019

As noted in #8701, Ctype.lower_contravariant ended up lowering the levels of type variables inside constrained parameters, where it should have first tried to expand the type.
This resulted in principality issues.

This PR fixes that by expanding when needed.
This could be more optimized, but the code intends to be simple.

This fix should also go in 4.09.

@garrigue garrigue requested a review from lpw25 Jun 9, 2019
begin try
let ty = !forward_try_expand_once env ty in
lower_rec contra ty
with Cannot_expand ->

This comment has been minimized.

Copy link
@gasche

gasche Jun 9, 2019

Member

This would be more readable with

match !forward_try_expand_once ty with
| ty -> lower_rec contra ty
| exception Cannot_expand ->
  ...

which has more localized exception handling (and a tail-call, which is nice).

This comment has been minimized.

Copy link
@garrigue

garrigue Jun 10, 2019

Author Contributor

Indeed, I even remember noting to myself that this try-with was not tail-recursive.
Funny how long it takes for new language features to become automatic.

begin match !forward_try_expand_once env ty with
| ty -> lower_rec contra ty
| exception Cannot_expand ->
(* abbrev := Mnil; Should not be needed anymore *)

This comment has been minimized.

Copy link
@gasche

gasche Jun 10, 2019

Member

I would like to understand this line, and also I think it could be better to not leave this kind of code comments in the source (I feel that nobody except maybe you quite knows why there are here, we are afraid of removing them but they don't make reading the code any easier.)

I looked for abbrev usage in the codebase. They are populated by Btype.memorize_abbrev, which is itself used by Btype.subst only -- so only subst creates non-Mnil abbrev caches. fordward_expand_once calls subst, so it populates the abbrev field. The only places which clear those caches are generalize_structure, lower_contraviant (before this patch) and the global-cleanup functions calling cleanup_abbrev (for example check_abbrev_env called above).

I have two questions:

  1. Why was abbrev := Mnil needed before?
  2. Why is it "not needed anymore"?

Here is my best shot at answering them:

  1. You need to clear the Tconstr abbreviation if you want to change the level (compared to the moment where the expansion was memoized), so structural generalization/lowering functions must clear the abbrev.
  2. If you do use forward_try_expand_once (as you do in this patch), you want to use the caching provided by the abbrev field when you encounter the constructor again, so you do not want to clear abbreviations. Besides, expand_abbrev_gen is careful to update the levels (I think?), so clearing abbrev is unnecessary and also dangerous performance-wise, as it defeats the purpose of expansion caching.

Is that reasoning correct? (Can we remove the commented-out code?)

This comment has been minimized.

Copy link
@garrigue

garrigue Jun 11, 2019

Author Contributor

Before a previous fix, lower_contravariant was called generalize_contravariant, and it was generalizing on its way. As a result this assignment was required. I forgot to remove it when doing that conversion, but it is safe to remove it, and as you remark maybe even required. I tend to be conservative when removing code, but the code is not needed and the comment is correct (this function now only changes the levels of type variables, not of constructor nodes), so it would be possible to remove both.

This comment has been minimized.

Copy link
@garrigue

garrigue Jun 11, 2019

Author Contributor

I understand why you don't like this kind of cryptic comments, but they are sometimes helpful when chasing bugs, as they give you some insight of the history of the code. Lots of bugs come from wrong changes.

This comment has been minimized.

Copy link
@gasche

gasche Jun 11, 2019

Member

If you want to keep a comment, maybe it could sound a bit more confident. For example:

(* Note: we are preserving the `abbrev` cache here to speedup future expansions.
   This is correct as only the level of type variables is touched in this function,
   not of type constructors. *)
@gasche
gasche approved these changes Jun 12, 2019
Copy link
Member

left a comment

I believe the PR is correct so here is an approval. I still believe that the abbrev := Mnil comment as stated is needlessly confusing, so I won't merge right away -- but someone else can do it if they feel like it.

@garrigue garrigue merged commit 6e1b7f4 into ocaml:trunk Jun 13, 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
@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Thank you for the approval.
I removed the comment, since it had achieved it goal: letting you properly review the removal of the assignment.
Maybe it could have been done by a comment inside github, but I'm not a tool expert...

garrigue added a commit that referenced this pull request Jun 13, 2019
@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Cherry-picked to 4.09.

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