Align specificity of `(a: T)...` vs `(a: T*)...` with mono case #7680

adriaanm · 2019-01-24T12:08:58Z

Fix scala/bug#11015
Fix scala/bug#4775
Fix scala/bug#8344

TODO:

test failures :-/
dig a little deeper into the explanation of why Seq[T] would be seen as compatible to T
snippet of interest

        // closely mimic monomorphic case of typesCompatible -- check arguments (keeping repeated param types)
        // this ensures that we deal uniformly with repeated params in the mono and poly cases
        val okArgs = isCompatibleArgs(args.map(_.instantiateTypeParams(undetparams, inferredArgs)),
                                      formalTypes(formals.map(_.instantiateTypeParams(undetparams, inferredArgs)), args.length, removeByName = false))

        okArgs && {
          val AdjustedTypeArgs.Undets(okparams, okargs, leftUndet) = adjustTypeArgs(undetparams, tvars, inferredArgs, pt)
          enhanceBounds(okparams, okargs, leftUndet)

The following type checked (choosing the single-param overload):

def foo(xs: Int*): Int = xs.toSeq.head
def foo(x: Int): Int = x
foo(2)

Whereas this was considered ambiguous:

def fooT[T](xs: T*): T = xs.toSeq.head
def fooT[T](x: T): T = x
fooT(2)

Another instance of the mono and poly cases of isApplicableToMethod
being out of whack.

isApplicableToMethod will use formalTypes to eliminate
the repeated param in the formal types at the start,
but we keep the repeated marker in the arguments -- here's a debug log:

/*
inferMethodAlternative all: List(method foo, method foo)
isCompatibleArgs false (List(Int*), List(Int))
isAsSpecific false: (xs: Int*)Int >> (x: Int)Int?
 --> the repeated case is not more specific than the single-arg case because
     you can't apply something of `Int*` to `Int`

isCompatibleArgs true (List(Int), List(Int))
isAsSpecific true: (x: Int)Int >> (xs: Int*)Int?
  --> the single param case is more specific than the repeated param case, because
      you can apply a single argument to the method with the repeated param

isCompatibleArgs true (List(Int), List(Int))
isAsSpecific true: (x: Int)Int >> (xs: Int*)Int?
isCompatibleArgs false (List(Int*), List(Int))
isAsSpecific false: (xs: Int*)Int >> (x: Int)Int?
isCompatibleArgs true (List(Int), List(Int))
isAsSpecific true: (x: Int)Int >> (xs: Int*)Int?
isCompatibleArgs false (List(Int*), List(Int))
isAsSpecific false: (xs: Int*)Int >> (x: Int)Int?
inferMethodAlternative applicable List(method foo, method foo) --> ranked: List(method foo)
 */

The bug in the polymorphic case of isApplicableToMethod
was that adjustTypeArgs would replace the T* by Seq[T],
thus removing the incompatibility in applying a Seq[T] when
a T is expected.

adriaanm · 2019-01-24T12:25:07Z

Or maybe we shouldn’t be instantiating type vars to a non-expression type like a repeated param type?

adriaanm · 2019-01-24T15:36:35Z

Welcome to Scala 2.13.0-8adcadb (Java HotSpot(TM) 64-Bit Server VM, Java 11).
Type in expressions for evaluation. Or try :help.

scala> java.util.List.of("")
res0: java.util.List[String] = []

adriaanm · 2019-01-24T16:08:56Z

Humm, lots of test failures. I'm deferring this to 2.13.1 -- there's no need to get this into 2.13.0

adriaanm · 2019-01-24T16:50:35Z

I forgot this is affecting specs, moving back to RC1.

som-snytt · 2019-01-24T22:38:24Z

Abstractly, this is because of auto-tupling?

scala/bug#4775
scala/bug#5414

I remember Lukas mentions the implementation on one of the old tickets, maybe not these.

(Object) vs (String*):

scala/bug#8344

adriaanm · 2019-01-25T13:44:23Z

Thanks! Auto-tupling was also hiding the second class T*, pretending like we can make values of that. Added a few bugs to the fixlist

adriaanm · 2019-01-25T14:57:18Z

🐰🕳

so, T* aka TypeRef(_, RepeatedParamClass, T) has Seq[T] as a base class, which defines toSeq: this.type, which means xs.toSeq: T* when xs: T*, but now this PR closes the hole that pretended T* was a first-class type in method applications'

i see no other way out but to adapt an expr: T* to expr: Seq[T], unless the expected type is T'*

som-snytt · 2019-01-25T16:07:43Z

To coin a phrase, when I was learning scala the handling of T* was obscure to me on a conceptual level. TIL bunnies live in the man hole cover that leads to the sewer. [Edited.]

scala/bug#4728
scala/bug#4728 (comment)

adriaanm · 2019-01-25T16:37:14Z

🤯scala/bug#3866

adriaanm · 2019-01-25T16:39:31Z

Urban dictionary doesn't help me this time with "to coin a phrase". ~~Mint~~ Might I ask you to elaborate?

... through drastic means: T* now only conforms (isCompatible) to other repeated parameter types. This breaks case classes with repeated param fields unless we add a case to adapt.... The following type checked (choosing the single-param overload): ``` def foo(xs: Int*): Int = xs.toSeq.head def foo(x: Int): Int = x foo(2) ``` Whereas this was considered ambiguous: ``` def fooT[T](xs: T*): T = xs.toSeq.head def fooT[T](x: T): T = x fooT(2) ``` Another instance of the mono and poly cases of isApplicableToMethod being out of whack. `isApplicableToMethod` will use formalTypes to eliminate the repeated param in the formal types at the start, but we keep the repeated marker in the arguments -- here's a debug log: /* inferMethodAlternative all: List(method foo, method foo) isCompatibleArgs false (List(Int*), List(Int)) isAsSpecific false: (xs: Int*)Int >> (x: Int)Int? --> the repeated case is not more specific than the single-arg case because you can't apply something of `Int*` to `Int` isCompatibleArgs true (List(Int), List(Int)) isAsSpecific true: (x: Int)Int >> (xs: Int*)Int? --> the single param case is more specific than the repeated param case, because you can apply a single argument to the method with the repeated param isCompatibleArgs true (List(Int), List(Int)) isAsSpecific true: (x: Int)Int >> (xs: Int*)Int? isCompatibleArgs false (List(Int*), List(Int)) isAsSpecific false: (xs: Int*)Int >> (x: Int)Int? isCompatibleArgs true (List(Int), List(Int)) isAsSpecific true: (x: Int)Int >> (xs: Int*)Int? isCompatibleArgs false (List(Int*), List(Int)) isAsSpecific false: (xs: Int*)Int >> (x: Int)Int? inferMethodAlternative applicable List(method foo, method foo) --> ranked: List(method foo) */ The bug in the polymorphic case of `isApplicableToMethod` was that `adjustTypeArgs` would replace the `T*` by `Seq[T]`, thus removing the incompatibility in applying a `T*` when a `T` is expected. Sadly, the bounds check also didn't catch it, since `T* <:< Any`... There was a similar issue in that we would tuple a T* with other args while trying to deal with arity mismatch. Also exclude T* there.

adriaanm · 2019-01-28T12:39:58Z

🎉

som-snytt · 2019-01-28T19:45:46Z

Now I'll have to go back and change all my stack overflow answers.

If you're a bad mono case you should stay home and recover.

adriaanm · 2019-01-28T21:24:08Z

This one definitely had me stereo-the-window.

retronym · 2019-01-29T00:24:42Z

spec/06-expressions.md

 - A parameterized method $m$ of type `($p_1:T_1, \ldots , p_n:T_n$)$U$` is
  _as specific as_ some other member $m'$ of type $S$ if $m'$ is [applicable](#function-applications)
-  to arguments `($p_1 , \ldots , p_n$)` of types $T_1 , \ldots , T_n$.
+  to arguments `($p_1 , \ldots , p_n$)` of types $T_1 , \ldots , Tlast$;


Should this be Tn rather than Tlast ?

Tlast is the type of the last arg in the m-prime application; it may be taken as T (instead of T*) or just Tn.

som-snytt · 2019-01-29T01:43:28Z

src/compiler/scala/tools/nsc/typechecker/Infer.scala

+    def isCompatibleArgs(tps: List[Type], pts: List[Type]) = {
+      val res = (tps corresponds pts)(isCompatible)
+//      println(s"isCompatibleArgs $res : $tps <:< $pts")
+      res


I think you need an @inline printing[A](msg, a: => A): A thingie with elidable printing.

How I learned to stop worrying about performance and have debug output when I need it. Comments! Higher-tech would be nice, but it would also look like regular code, when it isn't really.

retronym · 2019-01-29T04:18:52Z

src/compiler/scala/tools/nsc/typechecker/Infer.scala


-      val tp1 = methodToExpressionTp(tp)
+      // can only compare if both types are repeated or neither is (T* is not actually a first-class type, even though it has a BTS and thus participates in subtyping)
+      (!isRepeatedParamType(tp) || isRepeatedParamType(pt)) && {


We'll proceed through this if we have a Java and Scala repeated param type. Array[T] <=> Seq[T] adaptations might then come into play below. I haven't thought this through enough to know if this matters, but did you consider more fine grained checking ("are both/neither java repeated?" "are both/neither Scala repeated").

I didn't give it that much thought, but I think it's ok. This is to shield us from comparing the second-class repeated param types to first-class expression types.

If tp is a repeated param type, so must pt be. If we don't check that, we'll go up the BTS of those types, hitting Seq / Array, and then relating those repeated param types to regular expression types. We should really think of repeated param types as abstract types that you can only convert out of with a conversion (that conversion is already in adapt because of the tighter conformance implemented here.)

retronym · 2019-01-29T04:45:39Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

          instantiatePossiblyExpectingUnit(tree, mode, pt)
        }
+        // TODO: we really shouldn't use T* as a first class types (e.g. for repeated case fields), but we can't allow T* to conform to other types (see isCompatible) because that breaks overload resolution
+        else if (isScalaRepeatedParamType(tree.tpe) && !isScalaRepeatedParamType(pt)) adapt(tree setType(repeatedToSeq(tree.tpe)), mode, pt)


This looks okay on the assumption that the * only appears in the outermost part of tree.tp / pt.

For case classes, where we have the unfortunate case of * appearing at all, this seems to hold true:

scala> class C(val c: String*) defined class C scala> new C("") res0: C = C@558756be scala> res0.c val c: String* scala> res0.c res1: Seq[String] = WrappedArray("") scala> class C(val c: String*) { def foo = Seq(c) } defined class C scala> new C("").foo def foo: Seq[Seq[String]] scala> { () => new C("").foo } res3: () => Seq[Seq[String]] = $$Lambda$1467/1307655632@2ed7978c

Yep, we shouldn't emit fields with type T*, but then we need to adjust other logic to turn those back into varargs where needed. I chose the expedient route :-)

the language in question was added by Adriaan in scala#7680 but then accidentally got removed in 2020 by scala#7432 I looked through other spec commits in that date span and didn't find anything else that went missing

scala-jenkins added this to the 2.13.0-RC1 milestone Jan 24, 2019

adriaanm mentioned this pull request Jan 24, 2019

More precise inference for overloaded methods when arguments types align #7631

Merged

This comment has been minimized.

Sign in to view

adriaanm force-pushed the overload_poly_repeated branch 2 times, most recently from 033039b to 8adcadb Compare January 24, 2019 15:33

adriaanm requested a review from retronym January 24, 2019 15:37

adriaanm modified the milestones: 2.13.0-RC1, 2.13.1 Jan 24, 2019

adriaanm force-pushed the overload_poly_repeated branch 2 times, most recently from 1c235d6 to fd9dcb2 Compare January 25, 2019 13:29

This comment has been minimized.

Sign in to view

adriaanm force-pushed the overload_poly_repeated branch from fd9dcb2 to 4cfeab8 Compare January 25, 2019 14:55

adriaanm force-pushed the overload_poly_repeated branch from 4cfeab8 to 41cd12e Compare January 25, 2019 16:39

adriaanm force-pushed the overload_poly_repeated branch from 41cd12e to 1a2cc1b Compare January 28, 2019 11:30

retronym reviewed Jan 29, 2019

View reviewed changes

som-snytt reviewed Jan 29, 2019

View reviewed changes

retronym reviewed Jan 29, 2019

View reviewed changes

sjrd mentioned this pull request Jan 30, 2019

Release 2.13.0-RC1 scala/scala-dev#599

Closed

51 tasks

adriaanm merged commit 0a09f43 into scala:2.13.x Feb 4, 2019

szeiger mentioned this pull request Feb 5, 2019

Revert immutable varargs #7703

Closed

abgruszecki mentioned this pull request Apr 9, 2019

Fix #5792 scala/scala3#6230

Closed

SethTisue mentioned this pull request May 23, 2019

Update scala 2.13.0-M5 to 2.13.0-RC1 lightbend-labs/scala-logging#159

Closed

adriaanm added the backport candidate label Jun 25, 2019

SethTisue mentioned this pull request Nov 23, 2022

restore a bit of spec language that went missing #10225

Merged

pan3793 mentioned this pull request Aug 10, 2023

[TOREE-545] Upgrade Scalatest 3.2 and Mockito 4 apache/incubator-toree#211

Merged

som-snytt mentioned this pull request Aug 13, 2024

Java method with boxed/unboxed overloads causes ambiguity error if more than one boxed argument scala/bug#10532

Closed

Align specificity of (a: T)... vs (a: T*)... with mono case #7680

Align specificity of (a: T)... vs (a: T*)... with mono case #7680

Uh oh!

Conversation

adriaanm commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Jan 24, 2019

Uh oh!

This comment has been minimized.

adriaanm commented Jan 24, 2019

Uh oh!

adriaanm commented Jan 24, 2019

Uh oh!

adriaanm commented Jan 24, 2019

Uh oh!

som-snytt commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

adriaanm commented Jan 25, 2019

Uh oh!

adriaanm commented Jan 25, 2019

Uh oh!

som-snytt commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Jan 25, 2019

Uh oh!

adriaanm commented Jan 25, 2019

Uh oh!

adriaanm commented Jan 28, 2019

Uh oh!

som-snytt commented Jan 28, 2019

Uh oh!

adriaanm commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Align specificity of `(a: T)...` vs `(a: T*)...` with mono case #7680

Align specificity of `(a: T)...` vs `(a: T*)...` with mono case #7680

adriaanm commented Jan 24, 2019 •

edited

Loading

som-snytt commented Jan 24, 2019 •

edited

Loading

som-snytt commented Jan 25, 2019 •

edited

Loading