Skip to content

Pattern matching on a local class is unsound, should emit an unchecked warning #4812

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

Closed
smarter opened this issue Jul 19, 2018 · 11 comments · Fixed by #15134
Closed

Pattern matching on a local class is unsound, should emit an unchecked warning #4812

smarter opened this issue Jul 19, 2018 · 11 comments · Fixed by #15134

Comments

@smarter
Copy link
Member

smarter commented Jul 19, 2018

Adapted from http://wouter.coekaerts.be/2018/java-type-system-broken which demonstrates the same thing in Java:

object Test {
  var prev: Any = _
  def test[T](x: T): T = {
    class A(val elem: T)
    if (prev == null) {
      val a = new A(x)
      prev = a
      x
    } else {
      prev match {
        case prev: A => // This should warn, but doesn't
          prev.elem
        case _ =>
          x
      }
    }
  }

  def main(args: Array[String]): Unit = {
    test(1)
    val x: String = test("") // ClassCastException: java.lang.Integer cannot be cast to java.lang.String
  }
}

There's no way to distinguish instances of a local class coming from different method calls, so the type test is never safe and should always require an @unchecked annotation.

@Blaisorblade
Copy link
Contributor

Alternatively, we could allow the match, but weaken the type of prev.elem (or really, the captured T) to an unknown skolem. Usually that’s expressed by matching against Test#A instead of A, but there isn’t a way to write the correct prefix.

Your solution is probably reasonable as well tho, and I don’t expect many to care about this use case.

@odersky
Copy link
Contributor

odersky commented Apr 5, 2022

@dwijnand Do you want to take a look at this one?

@dwijnand
Copy link
Member

dwijnand commented Apr 6, 2022

Yep, I'll see what I can do.

@dwijnand
Copy link
Member

dwijnand commented May 6, 2022

Getting that to fail is easy:

@@ -152,6 +152,8 @@ object TypeTestsCasts {
       case AnnotatedType(t, _)  => recur(X, t)
       case tp2: RefinedType     => recur(X, tp2.parent) && TypeComparer.hasMatchingMember(tp2.refinedName, X, tp2)
       case tp2: RecType         => recur(X, tp2.parent)
+      case tp2
+      if tp2.typeSymbol.isLocal => false
       case _                    => true
     })

but then that makes things like this fail:

def foo: Unit =
  object O:
    sealed abstract class A
  class B extends O.A
  class C extends O.A

  val x: O.A = ???
  x match
    case x: B => ???
    case x: C => ???

Is there a way to tell if a class (and parents I guess too) is defined in terms of local scope (which can also nest) type parameters (which I guess could also be class type parameters...)? Seems like a whole lot of things, but maybe there's mechanics for this already somehow?

@SethTisue
Copy link
Member

(It would be nice if the error message gave some clue as to the specific nature of the problem. I wouldn't have guessed this — neither @WojciechMazur nor I guessed it at today at #16259.)

@dwijnand
Copy link
Member

It's better now with #16086

@OlegYch
Copy link
Contributor

OlegYch commented Jul 4, 2023

this produces warnings for perfectly safe code, eg:

def x = {
  case class D(i: Int)
  val d: Any = D(1)
  d match {
    case d: D => d.i
  }
}

@dwijnand
Copy link
Member

dwijnand commented Jul 4, 2023

How is the compiler to know that that d: Any is safe to compare against a type reference to local class D?

@OlegYch
Copy link
Contributor

OlegYch commented Jul 4, 2023

@dwijnand i guess because D does not have type parameters (unlike in the original example)?

@dwijnand
Copy link
Member

dwijnand commented Jul 4, 2023

Even without a type parameter it's not possible to tell if D was created by a previous method call. As Guillaume mentions in #15134 (comment):

Anything else could end up giving you an instance of a local class coming from a different call to the same method which semantically is a different class.

Captured as def test10 in tests/neg/i4812.scala.

@OlegYch
Copy link
Contributor

OlegYch commented Jul 4, 2023

@dwijnand hm that sounds unfortunate, thanks for the explanation
damn mutability

@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants