[abstracts] fix unify_with_variance for abstracts #9723
Conversation
|
|
||
| and unify_type_params uctx a b tl1 tl2 = | ||
| let uctx = get_nested_context uctx in |
There was a problem hiding this comment.
Technically, this uctx should not go into type_eq, but I don't think it makes a difference in the current implementation.
There was a problem hiding this comment.
Question is why type_eq needs a unification context? I mean there is no need for it right now, but is there some idea for future property in the unification context that will be used both by unify and type_eq?
There was a problem hiding this comment.
I was contemplating merging type_eq and unify, but I've moved away from that idea. It's hard to tell if a unification context could be useful for it, so I wouldn't mind removing it again for now.
| List.iter2 unify_param tl1 tl2 | ||
| | TAbstract(a1,tl1),TAbstract(a2,tl2) -> | ||
| let uctx = get_after_abstract_context() in | ||
| compare_underlying EqStrict (get_underlying_type t1) (get_underlying_type t2); |
There was a problem hiding this comment.
Looking at this I'm wondering why we do something with the underlying type here. I realize that the previous implementation already does this as well, but I can't immediately tell why.
There was a problem hiding this comment.
I guess there might be two reasons:
-
For static targets physical difference matters and so if you have
Array<Int>you can't simply cast it toArray<Float>, even thoughIntdefinesto Float. Though allowed cast from/to dynamic in abstract-t/t-abstract contradicts this (if i understand it right). -
If you allow an abstract over a child with defined cast to a parent to unify with a parent in a type param, this will permit unchecked covariance.
There was a problem hiding this comment.
But Array<Int> to Array<Float> has nothing to do with underlying types. Array is a normal class.
Shouldn't all this be covered by the to and from declarations? I don't see why the relationship of underlying types is significant here.
There was a problem hiding this comment.
Added commit with removal of underlying checks for abstract/non-abstract.
There was a problem hiding this comment.
Removed that commit. Instead added assertions showcasing second reason (so first reason covers abstract-abstract, second one - abstract-t). Example here:
class Parent {}
class Child extends Parent {}
abstract MyChild(Child) to Parent {}
class Main {
public static function main() {
((null: Array<Child>): Array<Parent>); // fails as expected
((null: Array<MyChild>): Array<Parent>); // works if no underlying type checks are made
}
}There was a problem hiding this comment.
I think the correct behavior here is to require bidirectional unification for type-parameters. This will then work when the abstract has to Parent from Parent, in which case there's no variance issue.
And then if we ever allow variance annotations on type parameters, we would only have to check one of the directions instead of both.
There was a problem hiding this comment.
What about <Array> to <ReadOnlyArray>?
There was a problem hiding this comment.
I'm not sure, that looks like it shouldn't work without to Array<T>, but at the same time I don't see what harm it could do.
There was a problem hiding this comment.
I think those breaking changes will require more discussion and input...
In the meantime, i would prefer to fix bugs in current behaviour (and express it in tests).
Are there some considerations that prevent this from being merged?
Performance or complexity of code?
vonagam
force-pushed
the
fix-with-variance-for-abstracts
branch
from
aa90a10
to
69ac183
Compare
Closes #9721.