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

Incorrect "the type test for A cannot be checked at runtime because it's a local class" warning #20741

Open
uosis opened this issue Jun 22, 2024 · 7 comments
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement

Comments

@uosis
Copy link

uosis commented Jun 22, 2024

Compiler version

Tested 3.4.2, 3.4.1, 3.4.0, 3.3.3, all with the same issue.

2.13.14 works fine and does not emit warning.

Minimized example

def f: Unit = {
  class A {
    override def equals(x: Any): Boolean = x match {
      case a: A => true
      case _ => false
    }
  }
}

Output Error/Warning message

1 warning found
def f: Unit
-- [E092] Pattern Match Unchecked Warning: -------------------------------------
4 |      case a: A => true
  |           ^
  |the type test for A cannot be checked at runtime because it's a local class
  |
  | longer explanation available when compiling with `-explain`

Why this Error/Warning was not helpful

The code seems correct and runs correctly, i.e. the type test executes correctly at runtime, contradictory to the warning.

Suggested improvement

No warning should be emitted.

@uosis uosis added area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 22, 2024
@som-snytt
Copy link
Contributor

som-snytt commented Jun 22, 2024

The following with member A prints false twice

class B:
  class A {
    override def equals(x: Any): Boolean = x match {
      case a: A => true
      case _ => false
    }
  }
  def f(x: Any): Any = {
    val y = A()
    println(y == x)
    y
  }

@main def test() =
  val b0 = B()
  val f = b0.f(null)
  val b1 = B()
  b1.f(f)

but with local A, it is false (of course) then true. It's merely testing the implementation class.

Not sure if more words from the code comment would be elucidating:

8. if `P` is a local class which is not statically reachable from the scope where `X` is defined, "it's a local class"

@Gedochao Gedochao removed the stat:needs triage Every issue needs to have an "area" and "itype" label label Jun 25, 2024
@clavigne
Copy link
Contributor

Can I try my hand at this one? Sounds interesting

@symingz
Copy link

symingz commented Oct 8, 2024

Something related: there are no warnings when a local case class is defined. But a local case class C's auto-generated equals method contains pattern match against C, or something equivalent. So if the warning is correct behavior, it should be emitted for local case classes as well.

@symingz
Copy link

symingz commented Oct 8, 2024

I think the warning is in fact correct. Consider the following:

def f: Any = {
  class A {
    override def equals(x: Any): Boolean = x match {
      case _: A => true
      case _    => false
    }
  }
  A()
}

@main def foo(): Unit =
  val x0 = f
  val x1 = f
  println(x0 == x1)

The program output is true. But that is wrong, because each invocation of f creates a new class A (this is particularly obvious if A references f's local variables). So the match of x1 against x0's type (that is, the A from the first f invocation) should fail. But it succeeded, thus the warning is warranted.

What needs fixing is that the same warning should be emitted for case classes as well. Consider the following:

trait C:
  def ff: Int

def f(x: Int): C =
  case class A() extends C:
    def ff = x
  A()

@main def foo(): Unit =
  val x0 = f(0)
  val x1 = f(1)
  println(x0 == x1)

The output is true, but it should be false, since x0.ff != x1.ff. So definitely a warning is needed here.

@som-snytt
Copy link
Contributor

som-snytt commented Oct 8, 2024

My previous comment was that the warning is warranted but the words could be improved.

Good catch on case class, which is unchecked:

          override def equals(x$0: Any): Boolean =
            this.eq(x$0.$asInstanceOf[Object]).||(
              x$0 match
                {
                  case x$0 @ _:A @unchecked => this.x.==(x$0.x).&&(x$0.canEqual(this))
                  case _ => false
                }

unchecked added in #4045 in transform/SyntheticMethods.scala

I wanted to suggest

case _: C[E @unchecked] =>

instead of

case _: C[E] @unchecked =>

but the local class warning is not emitted for polymorphic case:

import scala.util.chaining._

object Test {
  def f(x: Any): Any = {
    class C[A](i: Int) {
      override def equals(other: Any): Boolean = {
        other match {
          //case _: C[A @unchecked] => true
          case _: C[_] => true
          //case _: C[A] => true
          case _ => false
        }
      }
    }
    class D(i: Int) {
      override def equals(other: Any): Boolean = {
        other match {
          case _: D => true
          case _ => false
        }
      }
    }
    new C[Int](42).tap(y => println(y == x))
    new D(42).tap(y => println(y == x))
  }
}

@uosis
Copy link
Author

uosis commented Oct 10, 2024

I think the warning is in fact correct. Consider the following:

def f: Any = {
  class A {
    override def equals(x: Any): Boolean = x match {
      case _: A => true
      case _    => false
    }
  }
  A()
}

@main def foo(): Unit =
  val x0 = f
  val x1 = f
  println(x0 == x1)

The program output is true. But that is wrong, because each invocation of f creates a new class A (this is particularly obvious if A references f's local variables). So the match of x1 against x0's type (that is, the A from the first f invocation) should fail. But it succeeded, thus the warning is warranted.

Interesting. Should it not be an error then, not just warning? To me, this behavior seems very counter-intuitive.

@symingz
Copy link

symingz commented Oct 10, 2024

The use of warning is more or less consistent with those cases when erased types appear in a match. Although local classes are more subtle, they are, in a sense, "half erased".

A more ideal fix would be to implement the match of local classes correctly. This is done for inner classes (as shown by @som-snytt's example). My guess is that in the inner class case, the match not only check the runtime class, but also the captured outer object. For local classes, captured lexical closures (could be empty) can be checked to achieve the correct behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc better-errors Issues concerned with improving confusing/unhelpful diagnostic messages itype:enhancement
Projects
None yet
Development

No branches or pull requests

5 participants