Skip to content

Fix #5140: Ycheck failure in covariant java varargs #5403

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

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

abeln
Copy link
Contributor

@abeln abeln commented Nov 7, 2018

Allow passing Array[Dog] to a Java varargs that expects an
Array[Animal]. This types because Java vargs are represented as
RepeatedParam[+T], which is covariant.

However, after elimRepeated, the RPs go away and the true type
(Array) is revealed, which was causing a Ycheck error because arrays are
invariant before erasure.

Fix the Ycheck error by casting Array[Dog] to Array[Animal]. This is
unsound but consistent with the typer behaviour and what scalac does.

@abeln abeln requested a review from allanrenucci November 7, 2018 20:56
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is what scalac does then I suppose we should reproduce it

@smarter
Copy link
Member

smarter commented Nov 8, 2018

Could we instead change elimRepeated to replace RepeatedParam[X] by Array[_ <: X] ?

@abeln
Copy link
Contributor Author

abeln commented Nov 8, 2018

Switched to @smarter's suggestion that we change the type to Array[_ <: T], as opposed to a cast. PTAL

@abeln
Copy link
Contributor Author

abeln commented Nov 8, 2018

@smarter Here's the problem I run into with the Array[_ <: Animal] approach. If a Scala method overrides a Java varargs method, then elimRepeated has to add a bridge from the Scala method, which expects a Seq[Animal], to the Java signature of the method (which is what the callers see), which is Array[_ <: Animal].

The code generated by elimRepeated after this change then looks like

  ) class Sub() extends Base() { 
    def foo(x: Seq[Object] @Repeated): Object = x
    override def foo(x: Array[_ <: Object]): Object = 
      this.foo(Predef.wrapRefArray[Object](x))
  }
}

Which doesn't work because there's no sound way to convert an Array[_ <: Object] into a Seq[Object]. Or is there?

If there's no sound way, then I suggest we revert back to the asInstanceOf approach of the first commit.

@smarter
Copy link
Member

smarter commented Nov 8, 2018

The problem is that passing Object as the type argument to wrapRefArray doesn't work here, but this compile:

object Test {
  val x: Array[_ <: Object] = Array()
  val y = Predef.wrapRefArray(x)
  val z: Seq[Object] = y
}

The infered type argument is Array[_ <: Object]#T which is a bit awkward and can't be written in source code but can be inferred by the compiler. We could change whatever code is currently generating the wrapRefArray to do the same.

@abeln
Copy link
Contributor Author

abeln commented Nov 8, 2018

PTAL. Changed elimRepeated to generate

  ) class Sub() extends Base() { 
    def foo(x: Seq[Object] @Repeated): Object = x
      override def foo(x: Array[_ <: Object]): Object =
      this.foo(Predef.wrapRefArray[ <: Object](x))
  }
}

Which can't be written with surface syntax (unbound wildcard), but apparently typechecks.

Allow passing `Array[Dog]` to a Java varargs that expects an
`Array[Animal]`. This types because Java vargs are represented as
`RepeatedParam[+T]`, which is covariant.

However, after `elimRepeated`, the `RP`s go away and the true type
(Array) is revealed, which was causing a Ycheck error because arrays are
invariant before erasure.

Fix the Ycheck error by changing how `elimRepeated` transforms `RP[T]`s that
appear in Java code.

The previous transform was `RP[T]` => `Array[T]`.
The new transform is `RP[T]` => `Array[_ <: T]`.

This is unsound but consistent with the typer behaviour and what scalac does.
@abeln
Copy link
Contributor Author

abeln commented Nov 9, 2018

Addressed style comments and collapsed review commits.

@allanrenucci allanrenucci merged commit ef84d2f into scala:master Nov 9, 2018
@allanrenucci
Copy link
Contributor

Thanks @abeln!

@abeln abeln deleted the java-varargs branch November 12, 2018 15:49
allanrenucci added a commit to dotty-staging/dotty that referenced this pull request Dec 7, 2018
This is not necessary anymore after scala#5403
allanrenucci added a commit to dotty-staging/dotty that referenced this pull request Dec 9, 2018
This is not necessary anymore after scala#5403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants