Skip to content

[QoI] fix diagnosis of non-Optional enum used in optional pattern #4445

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

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

jtbandes
Copy link
Contributor

enum Foo { case Bar }
let maybeFoo = Foo.bar

Previously:

error: enum case 'Some' not found in type 'Foo'
guard let foo = maybeFoo else { fatalError() }
          ^

Now:

error: initializer for conditional binding must have Optional type, not 'Foo'
guard let foo = maybeFoo else { fatalError() }
          ^

Resolves SR-1456.

@jtbandes
Copy link
Contributor Author

jtbandes commented Aug 21, 2016

Hmm, there is a strange test case in SILGen/statements.swift where guard let t = a is expected to work when a is a custom optional type:

enum MyOptional<Wrapped> {
  case none
  case some(Wrapped)
}

Is this something we should still support? cc @lattner who originally wrote it

@DougGregor
Copy link
Member

That's weird; we don't want to support anything other than the true optional types in if-let/guard-let.

@slavapestov
Copy link
Contributor

Renaming MyOptional to Optional might do the trick.

@jtbandes
Copy link
Contributor Author

Would removing this be a language change that would need to go through swift-evolution? It seems like a bugfix, but conceivably some people could be using it.

@jtbandes
Copy link
Contributor Author

jtbandes commented Aug 22, 2016

I pre-emptively updated the commit to simply remove that test. 🙂
@swift-ci please smoke test

On a related note, how would y'all feel about coercePatternToType() gaining a new argument, a SourceLoc for the initializer expression (if there is one)? I'd love to have this diagnostic point to the initializer rather than the bound variable. I can do that in a separate PR.

@jtbandes
Copy link
Contributor Author

@swift-ci please test

@DougGregor
Copy link
Member

I don't consider this a language change that needs swift-evolution. And, passing a SourceLoc for the initializer seems totally reasonable here. Thanks!

@jtbandes
Copy link
Contributor Author

@DougGregor OK, thanks! If that qualifies as "LGTM", feel free to merge — GitHub won't let me do it for some reason.

What's the procedure for nominating changes for Swift 3.0? Do I just make another PR against swift-3.0-branch?

@DougGregor DougGregor merged commit d74b97c into swiftlang:master Aug 23, 2016
@DougGregor
Copy link
Member

Merged. You can make a PR against the swift-3.0-branch, given it a 3.0 milestone, and assign to @tkremenek .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants