SE-0192 — Non-Exhaustive Enums — review #2

The second review of the review of SE 0192 — Non-Exhaustive Enums begins now and runs through March 23, 2018.

The second review is follow up to the original review based on feedback on the thread and from the Core Team.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0192-non-exhaustive-enums.md

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. When emailing the review manager directly, please keep the proposal link at the top of the message.

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal?

  • Is the problem being addressed significant enough to warrant a change to Swift?

  • Does this proposal fit well with the feel and direction of Swift?

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Thanks,
Ted Kremenek
Review Manager

3 Likes

Hello,

The proposal talks about the "ignore pattern" case _. But it does not talk about the ignore pattern when it comes as case let x:.

I think the proposal would be enhanced if it would contain a clear description of the kind of value one can get with case let x:, and particularly if one should be prepared to deal with "funny" values. This would enhance the legibility of the proposal for readers who wonder what will be the consequences of this proposal for their application code.

The way I understand the proposal, I believe that one could get any value from case let x:, including future and private ones. But I really think that this should be written black on white in the proposal, because it may surprise some people, and will have a real impact on documentation and usage of non-frozen enums.

3 Likes

I spotted an unknown case which needs updating under ‘mixing unknown and default’. Otherwise, looks good.

edit: wrong section

The final proposal all looks good to me except one detail:

The compiler will warn if all enums in the pattern being matched by unknown are explicitly annotated as frozen. This is a warning rather than an error so that annotating an enum as frozen remains a source-compatible change. If the pattern contains any enums that are implicitly frozen (i.e. because it is a user-defined Swift enum), unknown is permitted, in order to make it easier to adapt to newly-added cases.

It seems odd to me that frozen enums treated differently than user-defined enums. IMO, the simplest rule would be that unknown is only allowed when default would otherwise be necessary (i.e. when an exhaustive switch of known cases is not sufficient).

I want to commend Jordan for all the work he’s put into driving this forward, and his willingness to take feedback into account and update the proposal. For something with a fairly small API surface, this topic sure did generate a lot of discussion!

I think it was the right decision to limit the scope of the change to just C and StdLib enums, and the single-word “unknown” spelling maintains the level of convenience we are used to with “default”. I’m pretty happy with where the proposal landed, including the warnings and errors.

+1

7 Likes

+1

Thank you for the effort you put into updating the proposal. Very happy with the result!

Couple of notes/questions:

-- Actually it seems there are 3 "kinds" of enums introduced in proposal: not just "non-frozen" vs "frozen" , but also "implicitly frozen". It is not clear we have exactly such separation (at least for me) until you read rules regarding "implicitly frozen" enums. I mean, probably this should be more clearly described in the beginning.

  • "If the pattern contains any enums that are implicitly frozen (i.e. because it is a user-defined Swift enum), unknown is permitted, in order to make it easier to adapt to newly-added cases."

-- I believe this sentence is about "exhaustive" switch over "implicitly frozen" enum, when all cases explicitly enumerated, right? I.e. when you have exhaustive switch over your own enum, you are allowed to have 'unknown:' in it without the warning. Of course, if new case is added to our enum, without fixing the 'switch', such 'uknown:' becomes reachable and so warning will be generated.
Probably this should be described with more details.

  • If a library author changes an enum previously marked frozen to make it non-frozen, the compiler will likewise produce an error for any switch that does not have a default case or _ pattern, noting that it needs either a default or unknown case.

-- Even if library will send only previously "frozen" case values? I mean if library sends an enum case that is expected by switch but switch has no "defalt/unknown:" branch - should this be a run-time error?

P.S. And YES, thank you Jordan for all the work on this proposal and on its details!

I don't think we should allow unknown in "implicitly frozen" enums. What purpose does that serve?

If the pattern contains any enums that are implicitly frozen (i.e. because it is a user-defined Swift enum), unknown is permitted, in order to make it easier to adapt to newly-added cases.

If the compiler knows its a frozen enum, then there's no point to allowing unknown. Or is this to guard for externally-updatable libraries beyond the standard library (ex Vapor or whatever)?

Yes, this is to support new cases in 3rd party source files - when you updated some such file, new case appeared and you don't need to fix this in your switch at this second, 'unknown:' branch will handle this new case. You may don't use 'unknown:' in your switch - then you'll have standard error(switch must be exhaustive) in case of new case in external source file.

1 Like

A couple of people have posted here asking why unknown is allowed in certain places. I think the simplest mental model is to view unknown as a synonym for default, which produces a warning if it is statically known to be reachable.

This view covers all aspect of the proposal except one:

If I were to suggest changing any part of the proposal, it would be to eliminate that line and thus allow unknown in all the same places as default. They have identical functionality after all, so the only difference should be when warnings are raised.

Moreover, the very next line in the proposal reads:

This is vacuously true when there are zero enums in the pattern, and thus should apply.

I'd ask someone to provide an illustration of this in code. This will help to understand the details.

I think this proposal is improved from the previous iteration but may require just some additional modification.

First, I'm not terribly happy with an official dividing line between the standard library/overlays and all else. It'd be quite different if the proposal commented specifically that the goal is eventually to enable third-party libraries to be able to make this distinction if they opt into the same ABI stability/library versioning guarantees as core libraries, even if that's not ready from the get-go.

Second, my comments about the unknown case are known: I continue to think that the optimal solution is an attribute modifying default. As @Nevin points out, it makes for a simple mental model to think of unknown as a flavor of default, and therefore naming it as such would be the most teachable and intuitive design, in my view. However, the proposed design is superior to some of the alternatives put forward in earlier discussions.

Third, like @Nevin has said, what also caught my eye earlier in the day when I read the proposal was the following line:

It is an error to use unknown with a pattern that does not contain any enums at all.

This seems unnecessary and contrary to straightforward mental models for other "catch-all clauses." I recall (but am not intimately familiar with the details) that some Obj-C types are bridged as structs with static members for various reasons; it seems like making the use of unknown an error instead of a warning particularly in this scenario would be a nuisance that turns on an implementation detail which really shouldn't be foremost in the user's mind.

Fourth and last, I'm delighted at the attention to detail in calling the opposite of frozen enums "non-frozen." That is indeed much more correct than "unfrozen" for the reasons detailed in the text. However, might I humbly suggest that the true opposite of "frozen" is "fresh."

5 Likes

Upon reading the unknown patterns section, I believe the reasons given there are misleading.

Let's start with the running example that originally uses the unknown case syntax:

switch excuse {
case .eatenByPet
  // …
unknown:          // warning: not all known enum cases have been matched
  // …
}

Now let's assume we introduced a ._ pattern having the following semantics:

  1. it successfully matches any case of a non-frozen enum;
  2. there will be a warning if it can match a statically known case of that enum.

First, rewrite the aforementioned example to become familiar with the new pattern matching syntax:

switch excuse {
case .eatenByPet
  // …
case ._:          // warning: not all known enum cases have been matched
  // …
}

Then rewrite the example given in the unknown patterns section of SE-0192, but make it shorter for clarity:

switch (excuse, notifiedTeacherBeforeDeadline) {
case (.eatenByPet, true):
  // …
case (._, true):  // warning: not all known enum cases have been matched
  // …
default:
  // …
}

As defined by the rule (2), the warning should be emitted because there exists a statically known enum case that the ._ pattern can match.

Adding more cases is tedious, because the cases in switch are matched one after another:

switch (excuse, notifiedTeacherBeforeDeadline) {
case (.eatenByPet, true): // 1
  // …
case (._, true): // 2 - warning: not all known enum cases have been matched
  // …
case (.thoughtItWasDueNextWeek, _): // 3
  // …
case (_, false): // 4
  // …
}

In particular, the input (.thoughtItWasDueNextWeek, true) would result in case 2 being chosen rather than case 3. However this behavior is not as surprising as presumed in SE-0192, because ._ pattern is almost the same as _ pattern except for a warning the former pattern can possibly produce.

Does this proposal fit well with the feel and direction of Swift?

No, unknown patterns would fit with them much better, or it was not demonstrated that such patterns don't fit.


Another indicator of potential problems is that SE-0192 'only affects C enums and enums in the standard library and overlays', but not user-defined Swift enums. So what if I use a third-party framework written in pure Swift and the authors of that framework are going to add more cases to their enums?

For that, I imagine having a case _ syntax for defining non-frozen enum in pure Swift:

enum Excuse {
  case eatenByPet
  case thoughtItWasDueNextWeek
  case _
}

The final case _ is not a real enum case, but it serves as an indicator that more cases can be added to that enum in the future. Such a syntax would perfectly complement ._ pattern matching, I suppose.


One more question is how to pass an unknown case value to an third-party API that expects a value of a non-frozen enum type. An unknown case value could be either an invalid value, or a "private case" value, or a case value defined in the future version of that API.

Obviously, the initializer of a non-frozen enum should be non-failable, and it should preserve the rawValue passed to it.

In addition, it could be possible to define an unknown value as an associated value of a non-frozen enum type, so that anyone can inspect it with pattern matching.

That said, a enum with a "private case" can be fully defined in pure Swift:

enum PaperSize: Int { // a enum with the raw type
                      // init(rawValue:) is non-failable for non-frozen enums
  case usLetter = 0
  case a4 = 1
  case photo4x6 = 2
  case _(Int)         // an associated value of a non-frozen enum
}

extension PaperSize {
  // a "private case" of a non-optional type
  static let stickyNote: PaperSize = PaperSize(rawValue: 255)
}

struct Example {
  var paperSize: PaperSize {
    get {
      return .stickyNote
    }
    set {
      switch newValue {
        case .usLetter:
          print("usLetter")
        case .a4:
          print("a4")
        case ._(_):   // warning: not all known enum cases have been matched
          print("an unknown case with an associated value")
      }
    }
  }
}
1 Like

In addition, it was previously noted that if case pattern matching doesn't make sense when used with unknown pattern, as 'there is no way to prevent it from producing a warning'.

But if you treat switch as a sequence of if-else statements, it becomes clear that:

  1. it doesn't make sense to use the unknown pattern in the first case of a switch statement as well;
  2. it makes perfect sense to use the unknown pattern after all known cases were considered in the sequence of if-else statements.
struct Example {
  func inspect(paperSize newValue: PaperSize) {
    if newValue == .usLetter || newValue == .a4 {
      print("usLetter or a4")

      // use normal pattern matching for the known enum case
    } else if case .photo4x6 = newValue {
      print("photo4x6")

      // use new pattern matching for the unknown enum case
      // no warning produced, because all known enum cases have been matched
    } else if case ._(let rawValue) = newValue {
      print("an unknown case with an associated value \(rawValue)")
    }
  }
}

What is your evaluation of the proposal?
I am very happy with the proposal as written, with unknown: as the spelling for the exhaustiveness warning case. I think it's important for it to be a single word to be as convenient to write as the default: case.

The hard distinction between enums declared in the standard library and overlays and those from other sources is a bit inelegant, but I appreciate that a future plan to allow third-party libraries to opt-in to enum resilience is formally included in the introduction section; 'the features described here may be used by third-party libraries in future'.

Is the problem being addressed significant enough to warrant a change to Swift?
Yes. Resilience is of critical importance to supporting Swift codebases in the future. This proposal limits source code churn to the best of its ability, whilst neatly accommodating the new behaviours.

Does this proposal fit well with the feel and direction of Swift?
Yes. Unlike the initial drafts of the proposal, exhaustiveness warnings are supported which are a defining feature of writing Swift code.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
N/A

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I followed along with all of the discussions from the last few months on this topic.

What is your evaluation of the proposal?
+1

Is the problem being addressed significant enough to warrant a change to Swift?

Yes, having default resilience measures for changes in C/Obj-C/Apple Library enums without breaking source or binary compatibility is a huge win.

Does this proposal fit well with the feel and direction of Swift?

Indeed, the addition of "unknown" to maintain exhaustiveness checking in such cases will be of great benefit to those who have been relying on this feature extensively and the rational behind it's usage is clear. Additionally, the tabling of "Exactly what it means to be a 'library with binary compatibility concerns'" as a larger discussion seems like the appropriate adjustment to the proposal.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Comparison with other languages sums up very well how other languages handle the issues of source and binary compatibility with enum changes very well. I believe that the design in this proposal is not only a step in the right direction for compatibility but a model that future languages should follow.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I've been a part of the discussions for many months now.

Thank you Jordan for driving this and listening to and adjusting to all the discussions - it's really encouraging to be a part of this language's community with that kind of direction at the helm.

I guess the proposal won't be changed anymore, but I wonder if adding #warn/#error directives really has not been discussed (at least I couldn't find it in the alternatives section)?
They would allow to chose wether a warning or an error is triggered, unknown cases could be treated diffently depending on library-supplied properties, and they could be used for other things as well (a better replacement for abstract methods than fatalError).

Small bits of feedback (I'll put larger ones in a separate post):

  • @VladimirS is correct about why unknown is allowed for your own enums—it's a useful feature that we get nearly for free. (I think it was his suggestion originally.)

  • I think I agree with @Nevin's statement that there's no reason to make unknown on an enum-less value into an error; a warning is sufficient. That's also consistent with the more general philosophy about warnings in Swift: "this is probably bad, but not so bad that you can't go off and test things before you fix it, and certainly not bad enough to prevent the compiler from generating code".

  • To @Tino's point about using directives similar to #warning: you're right, I forgot to put this into the "Alternatives Considered" section! The main reason is that you have to know something about the set of "all checked enum cases" in order to decide whether to diagnose whether a case was missing, and that's information you can only get by looking at the whole switch. (Or by some very good flow-sensitive analysis, but that's much beyond what Swift does today.) That's why this feature is part of the switch syntax rather than just a normal statement in a case body.

2 Likes

What is your evaluation of the proposal?

-1

Is the problem being addressed significant enough to warrant a change to Swift?

Yes

Does this proposal fit well with the feel and direction of Swift?

No, it makes the language less beginner-friendly, having to understand the nuance of unknown vs default, and is not a general solution as it's only for standard libs.

An inelegant solution is worse than no solution at all. I anticipate if accepted this will become a language wart.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/a

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Followed the many proposal threads