Fix isNullable when <:< term refs.
#1711
Conversation
|
/rebuild |
| ctx.warning(ex"comparing values of types ${lhs.widenDealias} and ${rhs.widenDealias} using `!=' will always yield true", | ||
| tree.pos) | ||
| } | ||
| qual.select(defn.Any_==).appliedToArgs(tree.args).select(defn.Boolean_!) |
There was a problem hiding this comment.
Why put that test here? And why is it done only on != but not on ==?
There was a problem hiding this comment.
Where in the pipeline would be the best place to put this test?
There was a problem hiding this comment.
I actually thing the test is wrong. Consider:
trait A
trait B
class C extends A with B
val c = new C
val x: A = c
val y: B = c
x != y
Multiversal equality is meant to deal with these kinds of things. I am not sure we need a separate mechanism.
There was a problem hiding this comment.
Yes, the tests is incomplete, in scalac there is a different waring for those cases:
scala> x != y
<console>:18: warning: A and B are unrelated: they will most likely always compare unequal
x != y
Now I'm convinced that these should not be a separate mechanism from multiversal equality.
There is also the question of the NullType <:< X tests failing for Xs that are term refs to some subtype of AnyRef. In all the currently tests those term refs are widen first and therefore we never have false negatives. Is this the correct way to handle those cases or the change to isNullabe should be done?
There was a problem hiding this comment.
I think the isNullable change is good. TermRefs should be nullable if their underlying typrs are nullable. So I propose to just keep this change and drop the rest.
Note that without the fix console tests will fail and without the warning there is no way to test the fix.
nicolasstucki
force-pushed
the
fix-isNullable
branch
from
634cb3d to
f68058c
Compare
nicolasstucki
changed the title
<:< term refs.
|
LGTM 👍 |
Note that without the fix console tests will fail and without
the warning there is no way to test the fix.