-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #2502: improve warning message of exhaustivity check #2504
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
Conversation
val explanation = | ||
hl"""|There are several ways to make the match exhaustive: | ||
| - add missing cases as shown in the warning | ||
| - change the return type of irrefutable extractors as `Some[T]` if exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say: "- If an extractor always return Some(...)
, write Some[X]
for its return type"
hl"""|There are several ways to make the match exhaustive: | ||
| - add missing cases as shown in the warning | ||
| - change the return type of irrefutable extractors as `Some[T]` if exist | ||
| - add a wildcard `_` case at the end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say: "- Add a case _ => ...
at the end to match all remaining cases"
@@ -657,7 +657,7 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic { | |||
else if (tp.classSymbol.is(CaseClass)) | |||
// use constructor syntax for case class | |||
showType(tp) + signature(tp).map(_ => "_").mkString("(", ", ", ")") | |||
else if (signature(tp).nonEmpty) | |||
else if (sym.is(Flags.Case) && signature(tp).nonEmpty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this condition a superset of the condition of the previous if (if (tp.classSymbol.is(CaseClass))
) and so this branch is just dead code?
@@ -657,7 +657,7 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic { | |||
else if (tp.classSymbol.is(CaseClass)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if a class is a case class, it might contain a custom unapply, for example:
abstract class BTypes {
trait BType
sealed trait RefBType extends BType {
def classOrArrayType: String = this match {
case ClassBType(internalName) => internalName
case a: ArrayBType => ""
}
}
final case class ClassBType(val internalName: String) extends RefBType
class ArrayBType extends RefBType
object ClassBType {
def unapply(x: RefBType): Option[String] = None
}
}
Even with this patch, it warns It would fail on: BTypes.ClassBType(_)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it's more reliable to show just _: T
for type space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM, nice improvement!
// does the companion object of the given symbol have custom unapply | ||
def hasCustomUnapply(sym: Symbol): Boolean = { | ||
val companion = sym.companionModule | ||
companion.findMember(nme.unapply, NoPrefix, Flags.Synthetic).exists || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use excluded = Synthetic
to avoid confusion.
Fix #2502: improve warning message of exhaustivity check