lloydmeta / enumeratum

A type-safe, reflection-free, powerful enumeration implementation for Scala with exhaustive pattern match warnings and helpful integrations.
MIT License
1.19k stars 149 forks source link

sbt-wartremover warns that inferred type containing Nothing #18

Closed omervk closed 9 years ago

omervk commented 9 years ago

sbt-wartremover is a linter for Scala and I came across something peculiar while using it in conjunction with enumeratum:

    object Result extends Enum[Result] {
        val values = findValues

        final case class A() extends Result
        final case class B() extends Result
    }

Wart Remover fails on val values = findValues, saying "Inferred type containing Nothing".

I'm not sure why this is and would appreciate it if you could shed some light about this...

lloydmeta commented 9 years ago

Interesting, I've not really used WartRemover before, but can you try a couple things and see if they help?:

  1. Try declaring your implementations as case object instead of case class with no parameters (there is no point in making a parameterless case class, I think)
  2. Try adding a type annotation to val values, like IndexedSeq[Result]
omervk commented 9 years ago
  1. Unfortunately, I do have a couple of case classes that take parameters, so that won't work...
  2. That was the first thing I tried :+1:... Didn't work...
lloydmeta commented 9 years ago

I see, if you must use case classes, it seems like it might not be a situation where Enums (or the concept of an enumerated type) are a good fit (because it implies that you have an arbitrary (unbound) number of values of each type of thing at run time).

However, if you are able to remodel your logic into something that can use case objects, it is a sign that you may be dealing with an Enumerated type. For example (stolen from the wiki entry of Enumerated type, instead of having something like:

sealed trait Card {
  def value: Int
}
case class Heart(value: Int) extends Card
case class Spade(value: Int) extends Card
// etc

have

sealed trait Suit extends EnumEntry
case object Suit extends Enum[Suit] {
  case object Heart extends Suit
  case object Spade extends Suit
  // etc
}

case class Card (value: Int, suit: Suit) 
omervk commented 9 years ago

You're right, I'm more interested in the exhaustive-match warning that comes with enumerations in pattern matching.

So is this issue a Won't Fix? I sure hope not...

lloydmeta commented 9 years ago

I'm still trying to figure out what it is you're trying to do. If it makes sense in the context of how one would use an Enumerated type, I think it would be worth fixing (doesn't have to be by me ;) )

Out of curiosity, does WartRemover still complain if you had used case objects ? I think that the macro can't find any object implementations and so creates an empty list (thereby causing WartRemover to complain)..

From what you've described (case classes), I get the feeling that you would not be able to get exhaustive matching as an enumeration anyways, since you actually don't have an enumeration :confused:

omervk commented 9 years ago

I've tried using this code:

sealed trait Foo extends EnumEntry

object Foo extends Enum[Foo] {
    val values = findValues

    case object A extends Foo
    case object B extends Foo
}

Now WartRemover complains about the following (two of each):

  1. Inferred type containing Product
  2. Inferred type containing Serializable

Both of these mean that the Scala compiler has fallen back to these easier types instead of what is, probably, the type you were looking for.

For me, each instance of the enum can have an added piece of information. For instance, an enumeration of Success, LoginFailure and OtherFailure, might have an extra piece of information attached to the instance of the OtherFailure value (like the exception). Yes, this is not the classic case of an enumeration (and also an issue if you want to create a value of the enum from its name). As I said, I'm simply looking for a way to test that my matching is exhaustive.

In general, I wholeheartedly recommend WartRemover - it's saved me from quite a few nasty bugs in my own code.

lloydmeta commented 9 years ago

Cool, thanks for checking with case objects. In that case, does WartRemover say where it is complaining ? I think this is a case where if you had used val values: IndexedSeq[Foo] = findValues, it would be fixed. Can you please confirm?

For me, each instance of the enum can have an added piece of information. For instance, an enumeration of Success, LoginFailure and OtherFailure, might have an extra piece of information attached to the instance of the OtherFailure value (like the exception). Yes, this is not the classic case of an enumeration (and also an issue if you want to create a value of the enum from its name). As I said, I'm simply looking for a way to test that my matching is exhaustive.

I can't tell you what your definition of an enum is or should be, but I can only say that Enumeratum follows the classic definition, a type with a known set of values (at compile time), similar to Scala's Enumeration and Java's.

In your case, what you have is a sealed type hierarchy with some subtypes, but no known set of values (because your subtypes can have an arbitrary number of values). If what you need is to check for type match exhaustiveness (as opposed to value based), you can already get there with just the Scala compiler and your current sealed hierarchy:

scala> val a: Option[Int] = Some(1)
a: Option[Int] = Some(1)

scala> a match {
     | case None => 
     | }
<console>:9: warning: match may not be exhaustive.
It would fail on the following input: Some(_)
              a match {

In general, I wholeheartedly recommend WartRemover - it's saved me from quite a few nasty bugs in my own code.

I can only agree with you there :)

omervk commented 9 years ago

I think this is a case where if you had used val values: IndexedSeq[Foo] = findValues, it would be fixed.

Sorry, I forgot to try that. If I do that, one of each of the warnings is gone, but the second still remains.

I can't tell you what your definition of an enum is or should be...

That's alright, I accept that what I'm doing here is a mix-up of the term and found this out during our conversation. :smile:

If what you need is to check for type match exhaustiveness (as opposed to value based), you can already get there with just the Scala compiler and your current sealed hierarchy:

You're absolutely right! TIL :+1:

lloydmeta commented 9 years ago

Sorry, I forgot to try that. If I do that, one of each of the warnings is gone, but the second still remains.

That's a strange one :S I guess it just doesn't like the macro; I'll keep this ticket around for this issue.

In any case, glad we were able to find a solution to your problem :)

omervk commented 9 years ago

Thanks for your help and for the cool lib :)

lloydmeta commented 9 years ago

@omervk if it isn't too much trouble, can you try your original code (with the case classes) using 1.3.2-SNAPSHOT?

I couldn't get rid of the Inferred type containing Product but the case where the macro can't find any values with the provided Enum type no longer causes Inferred type containing Nothing

omervk commented 9 years ago

Wow, looks like all four warnings no longer appear! Nice! :+1:

lloydmeta commented 9 years ago

I'm closing this because it has been addressed via 1.3.2