-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[CSSolver/SILGen] Fix solver to support function conversion with collection subtyping #13138
Conversation
@DougGregor please take a look, I most likely did it wrong but at least it works now :) |
test/Constraints/rdar35702810.swift
Outdated
@@ -0,0 +1,21 @@ | |||
// RUN: %target-swift-frontend -emit-sil -verify %s | %FileCheck %s |
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.
Please put this test case in test/SILGen/function_conversions.swift together with the others, and add executable test to test/Interpreter/FunctionConversion.swift.
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.
Thanks! Will do!
lib/SILGen/SILGenPoly.cpp
Outdated
@@ -490,6 +490,27 @@ ManagedValue Transform::transform(ManagedValue v, | |||
} | |||
} | |||
|
|||
if (outputSubstType->getStructOrBoundGenericStruct() && |
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.
Add a comment here to match the other comments in this function
@@ -2090,8 +2090,7 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, | |||
} | |||
|
|||
// Special implicit nominal conversions. | |||
if (!type1->is<LValueType>() && |
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.
Making these Subtype rather than Conversion is likely to have other consequences
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've talked to @DougGregor offline about this, upcasts were intended for everything >= Subtype
which is one right before Conversion
:)
This is a language change, does it need an evolution proposal? |
@slavapestov it's a bug, upcasts should have been supported to constraints starting from |
@swift-ci please smoke test |
lib/SILGen/SILGenPoly.cpp
Outdated
@@ -490,6 +490,28 @@ ManagedValue Transform::transform(ManagedValue v, | |||
} | |||
} | |||
|
|||
// - upcasts for structs |
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.
"for collections" to be more precise
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.
Ah absolutely, sorry I was fixed on "generic struct" for some reason while writing that comment...
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci please test source compatibility |
@DougGregor @slavapestov are you ok if I merge it as is (i've fixed the comment)? |
@@ -490,6 +490,28 @@ ManagedValue Transform::transform(ManagedValue v, | |||
} | |||
} | |||
|
|||
// - upcasts for collections |
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.
It looks like you should check that the input and output struct decls are the same. Otherwise, [T]
to AnyHashable conversion will fail, since we check for that further down. Can you add a test?
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.
Sure, will do!
…ection subtyping Fix collection subtyping relation in function argument position by emiting special re-abstraction thunk with collection upcast. Resolves: rdar://problem/35702810
@slavapestov Does this look better? I've updated both Interpreter and SILGen tests. |
@swift-ci please test |
@swift-ci please test source compatibility |
Fix collection subtyping relation in function argument position
by emiting special re-abstraction thunk with collection upcast.
Resolves: rdar://problem/35702810