Override finalResultType in SingleType. #2934
Conversation
| case Id(x) => x | ||
| } | ||
| } |
There was a problem hiding this comment.
We also need some help for other SingletonTypes. Test4 below fails with:
qbin/scalac sandbox/test.scala
sandbox/test.scala:31: error: type mismatch;
found : x.type (with underlying type Test4)
required: Test4.this.type
case Id(x) => x
^
one error found
Unless we apply this patch:
git diff
diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala
index ca01a4b..269044d 100644
--- a/src/reflect/scala/reflect/internal/Types.scala
+++ b/src/reflect/scala/reflect/internal/Types.scala
@@ -1240,6 +1240,7 @@ trait Types
if (pre.isOmittablePrefix) pre.fullName + ".type"
else prefixString + "type"
}
+ override def finalResultType: Type = this
/*
override def typeOfThis: Type = typeSymbol.typeOfThis
@@ -1358,6 +1359,7 @@ trait Types
toBoolean(trivial)
}
override def isGround = sym.isPackageClass || pre.isGround
+ override def finalResultType: Type = if (sym.isModule) super.finalResultType else this
private[reflect] var underlyingCache: Type = NoType
private[reflect] var underlyingPeriod = NoPeriod
~/code/scala cat sandbox/test.scala
object Test2 {
// Has never worked, but seems desirable given the recent changes to
// pattern type inference.
val a = ""
object Id {
def unapply(xxxx: Any): Some[a.type] = Some[a.type](a)
}
val b: a.type = (a: a.type) match {
case Id(x) => x
}
}
object Test3 {
val a = ""
object Id {
def unapply(xxxx: Any): Some[Test3.type] = Some[Test3.type](Test3)
}
val b: Test3.type = a match {
case Id(x) => x
}
}
class Test4 {
val a = ""
object Id {
def unapply(xxxx: Any): Some[Test4.this.type] = Some[Test4.this.type](Test4.this)
}
val b: Test4.this.type = a match {
case Id(x) => x
}
}
There was a problem hiding this comment.
I originally tried to do it in SingletonType, but that didn't work out so good, as maybe you also found.
There was a problem hiding this comment.
But if I understand your patch here, maybe my problem was only that I should have found the "isModule" exception first.
There was a problem hiding this comment.
Weirdly, this needs the "final" to compile post-patch.
class Super5 {
final val q = ""
def q1: q.type = q
}
class Test5 extends Super5 {
val a = ""
object Id {
def unapply(xxxx: Any): Some[Test5.super.q.type] = Some[Test5.super.q.type](q1)
}
val b: Test5.super.q.type = a match {
case Id(x) => x
}
}
Without it:
test/files/pos/infersingle.scala:49: error: type mismatch;
found : Test5.this.q.type (with underlying type String)
required: Test5.super.q.type
def unapply(xxxx: Any): Some[Test5.super.q.type] = Some[Test5.super.q.type](q1)
^
one error found
Does that make any sense? Isn't it an upcast?
There was a problem hiding this comment.
And, I'm thinking you just hadn't yet gotten to the parts which made me move it to SingleType. With your patch:
scalac -d /tmp test/files/pos/context.scala
error: scala.reflect.internal.Types$TypeError: type mismatch;
found : Symbols.this.$outer.type
required: SymbolWrapper.this.type
at scala.tools.nsc.typechecker.Contexts$Context.issueCommon(Contexts.scala:546)
at scala.tools.nsc.typechecker.Contexts$Context.issue(Contexts.scala:551)
at scala.tools.nsc.typechecker.ContextErrors$ErrorUtils$.issueTypeError(ContextErrors.scala:97)
at scala.tools.nsc.typechecker.ContextErrors$ErrorUtils$.issueNormalTypeError(ContextErrors.scala:86)
There were a handful of others as well.
There was a problem hiding this comment.
They look like type equivalence and/or subtyping bugs. I'm opening a ticket.
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 24324112) |
|
Here's a minimization of the bootstrap failure: trait T {
class C {
private object O1 {
object O2
}
}
} |
|
That failure makes a certain amount of sense - the private type O1 appears in the public type O2. The generated outer accessor is apparently referencing it, and the inferred type is stronger than it used to be - strong enough to have O1 in it where it didn't before. |
|
I doubt that check is super important pastTyper. But perhaps it's best to widen the tpt of the accessor. |
|
Trying to widen the tpt reveals an interesting question: what does it mean to widen a type which has multiple narrow types in its prefix? For instance, I can widen x3.type several times: |
|
In fact I can't look any more closely as I am reminded of another reason I'm leaving, access. There is no right answer here, there is only an unspecified utter mess. |
|
The type of the outer accessor has to depend on who is asking, and there's no facility for that. It should have the tightest type locally - it has to for things to work which depend on local refinement - but a type which is "widened until visible" for each caller based on the site. I have tried ineffectually for years to get the tiniest crumb of attention for this matter. Ah, it'll be so nice to work on my own thing. |
|
Given my assessment I think the only option, literally, is to disable the check after typer. That's what's in the latest variation. |
|
I suppose the alternative is to re-add symbol specific behaviour to That said, I can't see any problem with disabling that check in the latter stages of the compiler, and I think this patch moves in the right direction. Considering this change on merits independently of the interaction with the ever confusing checks for type escapees, I'm suspect we haven't ironed out the inconsistencies between |
|
Speaking of knowing what methods are doing, imagine how much a better idea we'd have of the behavior of resultType vs. finalResultType if they were written more like this: |
|
Single failure, from shot in the dark taken last night. Will repush. |
|
My assessment is that untangling resultType and finalResultType exceeds my scope, because what it really involves is the grand task of unraveling all the accidental behavior of the type proxies. Key types (ExistentialType and SingletonType!) are implemented based on type proxies written a decade ago and which bear the laughable comment "Important to keep this up-to-date when new operations are added!" Meanwhile they have been updated approximately never. In addition they are broken by design, because they are written as if "underlying" were some universal concept which can be applied uniformly. In an AnnotatedType, underlying is the type without annotations. In an ExistentialType, it is the type which is supposed to be hidden from view. In ThisType, it is the type of the class as seen via sym.typeOfThis, which brings in the self-type. In SingleType, it is the cached result of computing (pre memberType sym).resultType. In SuperType, it is the type of the qualifier. In a ConstantType, it is the type of the Constant it carries. Now what are the chances that for some given operation that if one should forward the operation to underlying for ONE of these situations, one should do so for ALL of them? Dropping annotations can change what a type is completely, e.g. if you drop your @cps annotations you start conforming to Any. Dropping existential quantification is obviously not something to be done too glibly. I know you know all this, but I'm being explicit because I'm no longer in a position to pursue tangential matters. I have to remain exclusively focused on positions and classpath until those things are done. I'm a sucker for trying to fix things so I chased this one a little ways, but I have to stop even if it requires chaining myself to the radiator. |
|
Okay, that last one is my last attempt. It's the same as attempt N-2 with Test4 commented out. You have the con, Mr. Zaugg. |
|
Here's a snippet of a diffed of So still DNLGTM yet, messing with the semantically unclear --- sandbox/old.log 2013-09-13 09:29:43.000000000 +0200
+++ sandbox/new.log 2013-09-13 09:18:13.000000000 +0200
@@ -149073,29 +149073,29 @@
()
};
final private[this] val ALL: Int(-2147483648) = -2147483648;
- final <stable> <accessor> def ALL: Int = -2147483648;
+ final <stable> <accessor> def ALL: Int(-2147483648) = elidable.this.ALL;
final private[this] val FINEST: Int(300) = 300;
- final <stable> <accessor> def FINEST: Int = 300;
+ final <stable> <accessor> def FINEST: Int(300) = elidable.this.FINEST;
final private[this] val FINER: Int(400) = 400;
- final <stable> <accessor> def FINER: Int = 400;
+ final <stable> <accessor> def FINER: Int(400) = elidable.this.FINER;
final private[this] val FINE: Int(500) = 500;
- final <stable> <accessor> def FINE: Int = 500;
+ final <stable> <accessor> def FINE: Int(500) = elidable.this.FINE;
final private[this] val CONFIG: Int(700) = 700;
- final <stable> <accessor> def CONFIG: Int = 700;
+ final <stable> <accessor> def CONFIG: Int(700) = elidable.this.CONFIG;
final private[this] val INFO: Int(800) = 800;
- final <stable> <accessor> def INFO: Int = 800;
+ final <stable> <accessor> def INFO: Int(800) = elidable.this.INFO;
final private[this] val WARNING: Int(900) = 900;
- final <stable> <accessor> def WARNING: Int = 900;
+ final <stable> <accessor> def WARNING: Int(900) = elidable.this.WARNING;
final private[this] val SEVERE: Int(1000) = 1000;
- final <stable> <accessor> def SEVERE: Int = 1000;
+ final <stable> <accessor> def SEVERE: Int(1000) = elidable.this.SEVERE;
final private[this] val OFF: Int(2147483647) = 2147483647;
- final <stable> <accessor> def OFF: Int = 2147483647;
+ final <stable> <accessor> def OFF: Int(2147483647) = elidable.this.OFF;
final private[this] val MAXIMUM: Int(2147483647) = 2147483647;
- final <stable> <accessor> def MAXIMUM: Int = 2147483647;
+ final <stable> <accessor> def MAXIMUM: Int(2147483647) = elidable.this.MAXIMUM;
final private[this] val MINIMUM: Int(-2147483648) = -2147483648;
- final <stable> <accessor> def MINIMUM: Int = -2147483648;
+ final <stable> <accessor> def MINIMUM: Int(-2147483648) = elidable.this.MINIMUM;
final private[this] val ASSERTION: Int(2000) = 2000;
- final <stable> <accessor> def ASSERTION: Int = 2000;
+ final <stable> <accessor> def ASSERTION: Int(2000) = elidable.this.ASSERTION;
private[this] val byName: Map[String,Int] = scala.this.Predef.Map.apply[String, Int](scala.this.Predef.ArrowAssoc[String]("FINEST").->[Int](300), scala.this.Predef.ArrowAssoc[String]("FINER").->[Int](400), scala.this.Predef.ArrowAssoc[String]("FINE").->[Int](500), scala.this.Predef.ArrowAssoc[String]("CONFIG").->[Int](700), scala.this.Predef.ArrowAssoc[String]("INFO").->[Int](800), scala.this.Predef.ArrowAssoc[String]("WARNING").->[Int](900), scala.this.Predef.ArrowAssoc[String]("SEVERE").->[Int](1000), scala.this.Predef.ArrowAssoc[String]("ASSERTION").->[Int](2000), scala.this.Predef.ArrowAssoc[String]("ALL").->[Int](-2147483648), scala.this.Predef.ArrowAssoc[String]("OFF").->[Int](2147483647), scala.this.Predef.ArrowAssoc[String]("MAXIMUM").->[Int](2147483647), scala.this.Predef.ArrowAssoc[String]("MINIMUM").->[Int](-2147483648));
<stable> <accessor> def byName: Map[String,Int] = elidable.this.byName
}
@@ -149941,9 +149941,9 @@
()
};
final private[this] val MinValue: Byte(-128) = -128;
- final <stable> <accessor> def MinValue: Byte = -128;
+ final <stable> <accessor> def MinValue: Byte(-128) = Byte.this.MinValue;
final private[this] val MaxValue: Byte(127) = 127;
- final <stable> <accessor> def MaxValue: Byte = 127;
+ final <stable> <accessor> def MaxValue: Byte(127) = Byte.this.MaxValue;
def box(x: Byte): Byte = java.lang.Byte.valueOf(x);
def unbox(x: Object): Byte = x.asInstanceOf[Byte].byteValue();
override def toString: String = "object scala.Byte";
@@ -150079,9 +150079,9 @@
()
};
final private[this] val MinValue: Char('\00') = '\00';
- final <stable> <accessor> def MinValue: Char = '\00';
+ final <stable> <accessor> def MinValue: Char('\00') = Char.this.MinValue;
final private[this] val MaxValue: Char('?') = '?';
- final <stable> <accessor> def MaxValue: Char = '?';
+ final <stable> <accessor> def MaxValue: Char('?') = Char.this.MaxValue;
def box(x: Char): Character = java.lang.Character.valueOf(x);
def unbox(x: Object): Char = x.asInstanceOf[Character].charValue();
override def toString: String = "object scala.Char";
@@ -150353,11 +150353,11 @@
()
};
final private[this] val LogWL: Int(6) = 6;
- final <stable> <accessor> private[collection] def LogWL: Int = 6;
+ final <stable> <accessor> private[collection] def LogWL: Int(6) = BitSetLike.this.LogWL;
final private[this] val WordLength: Int(64) = 64;
- final <stable> <accessor> private def WordLength: Int = 64;
+ final <stable> <accessor> private def WordLength: Int(64) = BitSetLike.this.WordLength;
final private[this] val MaxSize: Int(33554432) = 33554432;
- final <stable> <accessor> private[collection] def MaxSize: Int = 33554432;
+ final <stable> <accessor> private[collection] def MaxSize: Int(33554432) = BitSetLike.this.MaxSize;
private[collection] def updateArray(elems: Array[Long], idx: Int, w: Long): Array[Long] = {
var len: Int = elems.length;
while$9(){
@@ -158865,7 +158865,7 @@
()
};
final private[this] val UndeterminedEnd: Int(2147483647) = 2147483647;
- final <stable> <accessor> def UndeterminedEnd: Int = 2147483647;
+ final <stable> <accessor> def UndeterminedEnd: Int(2147483647) = PagedSeq.this.UndeterminedEnd;
def fromIterator[T](source: Iterator[T])(implicit evidence$1: scala.reflect.ClassTag[T]): scala.collection.immutable.PagedSeq[T] = new scala.collection.immutable.PagedSeq[T](((data: Array[T], start: Int, len: Int) => {
var i: Int = 0;
while$1(){
@@ -159025,7 +159025,7 @@
()
}; |
|
That's no surprise, it just means that ConstantType also was depending on finalResultType following underlying. I don't know what phase that printout is from, but those types are widened explicitly when the time comes. Was there some effect beyond creating a diff? |
|
I didn't find any problems, I was just hunting for surprises. The body of the accessor is also changed to refer to the field. |
|
Right, that is indeed constant folding not happening. |
|
As a general reminder here about where the bug is, here's the declared meaning of finalResultType: Thus there are at least two bugs per visible behavioral change here: 1) the wrong thing was being returned for finalResultType and 2) the rest of the implementation depends on the wrong thing being returned. I can easily make the constant issue go away by putting ConstantType back in the category of types with bugs 1) and 2). I'll take a very quick look to see if I can do better. |
An optimist, until the end! |
| * depends on finalResultType doing more than what it's supposed | ||
| * to do, so it's hard to fix that method in-place. | ||
| */ | ||
| def dropPolyAndMethodTypes(tp: Type): Type = tp match { |
There was a problem hiding this comment.
Could you please leave a breadcrumb over at finalResultType?
|
I'm going to bed, but it's looking like I could probably fix finalResultType directly after tweaking one or two more call sites. |
The implementation had come to depend on finalResultType accidentally doing things beyond its charter - in particular, widening types. After hunting down and fixing the call sites depending on the bugs, I was able to rewrite the method to do only what it's supposed to do. I threw in a different way of writing it entirely to suggest how some correctness might be obtained in the future. It's a lot harder for a method written like this to break.
|
Look at all the output which magically came to life in compiler-asSeenFrom. That's the extended payoff right there. "The long con" as they say. New combinations in the output means fewer types disappearing into .distinct and is almost certainly good news. Any number of mysteries may be on their way into remission. |
|
How's "BUILD SUCCESSFUL" @retronym? STILL DIZZY? |
|
The vertigo is receding. But I'd like to give the final result (ha!) a run through another tree diffs tomorrow. |
|
There's one more little touch about to show up in the TreeDSL branch. Widening the outer accessor. |
|
I met "moscal" while creating those same diffs. From what I remember, there is still a diff and it is that constant types exist for longer, but no longer interfering with constant folding. |
|
Moscal monopolized my attention. Should get a chance to review the diffs tomorrow. Sorry about the delay. |
|
Reviewed and merged as part of #2931 |
Otherwise we get unintended widening if a method's result is
a singleton type.