-
Notifications
You must be signed in to change notification settings - Fork 306
Fix type unification for type parameters #1479
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,12 +90,35 @@ func isEqualOrLessSpecific(t1, t2 *types.Type) bool { | |
| return isEqualOrLessSpecific(t1.Parameters()[0], t2.Parameters()[0]) && | ||
| isEqualOrLessSpecific(t1.Parameters()[1], t2.Parameters()[1]) | ||
| case types.TypeKind: | ||
| return true | ||
| p1Len, p2Len := len(t1.Parameters()), len(t2.Parameters()) | ||
| if p1Len > 0 && p2Len > 0 { | ||
| if p1Len != p2Len { | ||
| return false | ||
| } | ||
| return isEqualOrLessSpecific(t1.Parameters()[0], t2.Parameters()[0]) | ||
| } | ||
| return p1Len == 0 | ||
| default: | ||
| return t1.IsExactType(t2) | ||
| } | ||
| } | ||
|
|
||
| // hasTypeParam returns true if the type is a type parameter or contains any type parameters directly or transitively. | ||
| func hasTypeParam(t *types.Type) bool { | ||
| if t == nil { | ||
| return false | ||
| } | ||
| if t.Kind() == types.TypeParamKind { | ||
| return true | ||
| } | ||
| for _, param := range t.Parameters() { | ||
| if hasTypeParam(param) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // / internalIsAssignable returns true if t1 is assignable to t2. | ||
| func internalIsAssignable(m *mapping, t1, t2 *types.Type) bool { | ||
| // Process type parameters. | ||
|
|
@@ -143,7 +166,28 @@ func internalIsAssignable(m *mapping, t1, t2 *types.Type) bool { | |
| // Struct types. | ||
| return t2.IsAssignableType(t1) | ||
| case types.TypeKind: | ||
| return kind2 == types.TypeKind | ||
| if kind2 != types.TypeKind { | ||
| return false | ||
| } | ||
| p1Len, p2Len := len(t1.Parameters()), len(t2.Parameters()) | ||
| if p1Len == 0 || p2Len == 0 { | ||
| return p2Len == 0 | ||
| } | ||
| if p1Len != p2Len { | ||
| return false | ||
| } | ||
| fromType := t1.Parameters()[0] | ||
| toType := t2.Parameters()[0] | ||
| // If either type contains a type parameter (e.g., type(T) in foo(data, type(T)) -> T), | ||
| // delegate to inner type unification to bind or validate type parameter substitutions. | ||
| // Returns true if the inner types structurally match, unify with an unbound type param, | ||
| // or conform to an existing binding in 'm'. Returns false on structural/kind mismatches | ||
| // (e.g., int vs list(T)), occurs-check cycles, or conflicting type param bindings. | ||
| if hasTypeParam(fromType) || hasTypeParam(toType) { | ||
| return internalIsAssignable(m, fromType, toType) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there might be some issue with the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be ok, the type substitution check propagates for map parameters. I've added some tests around this just to be sure. |
||
| } | ||
| // Concrete types are coassignable in CEL (e.g., type(1) == type("a"), type([1]) == list). | ||
| return true | ||
| case types.OpaqueKind, types.ListKind, types.MapKind: | ||
| return t1.Kind() == t2.Kind() && t1.TypeName() == t2.TypeName() && | ||
| internalIsAssignableList(m, t1.Parameters(), t2.Parameters()) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.