-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Sema: Check 'where' clause requirements on type witnesses #33493
Sema: Check 'where' clause requirements on type witnesses #33493
Conversation
|
@swift-ci Please smoke 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.
Thanks for hunting this down!
|
Is this related to #31259? |
013f1bb
to
1755c42
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test macOS |
|
@AnthonyLatsis yeah, and unfortunately I see the same problem with SwiftUI: |
1755c42
to
ad35000
Compare
|
@AnthonyLatsis I added a hack to preserve backward compatibility with the SwiftUI generated interface. We skip the 'where' clause check if the typealias's underlying type is Never. We could fix this with a new language mode one day, but we'd still have to typecheck and parse the old interface with -swift-version 5. |
|
Just so I understand – the reason we must resort to a hack is because SwiftUI ships with Apple OSes, and must therefore preserve binary compatibility? |
ad35000
to
467ef9f
Compare
| typealias A = Int | ||
| } | ||
|
|
||
| extension S3 : P where T == Int {} |
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.
Could you change one of the cases or add a new one for a suitable witness where two extensions are constrained differently? For example, T: Equatable (type witness context) and T == Int (conformance context).
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 tried that, but we diagnose the second typealias as a re-declaration even if the requirements in the where clause were mutually exclusive. We might not want to change this behavior.
I also tried putting the typealias in a constrained protocol extension, but that hits a circularity via the GSB. I'll investigate this issue later.
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.
Sorry for the confusion, what I meant is ~
protocol P { associatedtype A }
struct Foo<T> {}
extension Foo where T: Equatable {
typealias A = ...
}
extension Foo : P where T == Int {} // OKThere 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, thanks for the explanation. I’ll add this in a follow-up commit since I have a few more changes coming in this area.
|
@AnthonyLatsis Type aliases are not part of the ABI. The problem is with the SwiftUI.swiftinterface file that ships in the SDK in Xcode. We have to be able to type check all the declarations in this file with the new compiler, so we have to accept the conformance as if the |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
| @@ -1,4 +1,4 @@ | |||
| // RUN: %target-swift-frontend -emit-ir -o %t.ll %s | |||
| // RUN: not %target-swift-frontend -emit-ir %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.
FWIW This was actually an invalid test case that we used to accept (and presumably we used to crash in the even more distant past). Since we now correctly reject it, I changed the expectation here.
In the included test case, conformance checking of Wrapper : B would pick up typealias Foo as a witness for the associated type B.Foo. However, this typealias Foo is defined in a constrained extension where T : A, and the underlying type references the associated type A.Foo on T. The resulting substitution is invalid when the conformance Wrapper : B is used in a context where T does not conform to A. Instead, we should ignore this typealias entirely, since it appears in an unusable constrained extension. Fixes <rdar://problem/60219705>, <https://bugs.swift.org/browse/SR-12327>, <https://bugs.swift.org/browse/SR-12663>.
467ef9f
to
6d84c18
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
If we can move the type alias under correct constraints without breaking the ABI, and both language release versions and the SwiftUI.swiftinterface ship with Xcode, why would we have to keep the hack behind a language mode anyway if a coordinated fix is possible to organize eventually for a subsequent Xcode release? |
In the included test case, conformance checking of Wrapper : B would
pick up typealias Foo as a witness for the associated type B.Foo.
However, this typealias Foo is defined in a constrained extension where
T : A, and the underlying type references the associated type A.Foo
on T.
The resulting substitution is invalid when the conformance Wrapper : B
is used in a context where T does not conform to A.
Instead, we should ignore this typealias entirely, since it appears
in an unusable constrained extension.
Fixes rdar://problem/60219705, https://bugs.swift.org/browse/SR-12327,
https://bugs.swift.org/browse/SR-12663.