[SE-0194] Implement deriving collections of enum cases #13655

CodaFi · 2018-01-01T02:07:32Z

This is the corresponding implementation for swiftlang/swift-evolution#114 "Deriving collections of enum cases".

Prior Discussion

swift-evolution discussion thread (other related threads are linked in the proposal).

Proposal Text

Rendered Markdown.

Resolves #42653.

CodaFi · 2018-01-01T02:10:03Z

恭賀新年！ Happy New Year.

Poor swift-ci, never gets a break though.

@swift-ci please test

swift-ci · 2018-01-01T02:36:21Z

Build failed
Swift Test Linux Platform
Git Sha - a726f8f9fd574258701ca5fb89df8c0532c1987a

swift-ci · 2018-01-01T02:56:26Z

Build failed
Swift Test OS X Platform
Git Sha - a726f8f9fd574258701ca5fb89df8c0532c1987a

jtbandes · 2018-01-01T03:36:24Z

test/stdlib/ValueEnumerableTests.swift

+  case garply
+}
+
+extension InExtension: ValueEnumerable {}


We should probably have test cases to show that extensions can’t be used to derive a conformance for an enum from another module or imported from a header.

The proposal wasn't clear on this. Equatable, Hashable, and Codable all disallow synthesis in extensions even for the same module. If that's the desired behavior I can remove this section of the test.

CodaFi · 2018-01-01T23:50:52Z

@swift-ci please smoke test

jtbandes · 2018-01-07T23:18:03Z

stdlib/public/core/CompilerProtocols.swift

+/// For any Swift enumeration where every case does not have an associated
+/// value, the Swift compiler can automatically fill out the `ValueEnumerable`
+/// conformance. When defining your own custom enumeration, specify a
+/// conformance to `ValueEnumerable` either in an extension or on the type


outdated: synthesizing in an extension no longer allowed

jtbandes · 2018-01-07T23:18:34Z

stdlib/public/core/ReflectionLegacy.swift

@@ -269,6 +269,20 @@ internal struct _StructMirror : _Mirror {
  internal var disposition: _MirrorDisposition { return .`struct` }
 }

+@_inlineable // FIXME(sil-serialize-all)


What does sil-serialize-all mean?

sil-serialize-all is a precursor to @_inlineable that stdlib used to build with. Does what it says on the package: all the SIL used to be serialized so the optimizer could work. The FIXME is because somebody needs to go through and evaluate whether all these functions are profitable to inline.

jtbandes · 2018-01-07T23:22:50Z

lib/AST/ASTContext.cpp

@@ -215,7 +215,10 @@ FOR_KNOWN_FOUNDATION_TYPES(CACHE_FOUNDATION_DECL)
  /// func _stdlib_isOSVersionAtLeast(Builtin.Word,Builtin.Word, Builtin.word)
  //    -> Builtin.Int1
  FuncDecl *IsOSVersionAtLeastDecl = nullptr;
-
+
+  /// func _stdlib_enumerateCaseValues<Enum: ValueEnumerable>(Enum.Type) -> [T]


nit: should be -> [Enum]. How did you choose this name? To me enumerate sounds like a verb here, but I would expect a noun. Perhaps enumeration or enum? or even a get prefix?

enumerate is the operative verb, as in “to determine the number of”. Good catch.

You can decide whether the API Design Guidelines apply here, but:

Name functions and methods according to their side-effects
Those without side-effects should read as noun phrases, e.g. x.distance(to: y), i.successor().

jtbandes · 2018-01-07T23:30:42Z

stdlib/public/core/CompilerProtocols.swift

+public protocol ValueEnumerable {
+  associatedtype ValueCollection: Collection
+    where ValueCollection.Element == Self
+  /// Creates a collection of all values of this type.


Why creates?

DougGregor

Very minor comments/requests, but this is looking great t me.

DougGregor · 2018-03-01T05:45:01Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+  if (!metaTy || metaTy->hasError())
+    return nullptr;
+
+  return BoundGenericType::get(arrayDecl, Type(),


Why not build an ArraySliceType to get the nice sugar here?

DougGregor · 2018-03-01T05:46:38Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+                               {metaTy->getRValueInstanceType()});
+}
+
+static Type deriveRawRepresentable_AllCases(TypeChecker &tc, Decl *parentDecl,


... you mean deriveCaseIterable_AllCases, not deriveRawRepresentable_AllCases

DougGregor · 2018-03-01T05:47:05Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+static BoundGenericType *computeAllCasesType(NominalTypeDecl *enumType) {
+  auto *arrayDecl = enumType->getASTContext().getArrayDecl();
+  assert(arrayDecl);
+  auto metaTy = enumType->getInterfaceType();


I think you mean getDeclaredInterfaceType here!

DougGregor · 2018-03-01T05:49:34Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+void deriveCaseIterable_enum_getter(AbstractFunctionDecl *funcDecl) {
+  auto *parentDC = funcDecl->getDeclContext();
+  auto *parentEnum = parentDC->getAsEnumOrEnumExtensionContext();
+  auto enumTy = parentEnum->getDeclaredType();


Hmm... How about using parentDC->getDeclaredTypeInContext() here, so it does the right thing should we loosen the restriction on synthesis to support (e.g.) extensions in the same file.

DougGregor · 2018-03-01T05:50:35Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+    return nullptr;
+
+  return BoundGenericType::get(arrayDecl, Type(),
+                               {metaTy->getRValueInstanceType()});


Well, okay, you can getRValueInstanceType() here, but I'd prefer that you use getDeclaredInterfaceType() above instead.

CodaFi · 2018-03-01T06:14:54Z

I've addressed what you've reviewed so far. I'll cast off the [DO NOT MERGE] and squash the patch when I'm less sleep-deprived.

DougGregor · 2018-03-01T06:25:22Z

Sounds great!

jtbandes · 2018-03-04T07:29:11Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+    return nullptr;
+  }
+
+  // We can only synthesize RawRepresentable for enums.


Missed one RawRepresentable here.

jtbandes · 2018-03-04T07:29:19Z

lib/Sema/DerivedConformances.cpp

@@ -192,6 +203,10 @@ ValueDecl *DerivedConformance::getDerivableRequirement(TypeChecker &tc,
    if (name.isSimpleName(ctx.Id_RawValue))
      return getRequirement(KnownProtocolKind::RawRepresentable);

+    // RawRepresentable.AllCases


jtbandes · 2018-03-04T07:34:27Z

test/Sema/enum_conformance_synthesis.swift

@@ -23,6 +24,7 @@ enum Generic<T> {

 if Generic<Foo>.A == .B { }
 var gaHash: Int = Generic<Foo>.A.hashValue
+print(Generic<Foo>.allCases)


Is this left over from debugging or intended to be included in this file? I suppose it doesn't matter since we're -typechecking only.

To suppress an unused lvalue warning

I think _ = would be (mildly) preferable

jtbandes · 2018-03-04T07:44:57Z

lib/Sema/DerivedConformanceCaseIterable.cpp

+  if (!canDeriveConformance(targetDecl))
+    return nullptr;
+
+  if (assocType->getName() == tc.Context.Id_AllCases) {


Doesn't the caller already check this?

jtbandes · 2018-03-04T07:45:18Z

lib/Sema/DerivedConformances.h

+/// Derive a CaseIterable type witness for an enum if it has no associated
+/// values for any of its cases.
+///
+/// \returns the derived member, which will also be added to the type.


Are the comments on these two functions supposed to be identical? (I also find it a bit strange that the functions are named identically, but I realize that's a pattern that already exists in this file — nonetheless, did you consider something like deriveCaseIterable_property_allCases and deriveCaseIterable_associatedtype_AllCases? Is there some motivation I'm missing here for this naming convention?)

natecook1000 · 2018-03-09T00:05:48Z

test/stdlib/CaseIterableTests.swift

+    case quux
+  }
+
+  expectEqual(SimpleEnum.allCases.count, 3)


What's type(of: SimpleEnum.allCases) here? Is it just Array<SimpleEnum>?

For now. I hope we can come up with something more creative.

CodaFi · 2018-03-09T05:21:01Z

With the acceptance I’m going to merge this through and file SRs to investigate the following outstanding issues:

SR-7151: A generalized implementation of synthesis that accounts for availability attributes and maintains the source-order invariant.
SR-7152: Potentially changing the Collection we default to from an Array.

Implements the minimum specified by the SE-proposal. * Add the CaseIterable protocol with AllCases associatedtype and allCases requirement * Automatic synthesis occurs for "simple" enums - Caveat: Availability attributes suppress synthesis. This can be lifted in the future - Caveat: Conformance must be stated on the original type declaration (just like synthesizing Equatable/Hashable) - Caveat: Synthesis generates an [T]. A more efficient collection - possibly even a lazy one - should be put here.

CodaFi · 2018-03-09T05:23:26Z

@swift-ci please smoke test

CodaFi · 2018-03-09T07:06:33Z

@swift-ci please test source compatibility

CodaFi · 2018-03-09T17:43:09Z

⛵️

DougGregor · 2018-03-12T16:58:25Z

Awesome! @CodaFi , can you update the ChangeLog?

Coeur · 2018-03-31T16:00:48Z

@CodaFi @DougGregor Is there a default implementation of CaseIterable for Hashable enums? For instance, could we add the following extension to Swift?

extension CaseIterable where Self: Hashable {
    static var allCases: [Self] {
        return [Self](AnySequence { () -> AnyIterator<Self> in
            var raw = 0
            return AnyIterator {
                let current = withUnsafeBytes(of: &raw) { $0.load(as: Self.self) }
                guard current.hashValue == raw else {
                    return nil
                }
                raw += 1
                return current
            }
        })
    }
}

Or is it not needed, and CaseIterable will already work by itself for Hashable enums?
What about enum that conforms to String?

natecook1000 · 2018-03-31T18:53:49Z

@Coeur Any enumeration without associated values gets its allCases requirement synthesized without any extra work, even when it has a raw value type like String:

enum Suit: String, CaseIterable {
  case spades, hearts, clubs, diamonds
}
Suit.allCases
// [Suit.spades, Suit.hearts, Suit.clubs, Suit.diamonds]
Suit.allCases.map({ $0.rawValue })
// ["spades", "hearts", "clubs", "diamonds"]

CodaFi added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Jan 1, 2018

jtbandes reviewed Jan 1, 2018

View reviewed changes

jtbandes reviewed Jan 7, 2018

View reviewed changes

CodaFi force-pushed the ace-attorney branch from f6ad33e to 42dd2a5 Compare January 8, 2018 16:52

CodaFi force-pushed the ace-attorney branch 2 times, most recently from 889794f to 7113a2b Compare March 1, 2018 04:26

CodaFi requested a review from DougGregor March 1, 2018 04:27

DougGregor approved these changes Mar 1, 2018

View reviewed changes

CodaFi force-pushed the ace-attorney branch from 7113a2b to 8cc7fdb Compare March 1, 2018 06:07

CodaFi force-pushed the ace-attorney branch from 8cc7fdb to 0ff5df6 Compare March 1, 2018 17:23

CodaFi changed the title ~~[DO NOT MERGE] Implement ValueEnumerable~~ [SE-0194] Implement deriving collections of enum cases Mar 1, 2018

CodaFi force-pushed the ace-attorney branch 3 times, most recently from c216900 to c73c836 Compare March 1, 2018 17:44

jtbandes reviewed Mar 4, 2018

View reviewed changes

CodaFi force-pushed the ace-attorney branch from c73c836 to 8d909a3 Compare March 4, 2018 16:58

natecook1000 reviewed Mar 9, 2018

View reviewed changes

CodaFi force-pushed the ace-attorney branch from 8d909a3 to dac0689 Compare March 9, 2018 05:23

CodaFi merged commit 72e4224 into swiftlang:master Mar 9, 2018

CodaFi deleted the ace-attorney branch March 9, 2018 17:43

CodaFi mentioned this pull request May 3, 2018

[SR-30] Enumerations could have a "allValues" property #42653

Closed

AnthonyLatsis removed the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Mar 23, 2023

[SE-0194] Implement deriving collections of enum cases #13655

[SE-0194] Implement deriving collections of enum cases #13655

Uh oh!

Conversation

CodaFi commented Jan 1, 2018 • edited by AnthonyLatsis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prior Discussion

Proposal Text

Uh oh!

CodaFi commented Jan 1, 2018

Uh oh!

swift-ci commented Jan 1, 2018

Uh oh!

swift-ci commented Jan 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jan 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Mar 1, 2018

Uh oh!

DougGregor commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Mar 9, 2018

Uh oh!

CodaFi commented Mar 9, 2018

Uh oh!

CodaFi commented Mar 9, 2018

CodaFi commented Jan 1, 2018 •

edited by AnthonyLatsis

Loading

CodaFi Jan 1, 2018 •

edited

Loading

CodaFi commented Mar 9, 2018 •

edited

Loading

Coeur commented Mar 31, 2018 •

edited by AnthonyLatsis

Loading