Implementation for /most/ of SE-0192 (frozen and non-frozen enums) #14945
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
|
Build failed |
Build comment file:Compilation-performance test failed |
jrose-apple
force-pushed
the
frozen-enums
branch
from
323abe7 to
094ea9c
Compare
|
@swift-ci Please test |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
Build comment file:Compilation-performance test failed |
jrose-apple
force-pushed
the
frozen-enums
branch
from
094ea9c to
a98032b
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
jrose-apple
force-pushed
the
frozen-enums
branch
from
a98032b to
906af8e
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please clean test macOS |
|
Build failed |
|
@swift-ci Please test macOS |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
|
|
@swift-ci Please test source compatibility |
jrose-apple
changed the title
stdlib/public/core/Codable.swift.gyb
Outdated
| @@ -1101,7 +1101,7 @@ public struct CodingUserInfoKey : RawRepresentable, Equatable, Hashable { | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| /// An error that occurs during the encoding of a value. | |||
| @_fixed_layout // FIXME(sil-serialize-all) | |||
| @_frozen // FIXME(sil-serialize-all) | |||
There was a problem hiding this comment.
This is a surprise to me; why should encoding and decoding errors be frozen? I'd expect this to grow over time, like most error types.
There was a problem hiding this comment.
They'll definitely get un-frozen. This commit did not remove any frozens that were currently there.
There was a problem hiding this comment.
... it did in stdlib/public/Platform/POSIXError.swift
There was a problem hiding this comment.
…oops. Okay, I'll be consistent.
| public enum _DebuggerSupport { | ||
| @_fixed_layout // FIXME(sil-serialize-all) | ||
| @_frozen // FIXME(sil-serialize-all) | ||
| @_versioned // FIXME(sil-serialize-all) | ||
| internal enum CollectionStatus { |
There was a problem hiding this comment.
Is there any point to a frozen internal enum?
There was a problem hiding this comment.
It's versioned, so yes.
| Name: EnumExhaustivity | ||
| Tags: | ||
| - Name: RegularEnumTurnedExhaustiveThenBackViaAPINotes | ||
| EnumExtensibility: open |
There was a problem hiding this comment.
I'd never seen this cute "no newline at end of file" GitHub graphic before
| @@ -239,6 +242,10 @@ namespace swift { | |||
| /// Diagnose uses of NSCoding with classes that have unstable mangled names. | |||
| bool EnableNSKeyedArchiverDiagnostics = true; | |||
|
|
|||
| /// Diagnose switches over non-frozen enums that do not have catch-all | |||
| /// cases. | |||
| bool EnableNonFrozenEnumExhaustivityDiagnostics = false; | |||
There was a problem hiding this comment.
This is for staging while we settle on the "unknown case" spelling, right? It'll go away then?
There was a problem hiding this comment.
It's mainly so I can land this part before SE-0192's review period ends, but yes.
There was a problem hiding this comment.
Please add some code generation (SILGen) tests for -enable-testing. I imagine we'll want to trap if the original library evolves resiliently, right?
Also, library evolution tests for non-frozen @objc enums to ensure the unknown case is handled properly.
|
|
||
| // All other checks are use-site specific; with no further information, the | ||
| // enum must be treated non-exhaustively. | ||
| if (!useDC) |
There was a problem hiding this comment.
Do we ever need to check exhaustiveness in SILGen or anything else that runs after Sema? Because we won't have a useDC anymore
There was a problem hiding this comment.
I think we'll have a DC up through SILGen, but you do have me worried about optimizations. Unfortunately we don't actually the right API for that ("AvailabilityContext"); fortunately, it'll err on the side of "non-exhaustive". (That is, passing nullptr or the containing module or the containing source file might answer "non-exhaustive" when it could be "exhaustive", but never the other way around.)
lib/Sema/TypeCheckAttr.cpp
Outdated
| void AttributeChecker::visitFrozenAttr(FrozenAttr *attr) { | ||
| switch (D->getModuleContext()->getResilienceStrategy()) { | ||
| case ResilienceStrategy::Default: | ||
| // FIXME: Don't warn about the use of this attribute in the standard |
There was a problem hiding this comment.
You might be able to get away with removing this now that resilience is on
There was a problem hiding this comment.
Ah, yes, I forgot about this!
| @@ -121,25 +127,25 @@ namespace { | |||
| // potentially push an enormous fixit on the user. | |||
| static const size_t MAX_SPACE_SIZE = 128; | |||
|
|
|||
| size_t computeSize(TypeChecker &TC, | |||
| size_t computeSize(TypeChecker &TC, const DeclContext *DC, | |||
There was a problem hiding this comment.
It's awkward to plumb the DC (and TC) through everything here -- can't we make them instance variables?
There was a problem hiding this comment.
Sure, that makes sense. I'll save it for the next half, though, where I implement unknown:.
stdlib/public/core/Codable.swift.gyb
Outdated
| @@ -1101,7 +1101,7 @@ public struct CodingUserInfoKey : RawRepresentable, Equatable, Hashable { | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| /// An error that occurs during the encoding of a value. | |||
| @_fixed_layout // FIXME(sil-serialize-all) | |||
| @_frozen // FIXME(sil-serialize-all) | |||
There was a problem hiding this comment.
... it did in stdlib/public/Platform/POSIXError.swift
In theory there could be a "fixed-layout" enum that's not exhaustive but promises not to add any more cases with payloads, but we don't need that distinction today. (Note that @objc enums are still "fixed-layout" in the actual sense of "having a compile-time known layout". There's just no special way to spell that.)
Still to do: test and fix up the use of multiple enum_extensibility annotations, possibly with API notes. This is important because the definition of NS/CF_ENUM /includes/ enum_extensibility(open) as of Xcode 9.0; there should be a convenient out people can use to declare exhaustive enums in C that's backwards-compatible.
Since NS_ENUM has an enum_extensibility(open) in it already, we want to allow people to undo that by sticking enum_extensibility(closed) on the end of their enum.
Previously (a03c40c) we assumed all Swift enums were non-frozen in ObjC, a weird choice in retrospect. Now that we actually distinguish frozen and non-frozen enums in Swift, we can use the 'enum_extensibility' attribute to mark them as open or closed in ObjC. Note that this only matters for Swift libraries compiled with -enable-resilience, i.e. those that might get a new implementation at runtime. Everyone else is now declaring a "closed" enum, matching the behavior in Swift.
Error codes, FloatingPointRoundingRule, Mirror.AncestorRepresentation, Mirror.DisplayStyle, and PlaygroundQuickLook.
That's just ARCamera.TrackingState and DispatchTimeoutResult. Verified with the framework owners at Apple.
|
We do have the "evolution" tests for non-frozen enums by testing private cases, but I'll add the ones for |
|
@jrose-apple The case I'm thinking of is when you switch over an @objc non-frozen enum, we want to make sure we don't hit UB if you pass in an unknown case |
|
Sorry, I forgot that one already landed. :-) See #14724. |
jrose-apple
force-pushed
the
frozen-enums
branch
from
1ace1fa to
348f966
Compare
|
Addressed feedback. @swift-ci Please smoke test |
|
Merging even though the proposal is still in review in order to get some living-on (and so that I don't have to keep rebasing it). Note that the diagnostics are still off and |
|
Is there a way to suppress the warning this produces if you don't have a |
|
I'm not working on Swift anymore, but people have talked about this for "I know this library is built with library evolution turned on, but I'm embedding it in my app, so it can't possibly change out from under me". It's not clear that such an annotation would really be any better than just writing the |
|
I do suggest taking this discussion to the forums rather than commenting on a two-year-old PR, if you haven't already. (As you noticed, it took a month for me to even see it!) |
AnthonyLatsis
added
compiler
An alternative to #11961 that only makes a frozen / non-frozen distinction for imported C enums and for Swift enums in the stdlib/overlays. (Really, it's "libraries compiled with
-enable-resilience", and exactly what that means has not been discussed publicly and is not part of SE-0192.)This does not include the implementation of
unknown case, or whatever we end up naming it. That's a big enough change on its own that it deserves its own PR, and this part can land first.The diagnostics part of this is behind a frontend flag,
-enable-nonfrozen-enum-exhaustivity-diagnostics.