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 f8e3126
Show file tree
Hide file tree
Showing 9 changed files with 96 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
63 changes: 63 additions & 0 deletions test/files/pos/t10418_unique.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 f8e3126

Please sign in to comment.