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

Implementation for /most/ of SE-0192 (frozen and non-frozen enums) #14945

Merged
merged 12 commits into from Mar 21, 2018

Conversation

Projects
None yet
4 participants
@jrose-apple
Copy link
Member

jrose-apple commented Mar 3, 2018

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.

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test source compatibility

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test compiler performance

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build failed
Swift Test Linux Platform
Git Sha - 323abe749e92c655446f025896a21cfc590bbf6c

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build comment file:

Compilation-performance test failed

@jrose-apple jrose-apple force-pushed the jrose-apple:frozen-enums branch Mar 3, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test compiler performance

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test source compatibility

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build failed
Swift Test OS X Platform
Git Sha - 323abe749e92c655446f025896a21cfc590bbf6c

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test source compatibility

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test compiler performance

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 3, 2018

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build failed
Swift Test OS X Platform
Git Sha - 323abe749e92c655446f025896a21cfc590bbf6c

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build failed
Swift Test Linux Platform
Git Sha - 323abe749e92c655446f025896a21cfc590bbf6c

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 3, 2018

Build comment file:

Compilation-performance test failed

@jrose-apple jrose-apple force-pushed the jrose-apple:frozen-enums branch Mar 5, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 5, 2018

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test OS X Platform
Git Sha - 094ea9cc2d9bd12436229dbdfec45dc12ca907da

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test Linux Platform
Git Sha - 094ea9cc2d9bd12436229dbdfec45dc12ca907da

@jrose-apple jrose-apple force-pushed the jrose-apple:frozen-enums branch Mar 5, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 5, 2018

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test Linux Platform
Git Sha - a98032b19ea6c4dc78a3b835b1c6a277f8f48068

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 5, 2018

Build failed
Swift Test OS X Platform
Git Sha - a98032b19ea6c4dc78a3b835b1c6a277f8f48068

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 5, 2018

@swift-ci Please clean test macOS

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 6, 2018

Build failed
Swift Test OS X Platform
Git Sha - 906af8ee3b9951cee00c26382e8762f8ccde9d1c

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 6, 2018

@swift-ci Please test macOS

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 6, 2018

@swift-ci Please test compiler performance

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 6, 2018

@swift-ci Please test source compatibility

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 6, 2018

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Mar 6, 2018

Build comment file:

Summary for master smoketest

No regressions above thresholds

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
LLVM.NumLLVMBytesOutput 46,401,160 46,401,104 -56 -0.0%
time.swift-driver.wall 79.3s 79.8s 573.7ms 0.72%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 112,854 112,854 0 0.0%
AST.NumLoadedModules 16,158 16,158 0 0.0%
AST.NumTotalClangImportedEntities 332,837 332,837 0 0.0%
AST.NumUsedConformances 9,378 9,378 0 0.0%
IRModule.NumIRBasicBlocks 155,717 155,717 0 0.0%
IRModule.NumIRFunctions 84,191 84,191 0 0.0%
IRModule.NumIRGlobals 71,479 71,479 0 0.0%
IRModule.NumIRInsts 1,531,605 1,531,605 0 0.0%
IRModule.NumIRValueSymbols 140,694 140,694 0 0.0%
LLVM.NumLLVMBytesOutput 46,401,160 46,401,104 -56 -0.0%
SILModule.NumSILGenFunctions 50,146 50,146 0 0.0%
SILModule.NumSILOptFunctions 90,001 90,001 0 0.0%
Sema.NumConformancesDeserialized 459,094 459,094 0 0.0%
Sema.NumConstraintScopes 1,046,545 1,046,545 0 0.0%
Sema.NumDeclsDeserialized 2,985,001 2,985,001 0 0.0%
Sema.NumDeclsValidated 184,557 184,557 0 0.0%
Sema.NumFunctionsTypechecked 64,396 64,396 0 0.0%
Sema.NumGenericSignatureBuilders 86,155 86,155 0 0.0%
Sema.NumLazyGenericEnvironments 592,926 592,926 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 50,877 50,877 0 0.0%
Sema.NumLazyIterableDeclContexts 430,908 430,908 0 0.0%
Sema.NumTypesDeserialized 3,201,944 3,201,944 0 0.0%
Sema.NumTypesValidated 273,531 273,531 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
LLVM.NumLLVMBytesOutput 38,687,956 38,687,952 -4 -0.0%
time.swift-driver.wall 168.9s 169.5s 638.3ms 0.38%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 11,180 11,180 0 0.0%
AST.NumLoadedModules 462 462 0 0.0%
AST.NumTotalClangImportedEntities 32,180 32,180 0 0.0%
AST.NumUsedConformances 9,383 9,383 0 0.0%
IRModule.NumIRBasicBlocks 175,319 175,319 0 0.0%
IRModule.NumIRFunctions 60,588 60,588 0 0.0%
IRModule.NumIRGlobals 58,324 58,324 0 0.0%
IRModule.NumIRInsts 1,344,811 1,344,811 0 0.0%
IRModule.NumIRValueSymbols 107,610 107,610 0 0.0%
LLVM.NumLLVMBytesOutput 38,687,956 38,687,952 -4 -0.0%
SILModule.NumSILGenFunctions 25,087 25,087 0 0.0%
SILModule.NumSILOptFunctions 43,011 43,011 0 0.0%
Sema.NumConformancesDeserialized 154,896 154,896 0 0.0%
Sema.NumConstraintScopes 903,286 903,286 0 0.0%
Sema.NumDeclsDeserialized 255,208 255,208 0 0.0%
Sema.NumDeclsValidated 37,442 37,442 0 0.0%
Sema.NumFunctionsTypechecked 12,616 12,616 0 0.0%
Sema.NumGenericSignatureBuilders 7,680 7,680 0 0.0%
Sema.NumLazyGenericEnvironments 39,262 39,262 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 5,093 5,093 0 0.0%
Sema.NumLazyIterableDeclContexts 24,374 24,374 0 0.0%
Sema.NumTypesDeserialized 309,127 309,127 0 0.0%
Sema.NumTypesValidated 44,462 44,462 0 0.0%

jrose-apple added some commits Sep 14, 2017

Add Parse + Sema support for '@_frozen'
(currently spelled with an underscore to indicate its WIP state)

Later commits will handle imported enums correctly and implement the
checks for switch cases.
Diagnose uncovered switches on non-frozen enums
Warn in Swift 4 mode and error in Swift 5 mode when switching on a
non-frozen enum without providing a default case.

Note that this is a preliminary implementation, in order to test the
rest of the feature.
Put non-frozen enum exhaustivity diagnostics behind a frontend flag
...spelled '-enable-nonfrozen-enum-exhaustivity-diagnostics'. This
is for staging purposes.
Treat exhaustive enums as "non-resilient", and add a few more tests
"Formally non-resilient" in this new world means "the enum has a fixed
representation", which implies a fixed layout algorithm. We're not
there yet, but non-exhaustive enums should be able to be fixed-layout
as well by picking a general representation that won't need to grow.
Specifically, that's enums with raw types, and possibly also indirect
enums as well.

(It's likely the '_fixed_layout' /attribute/ on enums will go away,
but the concept of a fixed-layout enum is still useful.)

@jrose-apple jrose-apple force-pushed the jrose-apple:frozen-enums branch Mar 20, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 20, 2018

Rebased.

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 20, 2018

@swift-ci Please test source compatibility

@jrose-apple jrose-apple changed the title [WIP] Implementation for SE-0192 (frozen and non-frozen enums) Implementation for /most/ of SE-0192 (frozen and non-frozen enums) Mar 20, 2018

@DougGregor
Copy link
Member

DougGregor left a comment

This looks good. The space engine part is appropriately mind-boggling, but I've convinced myself it looks good. Nice work!

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)

This comment has been minimized.

@DougGregor

DougGregor Mar 20, 2018

Member

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.

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

They'll definitely get un-frozen. This commit did not remove any frozens that were currently there.

This comment has been minimized.

@slavapestov

slavapestov Mar 20, 2018

Member

... it did in stdlib/public/Platform/POSIXError.swift

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

…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 {

This comment has been minimized.

@DougGregor

DougGregor Mar 20, 2018

Member

Is there any point to a frozen internal enum?

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

It's versioned, so yes.

Name: EnumExhaustivity
Tags:
- Name: RegularEnumTurnedExhaustiveThenBackViaAPINotes
EnumExtensibility: open

This comment has been minimized.

@DougGregor

DougGregor Mar 20, 2018

Member

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;

This comment has been minimized.

@DougGregor

DougGregor Mar 20, 2018

Member

This is for staging while we settle on the "unknown case" spelling, right? It'll go away then?

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

It's mainly so I can land this part before SE-0192's review period ends, but yes.

@slavapestov
Copy link
Member

slavapestov left a 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)

This comment has been minimized.

@slavapestov

slavapestov Mar 20, 2018

Member

Do we ever need to check exhaustiveness in SILGen or anything else that runs after Sema? Because we won't have a useDC anymore

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

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

This comment has been minimized.

@slavapestov

slavapestov Mar 20, 2018

Member

You might be able to get away with removing this now that resilience is on

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

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,

This comment has been minimized.

@slavapestov

slavapestov Mar 20, 2018

Member

It's awkward to plumb the DC (and TC) through everything here -- can't we make them instance variables?

This comment has been minimized.

@jrose-apple

jrose-apple Mar 20, 2018

Author Member

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)

This comment has been minimized.

@slavapestov

slavapestov Mar 20, 2018

Member

... it did in stdlib/public/Platform/POSIXError.swift

jrose-apple added some commits Dec 15, 2017

[ClangImporter] Translate Clang's enum_extensibility to @_frozen
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.
Ban @_fixed_layout on enums in favor of @_frozen
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.)
[ClangImporter] Prefer later enum_extensibility over earlier ones
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.
[PrintAsObjC] Use enum_extensibility to represent @_frozen
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.
[stdlib] Remove '@_frozen' from enums that shouldn't be frozen
Error codes, FloatingPointRoundingRule, Mirror.AncestorRepresentation,
Mirror.DisplayStyle, and PlaygroundQuickLook.
[stdlib] Add '@_frozen' to overlay enums that need it
That's just ARCamera.TrackingState and DispatchTimeoutResult.
Verified with the framework owners at Apple.
@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 20, 2018

We do have the "evolution" tests for non-frozen enums by testing private cases, but I'll add the ones for -enable-testing!

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Mar 20, 2018

@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

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 20, 2018

Sorry, I forgot that one already landed. :-) See #14724.

@jrose-apple jrose-apple force-pushed the jrose-apple:frozen-enums branch to 348f966 Mar 20, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 20, 2018

Addressed feedback.

@swift-ci Please smoke test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Mar 21, 2018

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 unknown is not yet implemented. (That's #14382, which I'm currently working on rebasing.)

@jrose-apple jrose-apple merged commit d150f96 into apple:master Mar 21, 2018

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@jrose-apple jrose-apple deleted the jrose-apple:frozen-enums branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment