Skip to content
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

Union unapply method does not disable match exhaustiveness checking #46

Merged

Conversation

marcgrr
Copy link
Contributor

@marcgrr marcgrr commented Feb 1, 2017

The comment in the test describes what is going on.

Concretely, before this change, code like this would not cause scalac to output a non-exhaustive match warning, even though it's a non-exhaustive match:

def foo(a: WithComplexTypesUnion.Union) {
  println(a match {
    case WithComplexTypesUnion.Union.EmptyMember(x) => x
  })
}

After this change, that code will output a non-exhaustive match warning. (This applies to scala 2.11. It is possible that 2.12 does not suffer from the same problem, or will eventually not suffer from it.)

Unfortunately, this change introduces a small backwards-incompatibility for anyone compiling their scala code with -Xfatal-warnings: Code that used to compile won't necessarily still compile. I'm happy to fix up the Coursera code before incorporating this change, but are there any other consumers of this library that we have to think about?

@amory-coursera
Copy link
Contributor

@marcgrr - Thanks for this change. I'm not aware of any non-Coursera users who use Courier with Scala (speak up if you're out there!). I think this change is worthy of a minor version bump (required to publish in any case...) but think it's an improvement worth making in spite of (possibly) breaking some existing code.

Can you run scripts/bumpVersion? Happy to merge in thereafter.

@eboto
Copy link
Contributor

eboto commented Mar 22, 2017

@amory-coursera us at Instrumental are non-Coursera Scala users of Courier. We have been wanting this feature for a while so this is fantastic. We have been preserving exhaustiveness checks by defaulting to pattern match against the type rather than against the unapplication. It will be great to stop needing that workaround.

Great addition @marcgrr !

@amory-coursera
Copy link
Contributor

Thanks for letting us know, @eboto . From your commits, it seems like you folks at instrumental are polyglot. Super cool.

@marcgrr Please do bump the version and then merge.

@marcgrr
Copy link
Contributor Author

marcgrr commented Mar 31, 2017

Sorry for taking so long! I've bumped the version and I'll merge soon after the ci build finishes.

In the meantime, I've discovered a different similar issue: The record unapply method should probably also return Some. But there are some cases that I don't quite understand where it returns None, so I can't simply change the return type. I'll file an issue so that it doesn't get forgotten.

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