Skip to content

Fix #12530: Wildcard of value types don't cover null #12533

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 4 commits into from
May 23, 2021

Conversation

liufengyun
Copy link
Contributor

Fix #12530: Wildcard of value types don't cover null


case Ident(nme.WILDCARD) =>
Or(Typ(erase(pat.tpe.stripAnnots), false) :: constantNullSpace :: Nil)
if pat.tpe <:< defn.AnyValType then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use isNullableClass instead? That way null won't pollute our patterns when -Yexplicit-nulls is enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, for exhaustiveness we don't report missing nulls, so maybe it's fine to do the same for unreachability?

scala> final case class Foo(x: Int)
// defined case class Foo

scala> def foo(x: Foo) = x match { case Foo(_) => }
def foo(x: Foo): Unit

scala> foo(null)
scala.MatchError: null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I guess we should emit a warning _ only matches null like we already do for case _ =>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a feature that reports a warning if the last wildcard only matches null (#4253 (comment)). I think removing the null related stuff will make the code simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I guess we should emit a warning _ only matches null like we already do for case _ =>

That would make the check more complex and expensive, while not very useful to users. I think removing null related logic will produce expected and helpful warnings to users while keeping the logic simple and fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we can avoid computing precisely whether a pattern covers exactly null. Instead, we can blindly provide a hint for null if the last case is _ and is unreachable. This allows us to use Typ(tp) instead of Or(Type(tp), nullSpace) for a wildcard pattern.

Doing that simplifies the code, and uncovers a bug that I'm now investigating:

object Test {
  sealed trait Cause[+E]

  object Cause {
    final case class Fail[E](value: E) extends Cause[E]
  }

  def fn(cause: Cause[Any]): String =
    cause match {
      case Cause.Fail(t: Throwable) => t.toString
      case Cause.Fail(any)          => any.toString
    }
}

Previously no warning is produced because the checker thought that 2nd case matches Fail(null), which is incorrect.

case (Some(false), _) =>
case (Some(true), _) =>
case (None, _) =>
case (_, false) => // reachable: null
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now issue a warning here (see the check file).

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

@smarter smarter assigned liufengyun and unassigned smarter May 23, 2021
@liufengyun liufengyun merged commit e2e77b5 into scala:master May 23, 2021
@liufengyun liufengyun deleted the fix-12530 branch May 23, 2021 18:40
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
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.

Missing warning for unreachable case when matching on a tuple
3 participants