use SELECTin by default in TASTy

[bishabosha]

The goal here is to stabilise the overload resolution for a method -from-tasty when a dependency either adds an overload; removes an override; moves a method to a superclass.

Unknowns:
How to handle refined methods, they at least need a special case in unpickling, but does that require a different tag?

[odersky]

I think pickling both signatures is not acceptable from a compression standpoint.

[odersky]

To come back to why we sometimes need SELECTin: It's when a reference goes to multiple overloaded definitions that have the same signature at the call site. This is should be a very uncommon case. Did you count how often this occurs in, say, our own codebase or the community-build?

[smarter]

No matter how uncommon it is, it still makes tasty-compatibiltiy extremely hard to preserve, since overloads (which were always binary-compatible so far) will now break compatibility in hard-to-predict ways (you'd need to solve some sort of constraint problem to check if there exists a prefix under which two overloads have the same signature). We should aim to preserve the overload/override binary compatibility guarantees of the JVM: https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html, otherwise tasty will be more a burden than an asset to library authors.

[odersky]

No matter how uncommon it is, it still makes tasty-compatibiltiy extremely hard to preserve, since overloads (which were always binary-compatible so far) will now break compatibility in hard-to-predict ways (you'd need to solve some sort of constraint problem to check if there exists a prefix under which two overloads have the same signature). We should aim to preserve the overload/override binary compatibility guarantees of the JVM: https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html, otherwise tasty will be more a burden than an asset to library authors.

Yes, but if this is a once in a blue moon event it might be better not compromise the Tasty format for it (and so far it looks like any patch would be a serious violation of conceptual integrity). An alternative would be to simply warn loudly on pickling a SelectIN.

[sjrd]

If TASTy is supposed to be the format that we use as reference for compatibility, then making it smaller and not handle some overloading cases is the compromise. We should avoid this at all cost. Any size improvement cannot ever compensate for a correctness issue.

[smarter]

+1 to what @sjrd said, but also I don't see any reason we shouldn't be able to make it work with a single signature, if that doesn't work it means there's an underlying bug somewhere.

[odersky]

I just instrumented and counted. I saw no generation of SelectIN in either the community build or the dotty build. We have a single test that generates one occurrence. So we should ask ourselves what is the right way to deal with it. Making Tasty more bulky and complicated is maybe not the right answer. One could also envisage going the route of forbidding references like this across compilation units.

[odersky]

More explanations: A tasty external reference is the name and the signature at the call site. It cannot in general be the signature of a symbol, since the symbol might not be unique. I believe any change of this conceptual model would cause considerable breakage. So we have to work in this framework to solve the issue. It could well be that we decide in the end that we want to restrict something.

[smarter]

One could also envisage going the route of forbidding references like this across compilation units.

We first saw this issue with ArrayBuffer#empty in the standard library: #9050, this isn't something we can reasonably drop support for in my opinion (and this would also be a headache for library authors since it would mean they'd need to design APIs that are not subject to this problem)

[odersky]

We first saw this issue with ArrayBuffer#empty in the standard library: #9050, this isn't something we can reasonably drop support for in my opinion (and this would also be a headache for library authors since it would mean they'd need to design APIs that are not subject to this problem)

Yes, that's the single test that broke. My guess is we have approximately two or three orders of magnitude more issues with erasure.

The current situation in the issue is handled by SelectIN. But if someone adds another method to some other class in the future that causes the same problem we would get an ambiguity.

The problem, is that, conceptually with intersection types, we do not select a symbol, we select based on the name and the type and the type is represented by a signature. So we cannot pretend that we select symbols in Tasty. It would probably cause many more problems than it fixes.

[bishabosha]

would selecting members using a MethodType along a name be too expensive?

[odersky]

would selecting members using a MethodType along a name be too expensive?

Certainly.

Also, here's my comment from #9050:

I'd argue it's the wrong design. Buffer.append should have these signatures:

def append(elem: A): this.type
def append(elem: A, elems: A*): this.type

I propose to not sacrifice the conceptual integrity of Tasty to support ill-designed legacy cases. Maybe we could direct our energy instead into how we can detect and reject these cases.

[bishabosha]

could signature include if the parameter is vararg? (to avoid ambiguity in the case where T in T* is Seq)
Edit: handling Vararg in signature is not the problem, see #11210 (comment)

[bishabosha]

latest commit goes back to a single signature with some special cases, e.g. in the case of a refined type it seems that asSeenFrom forgets method argument names, causing problems for named arguments, e.g. in https://github.com/lampepfl/dotty/blob/master/tests/pos/i8516.scala

EDIT: 9020fbc breaks the following previously untested code in tasty printing diff test (the CI does not test this path):

trait Boxy[T] {
  def addOne(t: T): this.type = this
}

trait BoxInt {
  def addOne(t: Int): this.type = this
}

@main def Test = {
  val qux: BoxInt & Boxy[Int] = new BoxInt with Boxy[Int] {
    override def addOne(t: Int): this.type = super[Boxy].addOne(t) // type of super selection changes in before/after
  }
}

[smarter]

The problem, is that, conceptually with intersection types, we do not select a symbol

After looking at the code I think that's not quite correct: when selecting a member of an intersection where both sides have candidates, we use Denotation#meet which will either return a JointRefDenotation whose symbol is set to one of the two candidates, or we return a MultiDenotation, later when we do overload resolution we will pick one of the overloaded denotation or fail typechecking, so pickling doesn't have to worry about issues with intersections.

In fact, the proof is in the pudding, we currently use SELECTin with intersection types and this appears to work fine:

trait Foo[A] {
  def append(elem: A): Unit = {}
}
trait Bar[A] {
  def append(elems: A*): Unit = {}
}
object Test {
  def bla(): Foo[Seq[String]] & Bar[Seq[String]] = ???

  def test: Unit = {
    val seq = Seq("")
    bla().append(seq)
  }
}

   151:             SELECTin(10) 36 [append[Signed Signature(List(java.lang.Object),scala.Unit) @append]]
   154:               APPLY(5)
   156:                 TERMREFsymbol 78 // bla
   158:                   THIS
   159:                     SHAREDtype 10
   161:               SHAREDtype 94 // Foo

Using -Ythrough-tasty -Ycheck:all confirms that this gets unpickled correctly).

If I'm interpreting https://github.com/lampepfl/dotty/blob/ce684de6ce4ce77c17750c675343d65a5ef24137/compiler/src/dotty/tools/dotc/core/Types.scala#L759-L769 correctly, the only situation where we're selecting a method and end up with a symbol-less denotation is when we're dealing with a structural selection, but those get replaced by regular method calls before pickling, so I think we're safe, unless there's some other case I'm missing.

[bishabosha]

I have another proposal, - drop SELECTin and add a vararg flag to SignedName - otherwise the special cases in this PR look pretty dodgy so far

[smarter]

I have another proposal, - drop SELECTin and add a vararg flag to SignedName

varargs are but one way to end up with identical signatures but different types, if we were to go in that direction we would have to basically put all types in signatures, so I don't think that's workable.

[bishabosha]

varargs are but one way to end up with identical signatures but different types

indeed, all you have to do is explicitly use Seq and not varargs

class Col[T] {
  def addOne(t: Seq[T]): this.type = this
  def addOne(t: T): this.type = this
}

object Foo {
  val foo = Col[Seq[Int]]()
  val bar = Seq.empty[Int]
  foo.addOne(bar) // uses SELECTin with 3.0.0-M3
}

[bishabosha]

It was said yesterday that this could be done post 3.0, but dropping a TASTy tag is not backwards compatible

[smarter]

Would you mind squashing this PR into a smaller number of commits (the thisType stuff could be its own commit but everything else seems like it could be just one commit) ? Also it'd be nice to document why we can't use SELECTin in the various cases where we don't since it's not always obvious.

[smarter]

@nicolasstucki Why does length not just call content().length ?

[smarter]

LGTM, awesome work!

[bishabosha]

@smarter I just added an increment to TastyFormat ExperimentalVersion