Local classes are uncheckable (type tests)

[dwijnand]

No description provided.

[dwijnand]

I didn't look into the detail, but it looks like doing it so bluntly breaks:

 - dotty.communitybuild.CommunityBuildTestB.http4s
 - dotty.communitybuild.CommunityBuildTestB.scodec
 - dotty.communitybuild.CommunityBuildTestB.fs2

[dwijnand]

@smarter maybe you're interested to review this, seeing as it's an old one you created.

[smarter]

No time to review this unfortunately, but traversing all types of all declarations of all base classes could be pretty expensive.

[dwijnand]

It's all types of all declarations of all base classes defined in the method, which is hopefully ok. And I'm all ears for whether there's a better way to do it.

[odersky]

I also think we should keep searching for a more efficient way to fix this. But I don't have the time right now to do it. So it's probably best to revisit this later.

[smarter]

Maybe we should consider all type tests to local classes as unchecked by default, the one case which is safe is when we go from one local class to another local class defined in the same scope:

def foo = {
  sealed trait A
  class B extends A
  val x: A = new B
  x match { case x: B => x }
}

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.

[smarter]

In other words the check would be something like "(x: A).isInstanceOf[B] emits a warning if B is a local class which is not statically reachable from the scope where A is defined"

[dwijnand]

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.

That seems overly strict. A local class that doesn't depend on anything in the method surely isn't "unchecked" when used, such as:

class A
def foo = {
  class B extends A
  val x: A = new B
  x match {
    case x: B =>
    case _ =>
  }
}

But I did realise from your comment that I'm not considering singleton types based on any terms in the method, like any parameters.

Going back to performance, the check here sounds big, but it's "only" on the types of the declarations in the local classes. So if you have a local class with 30 methods, or 10 local classes with 3 methods, it will take the time that takes. But I think it's fairly limited, as local classes tend to be few and small, in my experience. In the case of fs2, https://github.com/dotty-staging/fs2/blob/main/core/shared/src/main/scala/fs2/Pull.scala#L824-L1260 I think I counted 8 classes with about 5 methods each, which seems excessive to me. I'm also not caching this information anywhere, admittingly, so I could look into that if desired.

[smarter]

Your example matches the exception to the unchecked warning I gave above.

[dwijnand]

Oh. Then how are you defining "statically reachable"? Because I'd assumed being defined in a def disqualified it.

[smarter]

I meant reachable through only static references from a specific scope (which might be the scope of a def)

[smarter]

Oh but actually I misread your example, i didn't see that A was defined outside the def and B inside. Then I assert that we should get an unchecked warning: in general you cannot know if the B you get was created in the current call to the def or in a different call and escaped, and in the latter case it's really a different class B that happens to have the same runtime representation

[dwijnand]

Right. Do we have some verbiage to back up that claim? Because I think many users consider the fact that classes can be defined locally to be a proximity convenience rather than also implying method-call uniqueness in those class types. I find it particularly hard to defend that claim because it's impossible to build a true neg test for that match, using my class definitions.

[smarter]

It makes sense by analogy with inner classes (which store an outer pointer to allow for checked type tests)

[odersky]

Note: classSymbols is not the same as baseClasses: classSymbols of Foo | Bar is {Foo, Bar}.

[smarter]

classSymbols of Foo | Bar is {Foo, Bar}.

Is it? classSymbols calls parentSymbols which does https://github.com/lampepfl/dotty/blob/0260d758d89d01ab445c471aa159ca8367d596ba/compiler/src/dotty/tools/dotc/core/Types.scala#L539-L540
(not sure what the spec in question is)

[odersky]

Ah sorry, I meant Bar & Foo. For Bar | Foo, classSymbols is empty. But in any case it has nothing to do with baseClasses.

[dwijnand]

Right, sorry, parentSymbols, not baseClasses.

[odersky]

Otherwise LGTM

[odersky]

Why is this additional test needed?

[dwijnand]

This is for whether the scrutinee was annotated @unchecked:

  def test8[T](x: T): T =
    class A(val elem: T)
    val p = prev
    (p: @unchecked) match
      case prev: A => prev.elem
      case _       => prev = new A(x); x

[dwijnand]

Let me know if you're happy with my answer (assuming, as I do, that CI passes).

[odersky]

Yes, looks all good to me now,