SI-7126 Account for the alias types that don't dealias. #2167
Conversation
After this change:
qbin/scalac -Ydebug test/files/pos/t7126.scala 2>&1 | grep warning
warning: dropExistential did not progress dealiasing Test.this.T[Test.this.T], see SI-7126
one warning found
T[T]? Really? The true bug lies somewhere else; the comments of
the ticket illuminate the general areas of concern.
|
Review item: can we spot anything else in 3bf5118 that now recurses on |
|
Did somebody already take of getParts? That too must be a problem. |
|
Ah, found the other PR. |
|
Oh, "the same protection" you mean the infinite recursion. By the way , I noticed, belatedly, that said protection IS in place on AliasTypeRef, on the auto-normalizers. Personally I think we should make it true by construction that (this ne normalize) when "this" is an AliasTypeRef. |
|
The val lenientTargs = protoTypeArgs(tparams, formals, mt.resultApprox, pt)
val strictTargs = map2(lenientTargs, tparams)((targ, tparam) =>
if (targ == WildcardType) tparam.tpeHK else targ)
var remainingParams = paramTypes
def typedArgToPoly(arg: Tree, formal: Type): Tree = { //TR TODO: cleanup
val lenientPt = formal.instantiateTypeParams(tparams, lenientTargs)
val newmode =
if (isByNameParamType(remainingParams.head)) POLYmode
else POLYmode | BYVALmode
if (remainingParams.tail.nonEmpty) remainingParams = remainingParams.tail
val arg1 = typedArg(arg, forArgMode(fun, mode), newmode, lenientPt)
val argtparams = context.extractUndetparams()
if (!argtparams.isEmpty) {
val strictPt = formal.instantiateTypeParams(tparams, strictTargs)
inferArgumentInstance(arg1, argtparams, strictPt, lenientPt)
arg1
} else arg1
} |
ghost
assigned
|
My working hypothesis is that such types only show up during failed lines of inquiry in type inference. So code that is called within inference is at risk of infinite recursion (e.g. |
The kind-polymorphic nature of Nothing and Any in concert with type argument inference could lead to types like `T[T]` (where `type T=Any`). Compensatory action is taken later on to recover; see the usages of `TypeRef#typeParamsMatchArgs`. But this these types have a nasty property, they can dealias to themselves. Callers recursing through types who fail to account for this hit an infinite recursion, as was reported in SI-7126. This commit simply dealiases `T` when registering the type bound in `unifySimple`. We should try to weed out additional sources of these types.
|
Review by @adriaanm for the |
|
LGTM in isolation (I assume the test case no longer warns?), |
|
I don't think we should revert it wholesale, not at this point. I can't remember how/why it went in between 2.10.0 and 2.10.1, that feels very wrong now? In any case, I think it is likely that one or more things depends on it by now. Maybe we should just take a good hard look at it for any more places where we need to see normalized type constructors. |
|
Yep, the warning is eradicated. I second Paul's suggestion to sweep the remaining places in e5da30b where we no longer call |
Squashed commit of the following:
commit 55806cc0e6177820c12a35a18b4f2a12dc07bb39
Author: Paul Phillips <paulp@improving.org>
Date: Wed Dec 19 07:32:19 2012 -0800
SI-6846, regression in type constructor inference.
In 658ba1b some inference was gained and some was lost.
In this commit we regain what was lost and gain even more.
Dealiasing and widening should be fully handled now, as
illustrated by the test case.
(cherry picked from commit dbebcd5)
commit e6ef58447d0f4ef6de956fcc03ee283bb9028c02
Author: Paul Phillips <paulp@improving.org>
Date: Fri Dec 21 15:11:29 2012 -0800
Cleaning up type alias usage.
I determined that many if not most of the calls to .normalize
have no intent beyond dealiasing the type. In light of this I
went call site to call site knocking on doors and asking why
exactly they were calling any of
.normalize
.widen.normalize
.normalize.widen
and if I didn't like their answers they found themselves
introduced to 'dropAliasesAndSingleTypes', the recursive widener
and dealiaser which I concluded is necessary after all.
Discovered that the object called 'deAlias' actually depends
upon calling 'normalize', not 'dealias'. Decided this was
sufficient cause to rename it to 'normalizeAliases'.
Created dealiasWiden and dealiasWidenChain.
Dropped dropAliasesAndSingleTypes in favor of methods
on Type alongside dealias and widen (Type#dealiasWiden).
These should reduce the number of "hey, the type alias doesn't work" bugs.
(cherry picked from commit 3bf5118)
Conflicts:
src/compiler/scala/tools/nsc/interpreter/CompletionOutput.scala
commit c1d8803cea1523f458730103386d8e14324a9446
Author: Paul Phillips <paulp@improving.org>
Date: Sat Dec 22 08:13:48 2012 -0800
Shored up a hidden dealiasing dependency.
Like the comment says:
// This way typedNew always returns a dealiased type. This
// used to happen by accident for instantiations without type
// arguments due to ad hoc code in typedTypeConstructor, and
// annotations depended on it (to the extent that they worked,
// which they did not when given a parameterized type alias
// which dealiased to an annotation.) typedTypeConstructor
// dealiases nothing now, but it makes sense for a "new" to
// always be given a dealiased type.
PS:
Simply running the test suite is becoming more difficult all
the time. Running "ant test" includes time consuming activities
of niche interest such as all the osgi tests, but test.suite
manages to miss the continuations tests.
(cherry picked from commit 422f461)
commit da4748502792b260161baa10939554564c488051
Author: Paul Phillips <paulp@improving.org>
Date: Fri Dec 21 12:39:02 2012 -0800
Fix and simplify typedTypeConstructor.
Investigating the useful output of devWarning (-Xdev people,
it's good for you) led back to this comment:
"normalize to get rid of type aliases"
You may know that this is not all the normalizing does.
Normalizing also turns TypeRefs with unapplied arguments
(type constructors) into PolyTypes. That means that when
typedParentType would call typedTypeConstructor it would
find its parent had morphed into a PolyType. Not that it
noticed; it would blithely continue and unwittingly discard
the type arguments by way of appliedType (which smoothly
logged the incident, thank you appliedType.)
The simplification of typedTypeConstructor:
There was a whole complicated special treatment of AnyRef
here which appears to have become unnecessary. Removed special
treatment and lit a candle for regularity.
Updated lots of tests regarding newly not-so-special AnyRef.
(cherry picked from commit 394cc42)
commit 1f3c77bacb2fbb3ba9e4ad0a8a733e0f9263b234
Author: Paul Phillips <paulp@improving.org>
Date: Fri Dec 21 15:06:10 2012 -0800
Removed dead implementation.
Another "attractive nuisance" burning off time until I
realized it was commented out.
(cherry picked from commit ed40f5c)
|
I've started adding my comments to e5da30b. I'll be a short on scalac time for the rest of the week, so any help is appreciated. |
|
ok, the extra review pass over e5da30b has convinced me LGTM |
SI-7126 Account for the alias types that don't dealias.
After this change:
T[T]? Really? The true bug lies somewhere else; the comments ofthe ticket illuminate the general areas of concern.
This is a shallow fix intended for 2.10.1. I'll keep investigating the deeper issue, to help inform the decision about whether we are comfortable releasing 2.10.1 with only this bandaid.
Review by @paulp