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

Make Violation trait sealed for compiler exhaustiveness checks #45

Closed
suin opened this issue Jul 19, 2015 · 2 comments
Closed

Make Violation trait sealed for compiler exhaustiveness checks #45

suin opened this issue Jul 19, 2015 · 2 comments
Assignees
Milestone

Comments

@suin
Copy link

suin commented Jul 19, 2015

Could you add sealed keyword to Violation trait for the sake of type-safety of client code?
Compiler warns that match-case statement is not safe on code like:

@tailrec
def flattenViolations(treeViolations: List[Violation], flatViolations: List[Violation] = List()): List[Violation] = {
  treeViolations match {
    case Nil => flatViolations
    case (violation: RuleViolation) :: violations =>
      flattenViolations(violations, flatViolations :+ violation)
    case (violation: GroupViolation) :: violations =>
      flattenViolations(violation.children.toList ++ violations, flatViolations)
  }
}
[error] /...[snip].../AccordExtensions.scala:19: match may not be exhaustive.
[error] It would fail on the following input: List((x: com.wix.accord.Violation forSome x not in (com.wix.accord.GroupViolation, com.wix.accord.RuleViolation)))
[error]     treeViolations match {
[error]     ^

So client code has to have default case, even if it is unnecessary like:

  treeViolations match {
    case Nil => flatViolations
    case (violation: RuleViolation) :: violations =>
      flattenViolations(violations, flatViolations :+ violation)
    case (violation: GroupViolation) :: violations =>
      flattenViolations(violation.children.toList ++ violations, flatViolations)
    case _ => throw new RuntimeException("Invalid state") // Unnecessary case if `Violation` class were sealed.
  }
@holograph
Copy link
Contributor

Not a bad idea, though it's worth noting that one of the reasons it's not yet the case is because the result model is still WIP (obvious case that isn't covered is #21, and there may be ramifications following #36). That being said, changes are going to breaking anyway, so your suggestion still seems appropriate.

@holograph holograph added this to the 0.5 milestone Jul 19, 2015
@holograph holograph self-assigned this Jul 19, 2015
holograph added a commit that referenced this issue Jul 27, 2015
@holograph holograph changed the title sealed trait Violation Make Violation trait sealed for compiler exhaustiveness checks Jul 27, 2015
@holograph
Copy link
Contributor

I'm a little concerned that this may break client code extending Violation, but since I can't imagine anyone actually doing that I'm going ahead with this. If anyone has feedback to the contrary, I'd love to discuss your use-case.

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

No branches or pull requests

2 participants