-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Express noncopyability through the suppression of Copyable
#64699
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
Conversation
acf7b56
to
8c258a6
Compare
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
718c2d7
to
fc3194e
Compare
@swift-ci smoke test |
@swift-ci smoke test macOS |
@swift-ci please clean smoke test |
0a57160
to
2a63c25
Compare
@swift-ci please test |
5a28271
to
00bfe00
Compare
e0dbd4c
to
c006a38
Compare
swiftlang/swift-syntax#1588 |
1 similar comment
swiftlang/swift-syntax#1588 |
Just some minor missed test updates. I think this is ready for review. |
47162b3
to
4dcd8ca
Compare
swiftlang/swift-syntax#1588 |
Also has been referred to as "without Copyable" See SE-390 for details, or read the discussion here: https://forums.swift.org/t/second-review-se-0390-noncopyable-structs-and-enums/63866
many of these are mechanical changes
…ly types work with library evolution
Rather than try to omit explicit uses of `Copyable` in the output module for backwards compatability, we instead make it an error to use `Copyable` as a type when not suppressed. While it's straightforward to omit it for generic parameters, for uses of `Copyable` as an existential, we'd need to emit `Any` in its place. We'd also need to find and omit or modify explicit type constraints or same-type requirements involving Copyable. All of that sounded rather gross, and not really needed right now since any uses of `Copyable` explicitly is just not needed. update test with Copyable being banned generally
This will help prevent condfails when older compilers try to typecheck interfaces with `~Swift.Copyable` in them.
4dcd8ca
to
3e735ab
Compare
@swift-ci please test |
swiftlang/swift-syntax#1588 |
@swift-ci please smoke test |
3f2f702
to
58f1170
Compare
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.
mostly NFC feedback but also noticed some issues.
auto unprintable = [&options](Type subTy) { | ||
if (auto aliasTy = dyn_cast<TypeAliasType>(subTy.getPointer())) | ||
return !options.shouldPrint(aliasTy->getDecl()); | ||
if (auto NTD = subTy->getAnyNominal()) { | ||
if (!options.shouldPrint(NTD)) | ||
return true; | ||
} | ||
return false; | ||
}; | ||
|
||
// Collect types / suppressed entries written in the inheritance clause. | ||
for (auto entry: inherited) { | ||
if (auto ty = entry.getType()) { | ||
bool foundUnprintable = ty.findIf([&](Type subTy) { | ||
if (auto aliasTy = dyn_cast<TypeAliasType>(subTy.getPointer())) | ||
return !options.shouldPrint(aliasTy->getDecl()); | ||
if (auto NTD = subTy->getAnyNominal()) { | ||
if (!options.shouldPrint(NTD)) | ||
return true; | ||
} | ||
return false; | ||
}); | ||
if (foundUnprintable) | ||
if (ty.findIf(unprintable)) |
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.
reverting this NFC hunk would be nice
if (suppressedParam) | ||
return true; | ||
|
||
// TODO: eventually once where clauses support suppression, this ought to |
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.
"where"
Type inheritedType | ||
= evaluateOrDefault(ctx.evaluator, | ||
InheritedTypeRequest{decl, index, | ||
TypeResolutionStage::Structural}, | ||
Type()); | ||
if (!inheritedType) continue; | ||
|
||
// handle suppressed requirements | ||
if (inheritedEntries[index].isSuppressed) { | ||
// TODO: we should be adding a requirement for Copyable if not suppressed. |
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.
rephrase this as a future direction
Evaluator &evaluator, | ||
TypeOrExtensionDeclPointer decl, | ||
unsigned index, TypeResolutionStage stage) const { | ||
return resolveTypeLocWithin(decl, index, stage, getInheritedEntryAtIndex); |
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 revert this refactoring as it's not required anymore.
case TypeReprKind::Suppressed: | ||
// Can't have a suppressed type as a function param. | ||
diagnose(specifierRepr->getLoc(), diag::suppression_illegal_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.
ensure test coverage of this; forget if i added it
@@ -4314,6 +4345,39 @@ TypeResolver::resolveCompileTimeConstTypeRepr(CompileTimeConstTypeRepr *repr, | |||
return resolveType(repr->getBase(), options); | |||
} | |||
|
|||
NeverNullType | |||
TypeResolver::resolveSuppressedTypeRepr(SuppressedTypeRepr *repr, |
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.
what happens if you tried
struct S: ~U {}
struct U: ~S {}
// The first bit indicates "@unchecked", second bit indicates suppression. | ||
bool isUnchecked = rawID & 0b01; | ||
bool isSuppressed = rawID & 0b10; | ||
TypeID typeID = rawID >> 2; |
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.
NOTE: this is why reading an old module would be impossible.
/// For older compilers interacting with newer stdlibs, define the unavailable | ||
/// `_Copyable` marker protocol that they expect to find. | ||
@available(*, unavailable) | ||
typealias _Copyable = Copyable |
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.
must be a public typealias! I also wonder if it being a typealias even works. to play it safe maybe just duplicate the old one right here:
/// This marker protocol represents types that support copying.
/// This type is not yet available for use to express explicit
/// constraints on generics in your programs. It is currently
/// only used internally by the compiler.
@available(*, unavailable)
@_marker public protocol _Copyable {}
@swift-ci please test |
See https://forums.swift.org/t/second-review-se-0390-noncopyable-structs-and-enums/63866
This implements
~Copyable
constraints, which means "suppress the implicit conformance toCopyable
for this type"That's not the final accepted syntax; writing
without Copyable
appears to have the most support on the forums. A follow-up PR will change this syntax once the dust has settled from the review.companion PR: swiftlang/swift-syntax#1588
resolves rdar://106775103