Skip to content

Commit

Permalink
ObjectTpeJava part 2
Browse files Browse the repository at this point in the history
Make sure the TypeRef is truly unique, even when embedded
in other types, such as a TypeBounds. Since we're dealing
with structural equality, `TypeBounds(lo, hi) == TypeBounds(lo', hi')`
if `lo == lo'` and `hi == hi'`, if `hi = ObjectTpe` and `hi' = ObjectTpe'`,
`unique`'ing (hashconsing) such a `TypeBounds` would indirectly
replace `ObjectTpe` with `ObjectTpeJava` or vice versa.

We can't use `eq` when trying to identify `ObjectTpeJava` in `unique`
due to cycles.

`isAnyTpe` returns `true` for `ObjectTpeJava`.

Further fixes by Lukas:
  - Don't show ObjectTpeJava upper bound in TypeBounds.toString
  - Upper bounds of wildcards should be ObjectTpeJava
  - Fix wildcardExtrapolation for BoundedWildcardType with hi ObjectTpeJava

Reduce `_ >: lo <: ObjectTpeJava` to `lo`. This eliminates the
`BoundedWildcardType` in the expected type when type-checking the
lambda

    (c: java.util.Collection[String]).removeIf(x => true)

where

    boolean removeIf(Predicate<? super E> filter)
  • Loading branch information
adriaanm committed May 13, 2019
1 parent 1ae9f82 commit dc0180b
Show file tree
Hide file tree
Showing 10 changed files with 101 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -672,11 +672,11 @@ abstract class ClassfileParser(reader: ReusableInstance[ReusableDataReader]) {
case '+' => TypeBounds.upper(sig2type(tparams, skiptvs))
case '-' =>
val tp = sig2type(tparams, skiptvs)
// sig2type seems to return AnyClass regardless of the situation:
// we don't want Any as a LOWER bound.
if (tp.typeSymbol == AnyClass) TypeBounds.empty
else TypeBounds.lower(tp)
case '*' => TypeBounds.empty
// Interpret `sig2type` returning `Any` as "no bounds";
// morally equivalent to TypeBounds.empty, but we're representing Java code, so use ObjectTpeJava for AnyTpe.
if (tp.typeSymbol == AnyClass) TypeBounds.upper(definitions.ObjectTpeJava)
else TypeBounds(tp, definitions.ObjectTpeJava)
case '*' => TypeBounds.upper(definitions.ObjectTpeJava)
}
val newtparam = sym.newExistential(newTypeName("?"+i), sym.pos) setInfo bounds
existentials += newtparam
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ trait Definitions extends api.StandardDefinitions {
*
* We use `ObjectTpeJava`'s identity to equate it, but not `ObjectTpe`, to `AnyTpe` in subtyping and type equality.
*/
lazy val ObjectTpeJava = mkObjectTpeJava
lazy val ObjectTpeJava = new ObjectTpeJavaRef

lazy val SerializableTpe = SerializableClass.tpe
lazy val StringTpe = StringClass.tpe
Expand Down
29 changes: 18 additions & 11 deletions src/reflect/scala/reflect/internal/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,11 @@ trait Types
if (this eq other.asInstanceOf[AnyRef]) true
else other match {
case otherTypeRef: TypeRef =>
Objects.equals(pre, otherTypeRef.pre) && sym.eq(otherTypeRef.sym) && sameElementsEquals(args, otherTypeRef.args)
Objects.equals(pre, otherTypeRef.pre) &&
sym.eq(otherTypeRef.sym) &&
sameElementsEquals(args, otherTypeRef.args) &&
// `ObjectTpeJavaRef` is not structurally equal to `ObjectTpe` -- they should not be collapsed by `unique`
!(this.isInstanceOf[ObjectTpeJavaRef] || otherTypeRef.isInstanceOf[ObjectTpeJavaRef])
case _ => false
}
}
Expand Down Expand Up @@ -2706,18 +2710,21 @@ trait Types
private final class ClassArgsTypeRef(pre: Type, sym: Symbol, args: List[Type]) extends ArgsTypeRef(pre, sym, args)
private final class AliasNoArgsTypeRef(pre: Type, sym: Symbol) extends NoArgsTypeRef(pre, sym) with AliasTypeRef
private final class AbstractNoArgsTypeRef(pre: Type, sym: Symbol) extends NoArgsTypeRef(pre, sym) with AbstractTypeRef
private final class ClassNoArgsTypeRef(pre: Type, sym: Symbol) extends NoArgsTypeRef(pre, sym){
private final class ClassNoArgsTypeRef(pre: Type, sym: Symbol) extends NoArgsTypeRef(pre, sym) {
override def contains(sym0: Symbol): Boolean = (sym eq sym0) || pre.contains(sym0)
}

/** Expose ClassNoArgsTypeRef so we can create a non-uniqued ObjectTpeJava here and in reflect
*
* NOTE:
* - definitions.ObjectTpe is forced first, so that it ends up in the unique cache.
* - the created TypeRef is structurally equal to ObjectTpe, but with its own identity
* - we don't want the TypeRef we create here to be unique'd
*/
private[scala] def mkObjectTpeJava: Type = new ClassNoArgsTypeRef(definitions.ObjectTpe.prefix, definitions.ObjectClass)
/** Expose ObjectTpeJavaRef so we can create a non-uniqued ObjectTpeJava
* (using a type test rather than `eq`, which causes cycles).
*
* NOTE:
* - definitions.ObjectTpe is forced first, so that it ends up in the unique cache.
* - the created TypeRef is structurally equal to ObjectTpe, but with its own identity
* - we don't want the TypeRef we create here to be unique'd
*/
private[internal] final class ObjectTpeJavaRef extends NoArgsTypeRef(definitions.ObjectTpe.prefix, definitions.ObjectClass) {
override def contains(sym0: Symbol): Boolean = (sym eq sym0) || pre.contains(sym0)
}

object TypeRef extends TypeRefExtractor {
def apply(pre: Type, sym: Symbol, args: List[Type]): Type = unique({
Expand Down Expand Up @@ -5207,7 +5214,7 @@ trait Types
tp.dealias match {
case PolyType(_, tp) => typeIsAny(tp)
case TypeRef(_, AnyClass, _) => true
case _ => false
case _ => tp eq definitions.ObjectTpeJava
}

private[scala] val typeIsHigherKinded = (tp: Type) => tp.isHigherKinded
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/internal/tpe/TypeMaps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ private[internal] trait TypeMaps {
def apply(tp: Type): Type =
tp match {
case BoundedWildcardType(TypeBounds(lo, AnyTpe)) if variance.isContravariant => lo
case BoundedWildcardType(TypeBounds(lo, ObjectTpeJava)) if variance.isContravariant => lo
case BoundedWildcardType(TypeBounds(NothingTpe, hi)) if variance.isCovariant => hi
case tp => tp.mapOver(this)
}
Expand Down
2 changes: 1 addition & 1 deletion test/files/neg/abstract-class-error.check
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
S.scala:1: error: class S needs to be abstract. Missing implementation for:
def g(y: Int, z: java.util.List): Int // inherited from class J
(Note that java.util.List does not match java.util.List[String]. To implement this raw type, use java.util.List[_ <: Object])
(Note that java.util.List does not match java.util.List[String]. To implement this raw type, use java.util.List[_])
class S extends J {
^
one error found
6 changes: 3 additions & 3 deletions test/files/neg/abstract-report2.check
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Missing implementations for 13 members. Stub implementations follow:
def removeAll(x$1: java.util.Collection[_]): Boolean = ???
def retainAll(x$1: java.util.Collection[_]): Boolean = ???
def size(): Int = ???
def toArray[T <: Object](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray[T](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray(): Array[Object] = ???

class Foo extends Collection[Int]
Expand All @@ -29,7 +29,7 @@ Missing implementations for 13 members. Stub implementations follow:
def removeAll(x$1: java.util.Collection[_]): Boolean = ???
def retainAll(x$1: java.util.Collection[_]): Boolean = ???
def size(): Int = ???
def toArray[T <: Object](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray[T](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray(): Array[Object] = ???

class Bar extends Collection[List[_ <: String]]
Expand All @@ -47,7 +47,7 @@ Missing implementations for 13 members. Stub implementations follow:
def removeAll(x$1: java.util.Collection[_]): Boolean = ???
def retainAll(x$1: java.util.Collection[_]): Boolean = ???
def size(): Int = ???
def toArray[T <: Object](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray[T](x$1: Array[T with Object]): Array[T with Object] = ???
def toArray(): Array[Object] = ???

class Baz[T] extends Collection[T]
Expand Down
5 changes: 5 additions & 0 deletions test/files/pos/t10418_bounds.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Test {
def foo(c: java.util.Collection[String]): Unit = {
c.removeIf(x => true)
}
}
63 changes: 63 additions & 0 deletions test/files/pos/t11525.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// scalac: -Ystop-after:refchecks -verbose -Ydebug -uniqid
package java.lang

/* This is a pretty random test that very indirectly tests `unique`ing of `ObjectTpeJavaRef`
It's minimize from scala-js, where CI chanced on a compilation order that would first
unique `TypeBounds(lo, ObjectTpe)`, and then `TypeBounds(lo, ObjectTpeJava)`,
which would result in a Java reference to Object being replaced by one that is used
to represent a Scala occurrence of a reference to Object, which is distinct from Any.
When Java code refers to Object, it's taken as the same thing as Any, at least when
it comes to =:= and `... <:< Object-in-java`.
*/
import java.util.Iterator

class Class[A](o: Object)

class Comparable[A] { def compareTo(o: A): scala.Int = ??? }

object System {
def currentTimeMillis(): scala.Long = ???

def arraycopy(src: Object, srcPos: scala.Int, dest: Object, destPos: scala.Int, length: scala.Int): Unit = {
import scala.{Boolean, Double}

def mismatch(): Nothing =
throw new ArrayStoreException("Incompatible array types")

def copyPrim[@specialized T](src: Array[T], dest: Array[T]): Unit = {
var i = length-1
while (i >= 0) {
dest(i+destPos) = src(i+srcPos)
i -= 1
}
}

def copyRef(src: Array[AnyRef], dest: Array[AnyRef]): Unit = {
val x = (src.length, dest.length)

var i = length-1
while (i >= 0) {
dest(i+destPos) = src(i+srcPos)
i -= 1
}
}

(src match {
case src: Array[Boolean] =>
dest match {
case dest: Array[Boolean] => copyPrim(src, dest)
case _ => mismatch()
}

})
}

def identityHashCode(x: Object): scala.Int = {
x.getClass
1
}
}

trait Iterable[T] {
def iterator(): java.util.Iterator[T]
}
6 changes: 3 additions & 3 deletions test/files/run/reflection-magicsymbols-invoke.check
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ method ##: ()Int
method ==: (x$1: Any)Boolean
method asInstanceOf: [T0]=> T0
method equals: (x$1: Any)Boolean
method getClass: ()Class[_ <: Object]
method getClass: ()Class[_]
method hashCode: ()Int
method isInstanceOf: [T0]=> Boolean
method toString: ()String
Expand Down Expand Up @@ -45,7 +45,7 @@ method clone: ()Object
method eq: (x$1: AnyRef)Boolean
method equals: (x$1: Object)Boolean
method finalize: ()Unit
method getClass: ()Class[_ <: Object]
method getClass: ()Class[_]
method hashCode: ()Int
method isInstanceOf: [T0]=> Boolean
method ne: (x$1: AnyRef)Boolean
Expand Down Expand Up @@ -91,7 +91,7 @@ method clone: ()Array[T]
method eq: (x$1: AnyRef)Boolean
method equals: (x$1: Object)Boolean
method finalize: ()Unit
method getClass: ()Class[_ <: Object]
method getClass: ()Class[_]
method hashCode: ()Int
method isInstanceOf: [T0]=> Boolean
method length: => Int
Expand Down
2 changes: 1 addition & 1 deletion test/files/run/t5072.check
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ scala> class C
defined class C

scala> Thread.currentThread.getContextClassLoader.loadClass(classOf[C].getName)
res0: Class[_ <: Object] = class C
res0: Class[_] = class C

scala> :quit

0 comments on commit dc0180b

Please sign in to comment.