Skip to content

Tweak and implement weak conformance replacement #5371

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 3 commits into from
Nov 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 32 additions & 33 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,8 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
addTyped(arg, formal)
case _ =>
val elemFormal = formal.widenExpr.argTypesLo.head
val typedArgs = harmonic(harmonizeArgs)(args.map(typedArg(_, elemFormal)))
val typedArgs =
harmonic(harmonizeArgs, elemFormal)(args.map(typedArg(_, elemFormal)))
typedArgs.foreach(addArg(_, elemFormal))
makeVarArg(args.length, elemFormal)
}
Expand Down Expand Up @@ -1555,38 +1556,33 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
}

private def harmonizeWith[T <: AnyRef](ts: List[T])(tpe: T => Type, adapt: (T, Type) => T)(implicit ctx: Context): List[T] = {
def numericClasses(ts: List[T], acc: Set[Symbol]): Set[Symbol] = ts match {
def targetClass(ts: List[T], cls: Symbol, intLitSeen: Boolean): Symbol = ts match {
case t :: ts1 =>
val sym = tpe(t).widen.classSymbol
if (sym.isNumericValueClass) numericClasses(ts1, acc + sym)
else Set()
tpe(t).widenTermRefExpr match {
case ConstantType(c: Constant) if c.tag == IntTag =>
targetClass(ts1, cls, true)
case t =>
val sym = t.widen.classSymbol
if (!sym.isNumericValueClass || cls.exists && cls != sym) NoSymbol
else targetClass(ts1, sym, intLitSeen)
}
case Nil =>
acc
if (cls != defn.IntClass && intLitSeen) cls else NoSymbol
}
val clss = numericClasses(ts, Set())
if (clss.size > 1) {
def isCompatible(cls: Symbol, sup: TypeRef) =
defn.isValueSubType(cls.typeRef, sup) &&
!(cls == defn.LongClass && sup.isRef(defn.FloatClass))
// exclude Long <: Float from list of allowable widenings
// TODO: should we do this everywhere we ask for isValueSubType?

val lub = defn.ScalaNumericValueTypeList.find(lubTpe =>
clss.forall(cls => isCompatible(cls, lubTpe))).get

def lossOfPrecision(ct: Constant): Boolean =
ct.tag == IntTag && lub.isRef(defn.FloatClass) &&
ct.intValue.toFloat.toInt != ct.intValue ||
ct.tag == LongTag && lub.isRef(defn.DoubleClass) &&
ct.longValue.toDouble.toLong != ct.longValue

val cls = targetClass(ts, NoSymbol, false)
if (cls.exists) {
def lossOfPrecision(n: Int): Boolean =
cls == defn.FloatClass && n.toFloat.toInt != n
Copy link
Member

Choose a reason for hiding this comment

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

What about Bytes and Shorts? (and Chars, if they are considered numeric after all?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are covered under c.convertTo(cls.typeRef) != null

var canAdapt = true
val ts1 = ts.mapConserve { t =>
tpe(t).widenTermRefExpr match {
case ct: ConstantType if !lossOfPrecision(ct.value) => adapt(t, lub)
case ConstantType(c: Constant) if c.tag == IntTag =>
canAdapt &= c.convertTo(cls.typeRef) != null && !lossOfPrecision(c.intValue)
if (canAdapt) adapt(t, cls.typeRef) else t
case _ => t
}
}
if (numericClasses(ts1, Set()).size == 1) ts1 else ts
if (canAdapt) ts1 else ts
}
else ts
}
Expand All @@ -1603,7 +1599,8 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
if (ctx.isAfterTyper) trees else harmonizeWith(trees)(_.tpe, adaptDeep)
}

/** Apply a transformation `harmonize` on the results of operation `op`.
/** Apply a transformation `harmonize` on the results of operation `op`,
* unless the expected type `pt` is fully defined.
* If the result is different (wrt eq) from the original results of `op`,
* revert back to the constraint in force before computing `op`.
* This reset is needed because otherwise the original results might
Expand All @@ -1612,13 +1609,15 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
* the result of harmomization will be compared again with the expected type.
* Test cases where this matters are in pos/harmomize.scala.
*/
def harmonic[T](harmonize: List[T] => List[T])(op: => List[T])(implicit ctx: Context): List[T] = {
val origConstraint = ctx.typerState.constraint
val origElems = op
val harmonizedElems = harmonize(origElems)
if (harmonizedElems ne origElems) ctx.typerState.constraint = origConstraint
harmonizedElems
}
def harmonic[T](harmonize: List[T] => List[T], pt: Type)(op: => List[T])(implicit ctx: Context): List[T] =
if (!isFullyDefined(pt, ForceDegree.none)) {
val origConstraint = ctx.typerState.constraint
val origElems = op
val harmonizedElems = harmonize(origElems)
if (harmonizedElems ne origElems) ctx.typerState.constraint = origConstraint
harmonizedElems
}
else op

/** If all `types` are numeric value types, and they are not all the same type,
* pick a common numeric supertype and widen any constant types in `tpes` to it.
Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ class Typer extends Namer

def typedIf(tree: untpd.If, pt: Type)(implicit ctx: Context): Tree = track("typedIf") {
val cond1 = typed(tree.cond, defn.BooleanType)
val thenp2 :: elsep2 :: Nil = harmonic(harmonize) {
val thenp2 :: elsep2 :: Nil = harmonic(harmonize, pt) {
val thenp1 = typed(tree.thenp, pt.notApplied)
val elsep1 = typed(tree.elsep orElse (untpd.unitLiteral withPos tree.pos), pt.notApplied)
thenp1 :: elsep1 :: Nil
Expand Down Expand Up @@ -979,7 +979,7 @@ class Typer extends Namer

// Overridden in InlineTyper for inline matches
def typedMatchFinish(tree: untpd.Match, sel: Tree, selType: Type, pt: Type)(implicit ctx: Context): Tree = {
val cases1 = harmonic(harmonize)(typedCases(tree.cases, selType, pt.notApplied))
val cases1 = harmonic(harmonize, pt)(typedCases(tree.cases, selType, pt.notApplied))
.asInstanceOf[List[CaseDef]]
assignType(cpy.Match(tree)(sel, cases1), sel, cases1)
}
Expand Down Expand Up @@ -1135,7 +1135,7 @@ class Typer extends Namer
}

def typedTry(tree: untpd.Try, pt: Type)(implicit ctx: Context): Try = track("typedTry") {
val expr2 :: cases2x = harmonic(harmonize) {
val expr2 :: cases2x = harmonic(harmonize, pt) {
val expr1 = typed(tree.expr, pt.notApplied)
val cases1 = typedCases(tree.cases, defn.ThrowableType, pt.notApplied)
expr1 :: cases1
Expand Down
3 changes: 3 additions & 0 deletions compiler/test/dotc/run-from-tasty.blacklist
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
# Closure type miss match
eff-dependent.scala

# It seems we still harmonize types in fromTasty. Not sure where this happens
puzzle.scala
28 changes: 14 additions & 14 deletions docs/docs/reference/dropped/weak-conformance-spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ assigning a type to a constant expression. The new rule is:

and all expressions have primitive numeric types, but they do not
all have the same type, then the following is attempted:
- the expressions `Es` are partitioned into `Int` literals on the
one hand, and all other expressions on the other hand

- the expressions `Es` are partitioned into `Int` constants on the
one hand, and all other expressions on the other hand,
- if all the other expressions have the same numeric type `T`
(which can be one of `Byte`, `Short`, `Int`, `Long`, `Float`,
(which can be one of `Byte`, `Short`, `Char`, `Int`, `Long`, `Float`,
`Double`), possibly after widening, and if none of the `Int`
literals would incur a loss of precision when converted to `T`,
then they are thus converted (the other expressions are left
unchanged regardless)
- otherwise, the expressions `Es` are used unchanged
unchanged regardless),
- otherwise, the expressions `Es` are used unchanged.

A loss of precision occurs for an `Int -> Float` conversion of a constant
`c` if `c.toFloat.toInt != c`. For an `Int -> Byte` conversion it occurs
Expand All @@ -36,11 +36,11 @@ __Examples:__

inline val b = 33
def f(): Int = b + 1
List(b, 33, 5.5) : List[Double] // b is an inline val
List(f(), 33, 5.5) : List[AnyVal] // f() is not a constant
List(5, 11L) : List[Long]
List(5, 11L, 5.5) : List[AnyVal] // Long and Double found
List(1.0f, 2) : List[Float]
List(1.0f, 1234567890): List[AnyVal] // loss of precision
List(b, 33, 'a') : List[AnyVal] // Char is not a numeric
List(5.toByte, 11) : List[Byte]
Array(b, 33, 5.5) : Array[Double] // b is an inline val
Array(f(), 33, 5.5) : Array[AnyVal] // f() is not a constant
Array(5, 11L) : Array[Long]
Array(5, 11L, 5.5) : Array[AnyVal] // Long and Double found
Array(1.0f, 2) : Array[Float]
Array(1.0f, 1234567890): Array[AnyVal] // loss of precision
Array(b, 33, 'a') : Array[Char]
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed back from AnyVal to Char? Didn't we agree that Chars should never be considered as numeric types for the purpose of harmonization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we agreed that a Char literal should not be converted to another numeric type, same as a Float or a Double is not converted.

But this case is different: It's an Int constant that is converted to a Char type. Same as it could be converted to a Short or Byte type. I agree that Char feels strange, but the fact is it is a member of Java's numeric hierarchy, so I believe it's better to be consistent.

We do allow:

scala> val c: Char = 65
c: Char = A

So it's inconsistent to not also allow this in harmonization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see now that Char is not in the spec.

if all the other expressions have the same numeric type T (which can be one of Byte, Short, Int, Long, Float, Double)

Somehow I overlooked this before. So we'd either have to put Char back in the spec, or change the implementation to not convert to Char. At this stage it's all the same for me. Consistency with assignment is nice to have but if people feel strongly agaist I won't argue any longer.

Array(5.toByte, 11) : Array[Byte]
27 changes: 18 additions & 9 deletions tests/pos/harmonize.scala → tests/neg/harmonize.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ object Test {
val n = 1
inline val nn = 2
val y = if (x) 'A' else n
val z: Int = y
val z: Int = y // error: no widening

val yy1 = n match {
case 1 => 'A'
Expand All @@ -19,38 +19,47 @@ object Test {
case 2 => nn
case 3 => 1.0f
}
val zz2: Float = yy2 // widening to Float
val zz2: Float = yy2 // error: no widening from Char to Float

val yy3 = n match {
case 1 => 'A'
case 2 => 3L
case 3 => 1.0f
}
val zz3: Double = yy3 // widening to Double
val zz3: Double = yy3 // error: no widening from Char to Double

val a = try {
'A'
} catch {
case ex: Exception => nn
case ex: Error => 3L
}
val b: Long = a
val b: Long = a // error: no widening

val a_ok = try {
1
} catch {
case ex: Exception => nn
case ex: Error => 3L
}

val b_ok: Long = a_ok // ok

val xs = List(1.0, nn, 'c')
val ys: List[Double] = xs
val ys: List[Double] = xs // error: no widening

}

inline val b = 33
def f(): Int = b + 1
val a1 = Array(b, 33, 'a')
val b1: Array[Int] = a1
val b1: Array[Int] = a1 // error: no widening
val a2 = Array(b, 33, 'a', f())
val b2: Array[Int] = a2
val b2: Array[Int] = a2 // error: no widening
val a3 = Array(1.0f, 'a', 0)
val b3: Array[Float] = a3
val b3: Array[Float] = a3 // error: no widening
val a4 = Array(1.0f, 1L)
val b4: Array[Double] = a4
val b4: Array[Double] = a4 // error: no widening
val a5 = Array(1.0f, 1L, f())
val b5: Array[AnyVal] = a5
val a6 = Array(1.0f, 1234567890)
Expand Down
4 changes: 2 additions & 2 deletions tests/run/puzzle.check
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
53.0
53.0
5
5
1 change: 1 addition & 0 deletions tests/run/puzzle.scala
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// no longer a puzzler, now prints 5, 5
object Test {

def main(args: Array[String]): Unit = {
Expand Down
53 changes: 53 additions & 0 deletions tests/run/weak-conformance.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
object Test extends App {
inline val b = 33

locally {
def f(): Int = b + 1
val x1 = List(b, 33, 5.5) ; x1: List[Double] // b is an inline val
val x2 = List(f(), 33, 5.5) ; x2: List[AnyVal] // f() is not a constant
val x3 = List(5, 11L) ; x3: List[Long]
val x4 = List(5, 11L, 5.5) ; x4: List[AnyVal] // Long and Double found
val x5 = List(1.0f, 2) ; x5: List[Float]
val x6 = List(1.0f, 1234567890); x6: List[AnyVal] // loss of precision
val x7 = List(b, 33, 'a') ; x7: List[Char]
val x8 = List(5.toByte, 11) ; x8: List[Byte]

val x9: List[AnyVal] = List(1.0f, 0)
assert(x9(0).getClass == classOf[java.lang.Float])
assert(x9(1).getClass == classOf[java.lang.Float]) // expected type not fully defined, since `List` is covariant
val x10 = List[Any](1.0f, 0)
assert(x10(0).getClass == classOf[java.lang.Float])
assert(x10(1).getClass == classOf[java.lang.Integer])
}

locally {
def f(): Int = b + 1
val x1 = Array(b, 33, 5.5) ; x1: Array[Double] // b is an inline val
val x2 = Array(f(), 33, 5.5) ; x2: Array[AnyVal] // f() is not a constant
val x3 = Array(5, 11L) ; x3: Array[Long]
val x4 = Array(5, 11L, 5.5) ; x4: Array[AnyVal] // Long and Double found
val x5 = Array(1.0f, 2) ; x5: Array[Float]
val x6 = Array(1.0f, 1234567890); x6: Array[AnyVal] // loss of precision
val x7 = Array(b, 33, 'a') ; x7: Array[Char]
val x8 = Array(5.toByte, 11) ; x8: Array[Byte]

val x9: Array[AnyVal] = Array(1.0f, 0)
assert(x9(0).getClass == classOf[java.lang.Float])
assert(x9(1).getClass == classOf[java.lang.Integer]) // expected type fully defined since Array is nonvariant
val x10 = Array[Any](1.0f, 0)
assert(x10(0).getClass == classOf[java.lang.Float])
assert(x10(1).getClass == classOf[java.lang.Integer])
}

locally {
def f(): Int = b + 1
val x1 = if (true) b else if (true) 33 else 5.5 ; x1: Double // b is an inline val
val x2 = if (true) f() else if (true) 33 else 5.5) ; x2: AnyVal // f() is not a constant
val x3 = if (true) 5 else 11L) ; x3: Long
val x4 = if (true) 5 else if (true) 11L else 5.5) ; x4: AnyVal // Long and Double found
val x5 = if (true) 1.0f else 2) ; x5: Float
val x6 = if (true) 1.0f else 1234567890) ; x6: AnyVal // loss of precision
val x7 = if (true) b else if (true) 33 else 'a') ; x7: Char
val x8 = if (true) 5.toByte else 11) ; x8: Byte
}
}