Fixes to Type#isRef #1593
Fixes to Type#isRef #1593
Conversation
Stripping refinements makes sense when checking if a type is a reference to a parameterized type like Option, but this shouldn't be the default, otherwise refinements to Any may be discarded by methods like lub and glb as illustrated in the tests.
Otherwise any applied higher-kinded type upper-bounded by Any is considered to be a reference to Any and will be mishandled by methods like lub and glb, see the tests.
|
Alright, the tests pass now :) |
| case this1: HKApply => this1.superType.isRef(sym) | ||
| case this1: RefinedType if stripRefinements => this1.parent.isRef(sym, stripRefinements) | ||
| case this1: RecType => this1.parent.isRef(sym, stripRefinements) | ||
| case this1: HKApply => this1.superType.isRef(sym, stripRefinements) && this1.lowerBound.isRef(sym, stripRefinements) |
There was a problem hiding this comment.
If stripRefinements == false, it seems we should also stop at RecType and HKApply since they are conceptually analogous to RefinedType.
There was a problem hiding this comment.
I tend to think it would be better to split the method in two. isRef and isRefInstance (?). isRefInstance could presumably just be this:
def isRefInstance(sym: Symbol)(implicit ctx: Context): Boolean = dealias match {
case this1: TypeRef => this1.symbol eq sym
case this1: RefinedOrRecType => this1.parent.isRef(sym)
case this1: HKApply => this1.superType.isRef(sym) && this1.lowerBound.isRef(sym)
case _ => false
}
There was a problem hiding this comment.
And isRef would just be the remaining cases. That seems simpler logic and avoids the boolean arguments in the calls.
There was a problem hiding this comment.
Having two methods would be nice yes, I avoided that so that cases like refined aliases of refined aliases of refined instances work, but that might not be important. An additional difficulty is that we cannot use dealias because of Namer#typeDefSig. Another option would be to have:
private[this] def isRef(sym: Symbol, stripRefinements: Boolean = false)(...) = ...
def isRef(sym: Symbol) = isRef(sym)
def isAppliedRef(sym: Symbol) = isRef(sym, true)There was a problem hiding this comment.
I think the comment in Namer applies only to direct isRefs not to isRefInstance. Even if I am wrong, I claim the logic is still simpler splitting the method in two. I don't believe even an internal boolean is worth it.
|
@smarter - please update :) |
|
@smarter I tried the two test cases in this PR on latest master and they pass. Should I open a new PR with the two test cases and close this one? |
|
@smarter apart for these two test cases do you have another way to reproduce it? |
Add regression tests for #1593
|
This is the sort of thing I had in mind for |
|
@smarter What to do with this? Do we still need this? |
|
It'd have to be redone from scratch anyway so we can close this one. |
Review by @odersky