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

Improve handling of references to Object coming from Java code #9601

Merged
merged 4 commits into from
Aug 22, 2020

Conversation

smarter
Copy link
Member

@smarter smarter commented 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 #7467, #7963, #8588, #8977.

@odersky
Copy link
Contributor

odersky commented Aug 20, 2020

LGTM, except for the missing explanation of the puzzler in translateJavaArrayElementType.

@odersky odersky assigned smarter and unassigned odersky Aug 20, 2020
This appears to already be fixed.
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.
Safer to use than ctx.compilationUnit.isJava since compilationUnit is
currently nullable (and ends up being null at least in some of our tests).
@smarter
Copy link
Member Author

smarter commented Aug 21, 2020

Addressed comments by updating the documentation of FromJaveObjectSymbol and translateJavaArrayElementType, as well as adding an extra commit which introduces Context#isJava and uses it where possible.

@smarter smarter assigned odersky and unassigned smarter Aug 21, 2020
@retronym
Copy link
Member

Thanks for aligning this, @smarter.

@smarter smarter mentioned this pull request Sep 2, 2020
smarter added a commit to dotty-staging/dotty that referenced this pull request Sep 3, 2020
We recntly merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).
smarter added a commit to dotty-staging/dotty that referenced this pull request Sep 3, 2020
We recently merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).
smarter added a commit to dotty-staging/dotty that referenced this pull request Sep 3, 2020
We recently merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).

Fixes scala#9688, supercedes scala#6230.
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.

Dotty rejects all overrides of some Java methods as duplicate methods
3 participants