Skip to content
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

Rework unification of Object and Any in Java/Scala interop #7966

Merged
merged 1 commit into from
May 8, 2019

Conversation

retronym
Copy link
Member

@retronym retronym commented Apr 4, 2019

References to j.l.Object from Java sources or classfiles use
definitions.ObjectTpeJava, which behaves like ObjectTpe but is
slackened to AnyTpe during =:= and <:< when this can help yield
positive results.

This subsumes two prior mechanisms (usages of obj2Any and
MethodType.isJava)

Fixes scala/bug#10418
Fixes scala/bug#11469

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Apr 4, 2019
def removeAll(x$1: java.util.Collection[_]): Boolean = ???
def retainAll(x$1: java.util.Collection[_]): Boolean = ???
def size(): Int = ???
def toArray[T](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray[T <: Object](x$1: Array[T]): Array[T] = ???
Copy link
Member Author

@retronym retronym Apr 4, 2019

Choose a reason for hiding this comment

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

Interesting change here. Needs to look at this more to decide if its a re- or pro-gression...

@retronym retronym requested a review from adriaanm April 4, 2019 06:05
@retronym retronym force-pushed the ticket/10418 branch 3 times, most recently from 3426455 to 565be77 Compare April 5, 2019 06:44
@retronym
Copy link
Member Author

Chatted about this with @odersky and @adriaanm. A slightly cleaner way to express this might be to make ObjectTpeJava a type ref to a synthetic type alias (named 'Object', owned by java.lang, but not entered in its scope). The RHS of that alias would be a bounded wildcard type >: Object <: Any. We might need to make sure that the constraint solver deals with this type appropriately.

References to `j.l.Object` from Java sources or classfiles use
`definitions.ObjectTpeJava`, which behaves like `ObjectTpe`
but is slackened to `AnyTpe` during `=:=` and `<:<` when this
can help yield positive results.

This subsumes two prior mechanisms (usages of `obj2Any` and `MethodType.isJava`)
@adriaanm adriaanm modified the milestones: 2.13.1, 2.13.0-RC2 May 8, 2019
@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Dropping 81a7e77, adding another 11469 test to the first commit.

@adriaanm adriaanm changed the title WIP Rework the way we unify Object and Any in Java/Scala interop Rework unification of Object and Any in Java/Scala interop May 8, 2019
@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

scala-js says /cc @sjrd

javalanglib/src/main/scala/java/lang/Iterable.scala:18: error: 
type arguments [T] do not conform to trait Iterator's type parameter bounds [E <: Object]
   def iterator(): Iterator[T]
                   ^

@sjrd
Copy link
Member

sjrd commented May 8, 2019

That looks problematic. The file Iterable.scala is here:
https://github.com/scala-js/scala-js/blob/v1.0.0-M7/javalanglib/src/main/scala/java/lang/Iterable.scala
It literally contains

package java.lang

import java.util.Iterator

trait Iterable[T] {
  def iterator(): Iterator[T]
}

It doesn't seem like this pattern is particularly Scala.js-specific. It's a straightforward mention of a Java interface with a Scala type parameter.

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

So you don't do any weird tricks to rewire the info of the type param to use Object as its bound?

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Sorry, scratch that. I thought I was looking at the source of Iterator

@sjrd
Copy link
Member

sjrd commented May 8, 2019

So you don't do any weird tricks to rewire the info of the type param to use Object as its bound?

OMG no. I don't touch the typer. I don't even touch infos at all, unless in the presence of types that extend js.Any.

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Haha ok phew 😅

Still weird, as this PR's standard scalac compiles this source file without problem.

@sjrd
Copy link
Member

sjrd commented May 8, 2019

Looks like a dbuild-specific issue. I cannot reproduce the issue locally with

> set resolvers in Global += "scala-pr-validation" at "https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots/"
> ++2.13.0-pre-d273f13-SNAPSHOT
> javalanglib/compile

I hate dbuild-specific issues 😫

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Bizarre. Dbuild is actually using 2.13.0-pre-751a34f-SNAPSHOT, which should be identical. Could you please try with that one as well?

@sjrd
Copy link
Member

sjrd commented May 8, 2019

Bizarre. Dbuild is actually using 2.13.0-pre-751a34f-SNAPSHOT, which should be identical. Could you please try with that one as well?

Same outcome: works fine locally.

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Thanks. Well, I guess I'm in for some fun dbuild debugging :-)

@adriaanm
Copy link
Contributor

adriaanm commented May 8, 2019

Well, at least it fixed the issue with jackson-module-scala -- it built fine with my workaround reverted

adriaanm added a commit to scala/community-build that referenced this pull request May 8, 2019
adriaanm added a commit that referenced this pull request May 9, 2019
@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2019

[EDIT]: the akka testkit breakage was not due to this PR, scala/bug#11523


Seems like this did break something in akka testkit -- haven't looked beyond copy pasting the error from https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/2087/consoleText:

akka-testkit/src/main/java/akka/testkit/JavaTestKit.java:150:1: <R>compose(scala.PartialFunction<R,T1>) in scala.runtime.AbstractPartialFunction cannot implement <R>compose(scala.PartialFunction<R,A>) in scala.PartialFunction
  return type scala.PartialFunction<R,R> is not compatible with scala.PartialFunction<R,java.lang.Object>
          new JavaPartialFunction<Object, Object>() {
akka-testkit/src/main/java/akka/testkit/JavaTestKit.java:561:1: <R>compose(scala.PartialFunction<R,T1>) in scala.runtime.AbstractPartialFunction cannot implement <R>compose(scala.PartialFunction<R,A>) in scala.PartialFunction
  return type scala.PartialFunction<R,R> is not compatible with scala.PartialFunction<R,T>
                  new CachingPartialFunction<Object, T>() {

(akka-testkit / Compile / compileIncremental) javac returned non-zero exit code

@adriaanm
Copy link
Contributor

/cc @anatoliykmetyuk this would be a good candidate to forward port to dotc. Note the follow-up PR, which makes the special ObjectJavaTpe truly unique.

@SethTisue
Copy link
Member

#8049 is a followup to this

@ches
Copy link

ches commented May 24, 2019

this would be a good candidate to forward port to dotc

@adriaanm Is this likely to get backported too? If it resolves jdk11 migration issues like scala/bug#10418 for 2.11/12.

@SethTisue
Copy link
Member

as far as I know a backport is unlikely given the tricky nature of the changes made.

@smarter
Copy link
Member

smarter commented Jan 14, 2020

This PR description says:

References to j.l.Object from Java sources or classfiles use
definitions.ObjectTpeJava

But as far as I can tell, ObjectTpeJava is only used for references to Object coming from classfiles and never for references coming from .java source files, and in fact some of the tests in this PR that are only run under separate compilation currently fail under joint compilation:

% scalac test/files/pos/t11469_2/Test_1.java test/files/pos/t11469_2/Test_2.scala
test/files/pos/t11469_2/Test_2.scala:5: error: name clash between defined and inherited member:
def m(a: java.util.Optional[_]): Unit in class B and
override def m(a: java.util.Optional[_ <: Object]): Unit at line 5
have same type after erasure: (a: java.util.Optional)Unit
    override def m(a: java.util.Optional[_ <: Object]): Unit = { }
                 ^
one error found

Is this intentional ?

@retronym
Copy link
Member Author

retronym commented Jan 14, 2020

But as far as I can tell, ObjectTpeJava is only used for references to Object coming from classfiles and never for references coming from .java source files

The change to checkAccessible works for references to Object from .java source files. But maybe there is a hole in way we expand Foo[_] to Foo[_ <: Object]...

@retronym
Copy link
Member Author

#8638

smarter added a commit to dotty-staging/dotty that referenced this pull request Aug 19, 2020
In Java unlike Scala, `Object` is the top type, this leads to various
usability problems when attempting to call or override a Java method
from Scala. So far, we relied mainly on one mechanism to improve the
situation: in the ClassfileParser, some references to `Object` in
signatures were replaced by `Any` (cf `objToAny`). But this had several
shortcomings:
- To compensate for this substitution,
  `TypeComparer#matchingMethodParams` had to special case Any in Java
  methods and treat it like Object
- There were various situation were this substitution was not applied,
  notably when using varargs (`Object... args`) or when jointly
  compiling .java sources since this is handled by JavaParser and not
  ClassfileParser.

This commit replaces all of this by a more systematic solution: all
references to `Object` in Java definitions (both in classfiles and .java
source files) are replaced by a special type `FromJavaObject` which is a
type alias of `Object` with one extra subtyping rule:

   tp <:< FromJavaObject

is equivalent to:

   tp <:< Any

See the documentation of `FromJavaObjectSymbol` for details on
why this makes sense.

This solution is very much inspired by
scala/scala#7966 (which was refined in
scala/scala#8049 and
scala/scala#8638) with two main differences:
- We use a type with its own symbol and name distinct from
  `java.lang.Object`, because this type can show up in inferred types
  and therefore needs to be preserved when pickling so that unpickled
  trees pass `-Ycheck`.
- Unlike in Scala 2, we cannot get rid of `JavaMethodType` because when
  calling `Type#signature` we need to know whether we're in a Java
  method or not, because signatures are based on erased types and erasure
  differs between Scala and Java (we cannot ignore this and always
  base signatures on the Scala erasure, because signatures are used
  to distinguish between overloads and overrides so they must agree
  with the actual JVM bytecode signature).

Fixes scala#7467, scala#7963, scala#8588, scala#8977.
smarter added a commit to dotty-staging/dotty that referenced this pull request Aug 21, 2020
In Java unlike Scala, `Object` is the top type, this leads to various
usability problems when attempting to call or override a Java method
from Scala. So far, we relied mainly on one mechanism to improve the
situation: in the ClassfileParser, some references to `Object` in
signatures were replaced by `Any` (cf `objToAny`). But this had several
shortcomings:
- To compensate for this substitution,
  `TypeComparer#matchingMethodParams` had to special case Any in Java
  methods and treat it like Object
- There were various situation were this substitution was not applied,
  notably when using varargs (`Object... args`) or when jointly
  compiling .java sources since this is handled by JavaParser and not
  ClassfileParser.

This commit replaces all of this by a more systematic solution: all
references to `Object` in Java definitions (both in classfiles and .java
source files) are replaced by a special type `FromJavaObject` which is a
type alias of `Object` with one extra subtyping rule:

   tp <:< FromJavaObject

is equivalent to:

   tp <:< Any

See the documentation of `FromJavaObjectSymbol` for details on
why this makes sense.

This solution is very much inspired by
scala/scala#7966 (which was refined in
scala/scala#8049 and
scala/scala#8638) with two main differences:
- We use a type with its own symbol and name distinct from
  `java.lang.Object`, because this type can show up in inferred types
  and therefore needs to be preserved when pickling so that unpickled
  trees pass `-Ycheck`.
- Unlike in Scala 2, we cannot get rid of `JavaMethodType` because when
  calling `Type#signature` we need to know whether we're in a Java
  method or not, because signatures are based on erased types and erasure
  differs between Scala and Java (we cannot ignore this and always
  base signatures on the Scala erasure, because signatures are used
  to distinguish between overloads and overrides so they must agree
  with the actual JVM bytecode signature).

Fixes scala#7467, scala#7963, scala#8588, scala#8977.
@retronym retronym mentioned this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidate java interop release-notes worth highlighting in next release notes
Projects
None yet
9 participants