-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support clever expansion of gadt equations in presence of local module #10348
Conversation
A further relaxation would be to allow expanding parametric types. But this can be tricky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is definitely an improvement. I have a couple of suggestions/questions, once they are addressed I think this is good to merge.
typing/ctype.ml
Outdated
if unify_eq t1 t2 then () else | ||
match (t1.desc, t2.desc) with | ||
| (Tconstr (p1, tl1, _), Tconstr (p2, tl2, _)) | ||
when is_public_type env p1 && is_public_type env p2 -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this when
clause? It's not clear to me why the type being private or being a variant/record would make the actions in this clause unsound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall probably write a more explicit comment.
Contrary to unify2
, this code forgets the abbreviations it is expanding.
In particular this means that since unify2
should be called on the non-expanded types, we cannot call it directly here, hence the exception Cannot_expand
when reverting to unify2
.
Since we do not want to do useless work, and the case which interests us is when there is a risk of superfluously expanding a GADT equation, we stop as soon as there is no chance to reach one.
Since GADT equations are only defined for abstract types, and since an abstract type can only be reached by expanding a public type, it is sufficient to consider public types.
typing/ctype.ml
Outdated
unify2 env t1 t2 | ||
end | ||
| (Tconstr _, Tconstr _) when Env.has_local_constraints !env -> | ||
(try unify_public !env t1 t2 with Cannot_expand -> unify2 env t1 t2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than raising Cannot_expand
, would it be better to just call straight into unify2
from unify_public
? It seems like that might save us from redoing some expansion work at the start of unify2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As indicated above, unify2
should be called with the non-expanded version of the type.
Note that thanks to memoization, redoing expansions just amounts to looking them up , without any allocation.
This small cost could be avoided by having unify2
receive both the original and partly expanded forms, like unify3
. Since we only do extra work when expanding non-parametric type abbreviations, I'm not sure it is worth adding extra complexity to the code to optimize that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having unify2 receive both the original and partly expanded forms
Yes, that is what I had in mind. I think it is a tiny change and if it would allow us to remove the is_public_type
conditions then I think the code would overall be simpler.
Since we only do extra work when expanding non-parametric type abbreviations
I don't see why the work is only for non-parametric type abbreviations. It seems to be for all type abbreviations. I think that is fine, especially if we reuse the work by adding the extra parameters to unify2
.
Thanks for your comments. I will at least add explanations to the code, as it may be a bit terse currently. |
Ok, I did as suggested. I'm still a bit concerned about the interaction between scope and expansion ( Here is an example that should not be typable, but is allowed in both 4.12 and trunk. Interestingly, it is rejected with this PR, but I'm not sure this means that this PR is safer. type (_,_) eq = Refl : ('a,'a) eq;;
let f (type a b c) (w1 : (a,b) eq) (w2 : (b,c) eq) (a : a) (b : b) (c : c) =
let Refl = w1 in ignore (let Refl = w2 in let x = if true then a else if true then b else c in x);; Namely, the result of the if expression uses both equations, so it should not be allowed to escape from the matching of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I still have a couple of questions.
typing/ctype.ml
Outdated
if unify_eq t1 t2 then () else | ||
try match (t1.desc, t2.desc) with | ||
| (Tconstr (p1, tl1, a1), Tconstr (p2, tl2, a2)) -> | ||
if Path.same p1 p2 && tl1 = [] && tl2 = [] && is_public_type !env p1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we still need the is_public_type
test here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is mostly conservatism.
is_public_type
only checks type_kind
and type_private
, and if it fails for either reason the type cannot be expanded anyway, so we will just end up doing the same unification in unify3
.
Maybe it is ok to just remove it. It could change error messages though (see below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best to just remove it. It makes the code harder to think about and doesn't really add anything.
I have rebased. |
@lpw25 rebased again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to merge, although I would prefer it if is_public_type
were removed.
typing/ctype.ml
Outdated
if unify_eq t1 t2 then () else | ||
try match (t1.desc, t2.desc) with | ||
| (Tconstr (p1, tl1, a1), Tconstr (p2, tl2, a2)) -> | ||
if Path.same p1 p2 && tl1 = [] && tl2 = [] && is_public_type !env p1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best to just remove it. It makes the code harder to think about and doesn't really add anything.
Removing |
@lpw25 remarked in #10277 that when combining local modules with GADTs, one could end up expanding a local equation during unification, leading to a spurious error.
This fixes that by allowing the special case for unification of two abstract local types to be used for other types too.