[WIP] Implementation of non-exhaustive enums

[jrose-apple]

Initial implementation of https://github.com/jrose-apple/swift-evolution/blob/non-exhaustive-enums/proposals/nnnn-non-exhaustive-enums.md, which I plan to submit for swift-evolution review soon.

[jrose-apple]

I haven't done the importer parts yet, and there are still more tests to write, but I thought I should throw the basic thing up ahead of that. @CodaFi, mind taking a look at the switch statement part? (the last commit)

[slavapestov]

Hi @jrose-apple, I already had an exhaustiveness check in SILGenPattern.cpp that's driven by hasFixedLayout(). It should probably be removed now?

Also I think we should redo hasFixedLayout() to check the exhaustiveness attribute if the given decl is an EnumDecl. Right now it looks for @_fixed_layout, which should probably be banned on enums (and all tests updated).

I do want to decouple "use resilient access" from "non-exhaustive" though, so it's up to you how much you want to redo the hasFixedLayout() stuff. I would prefer that hasFixedLayout() returns true if the module is non-resilient or the enum is exhaustive, since this is what IRGen uses to decide how to project and inject enum cases.

[jrose-apple]

Thanks for listing out all these tasks; that's way easier then trying to track them down myself. I think it's okay to go to review without these, but they're definitely things I consider part of the work.

[CodaFi]

These aren't just tags, they're powering the constexpr bit-mangling/pattern-matching gadget in the rest of this file. With these values you get easy uniqueness of bit sequences closed under |.

[CodaFi]

It works either way, I guess but I would prefer the tag values become explicit then.

[jrose-apple]

That doesn't make sense to me. Your pairwise case works for any two uint8_ts, and that's already encoded in the enum's underlying type. And indeed, it would not work for arbitrary flags, and so making the constants powers of two is misleading.

[CodaFi]

That doesn't make sense to me.

Let me be clearer: By "easy" I mean I could dump the binary value of a pair of space kinds and easily see from the bits which ones I had. Either way works for me, but if it's misleading to treat it as an option set I would still like the tag values to be explicitly stated in the source.

[jrose-apple]

We don't do that for any other enums except where the raw value is significant, so again it seems weird to include the values (especially when there's only 5 of them and the debugger can print them for you). I'm also not sure how this is really any different with regards to dumping. Before you'd get values like 0x0408, 0x0210, etc; now you get 0x0203 and 0x0104.

[CodaFi]

WRT Sema making the space atomic based on the attribute is the right approach. There is a special case to optimize here: If we have an open enum or the product of all open enums with no default clause then we don't need to go through the space subtraction and size checks and stuff. Just add the default and bail.

[jrose-apple]

A problem I found with this approach in the Space Engine: in Swift 4 mode, we should still distinguish between "all cases covered but the enum is non-exhaustive" (warning) and "known cases are uncovered" (error). I think I'm going to have to make a new space kind, or at least a special kind of Constructor space that's not resolvable.

[CodaFi]

Can't that be determined from the type of the switch subject? Because spaces for open enums are atomic, in the general case you can walk into a tuple/constructor space and check for that atomic type space that resolves to an open enum.

[CodaFi]

Alternatively, you could not make a distinction between open and closed enum spaces and then make the determination about exhaustiveness by doing the aforementioned type check.

[jrose-apple]

@slavapestov SILGen work is up.

@CodaFi I don't understand what you're getting at, which I'm pretty sure is just a sign of my unfamiliarity with the problem space. Just to be sure we're on the same page, this is the behavior I'm hoping to get in Swift 4 mode:

// _nonexhaustive public enum X {
//   case a, b
// }

switch getX() {
case .a: break
case .b: break
} // warning

switch getX() {
case .a: break
} // error

My current patch downgrades these both to warnings, because the non-exhaustive enum is treated as an opaque space, and then the constructors don't subtract from it.

[CodaFi]

Alright, so we currently have four diagnosable cases we're worried about here:

1. (exhaustive) enum + switch covered by known values:

Easy. Everything is fine.

2. (exhaustive) enum + switch not covered by known values:

Easy. Diagnose as an error in Swift 4 mode, warning in 3 in 3 mode under @ _downgrade_exhaustivity_check, and present missing cases.

3. _nonexhaustive enum + switch covered by known values:

Easy. If we have a default, we're done. Otherwise offer to insert one.

4. _nonexhaustive enum + switch not covered by known values:

Hard. Need to simultaneously offer missing cases as an error + a default.

The last case is the one we care about here, and it can be detected without defining a new kind of space (you would be duplicating the Type space if you did this). Roll back this check so the space engine performs subtractions the way it is supposed to, then make sure that decomposition only occurs for the in-module cases - this should guarantee that switches over out-of-module cases should pass through the space subtraction routine without affecting coverage.

Now, when you go to diagnose missing cases, you examine the type of the subject expression to see if you need to emit a warning or an error. This should align with the four cases above.

[jrose-apple]

Ah, sorry, I think you got case 2 and case 3 mixed up. Case 2 is always an error. Case 3 needs to be an error in Swift 5 and a warning in Swift 3/4. Case 4 needs to be an error, always, with some fix-it.

[CodaFi]

Ah, sorry, I think you got case 2 and case 3 mixed up

Yeah, I mixed my memory of implementing @_downgrade_exhaustivity_check into this (which adds a whole other dimension of pain).

Case 3 needs to be an error in Swift 5 and a warning in Swift 3/4.

Does this mean that we consider it a compatible change to mark a previously exhaustive (I assume this is the default) enum as nonexhaustive? If not, then this should always be an error.

[CodaFi]

This was the diff I needed to get that fixit you wanted wired up. We can definitely offer a more specific diagnostic in this case

diff --git a/lib/Sema/TypeCheckSwitchStmt.cpp b/lib/Sema/TypeCheckSwitchStmt.cpp
index 4d10c824f9..dcf2edb091 100644
--- a/lib/Sema/TypeCheckSwitchStmt.cpp
+++ b/lib/Sema/TypeCheckSwitchStmt.cpp
@@ -972,11 +972,7 @@ namespace {
       }
 
       static bool canDecompose(Type tp, const DeclContext *DC) {
-        if (tp->is<TupleType>() || tp->isBool())
-          return true;
-        if (const EnumDecl *enumDecl = tp->getEnumOrBoundGenericEnum())
-          return enumDecl->isExhaustive(DC);
-        return false;
+        return tp->is<TupleType>() || tp->isBool() || tp->getEnumOrBoundGenericEnum();
       }
 
       // HACK: Search the space for any remaining cases that were labelled
@@ -1119,12 +1115,16 @@ namespace {
           spaces.push_back(projection);
         }
       }
-      
+
+      bool isNonExhaustiveSubject = false;
+      if (auto *enumDecl = subjectType->getEnumOrBoundGenericEnum())
+        isNonExhaustiveSubject = enumDecl->isExhaustive(DC);
+
       Space totalSpace(subjectType, Identifier(),
                        Space::isSwift4NonExhaustiveEnum(TC, DC, subjectType));
       Space coveredSpace(spaces);
       size_t totalSpaceSize = totalSpace.getSize(TC, DC);
-      if (totalSpaceSize > Space::getMaximumSize()) {
+      if (!isNonExhaustiveSubject && totalSpaceSize > Space::getMaximumSize()) {
         // Because the space is large, we have to extend the size
         // heuristic to compensate for actually exhaustively pattern matching
         // over enormous spaces.  In this case, if the covered space covers
@@ -1142,6 +1142,11 @@ namespace {
       
       auto uncovered = totalSpace.minus(coveredSpace, TC, DC).simplify(TC, DC);
       if (uncovered.isEmpty()) {
+        // If we've got here with a non-exhaustive subject then all we need is
+        // a `default`.
+        if (isNonExhaustiveSubject) {
+          diagnoseMissingCases(/*justNeedsDefault*/ true, Space());
+        }
         return;
       }

[jrose-apple]

Case 3 needs to be an error in Swift 5 and a warning in Swift 3/4.

Does this mean that we consider it a compatible change to mark a previously exhaustive (I assume this is the default) enum as nonexhaustive? If not, then this should always be an error.

This is the reality of introducing nonexhaustive enums for the first time: existing code is written without default cases. So for people transitioning to Swift 5 for the first time (or being diligent in Swift 4.1), yes, enums will go from exhaustive to non-exhaustive. (See the proposal for more details on the default behavior.)

Thanks for the patch! I'll incorporate that into the series for now. It's probably about good enough to kick off the review, anyway.

[jrose-apple]

Still more work to do, but I think this is sufficient for proposal review!

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 2da03935b230f1187ab83bb7fb65750b52e5cc71

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - b8713c7264e9ab3c06a7811c3f06b76c7e063aba

[jrose-apple]

@swift-ci please test

[jrose-apple]

@swift-ci Please clean test Linux

[jrose-apple]

@swift-ci Please test source compatibility

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - b8713c7264e9ab3c06a7811c3f06b76c7e063aba

[jrose-apple]

@swift-ci Please test source compatibility

[jrose-apple]

Still to do:

• Incorporate Robert's patch above
• Make the warning/error case I was talking about above work in more complicated scenarios
• Use a fixed layout for enums with raw types (a trivial 32-bit value)
• Vastly improve diagnostics; in particular they should mention that the enum is non-exhaustive
• Consider synthesizing a validating init(rawValue:) for imported closed enums
• Replace @_fixedLayout in the standard library's resilient build with _exhaustive

What's not included here is marking any enums non-exhaustive in the standard library or overlays; we want to hold off on that until we're ready to take Swift 5 changes.

[jrose-apple]

Picking this up again. Rebased, but still need to revisit the patches and see what's left.

@swift-ci Please test

[jrose-apple]

Rebased again! Follow-up work (will take place in other PRs, to keep this one from growing any more):

• Add unknown case, or however we end up spelling it. This subsumes a good chunk of the diagnostic work I was planning earlier because it's now necessary up front
• Improve run-time diagnostics; in particular they should mention that the enum is non-frozen
• Consider synthesizing a validating init(rawValue:) for imported frozen enums

[jrose-apple]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 4ad31959beed15ff4832bf63d7ea798ef4f18f3e

[jrose-apple]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 4ad31959beed15ff4832bf63d7ea798ef4f18f3e

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 4ad31959beed15ff4832bf63d7ea798ef4f18f3e

[jrose-apple]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 29a8890

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 29a8890

[jrose-apple]

The proposal took a new direction, implemented in #14945. Closing.