Attempt to fix a potentially missing case in TypeComparer

[abgruszecki]

This is the potentially missing case that I described in #5736 (comment).

[abgruszecki]

@smarter to clarify, this is the case I was talking about. To see why it's necessary, consider that if tp1, tp2 are TypeParamRefs, constraint already contains tp1 <: tp2 and we're not in frozen constraint, we will try to record that tp1 <: tp2, do nothing because it's already recorded, and return true. However, if the constraint is frozen, we won't try to record anything and ultimately (I believe) we will return false.

Since I'm not that familiar with type inference, I have no idea what kind of code would require this and cannot test it.

[odersky]

It would be good to have a test case that fails if the case is missing! Failing that, one can try to instrument the code to see whether the new logic makes a difference anywhere in the existing code base.

[abgruszecki]

@odersky the following code fails without this case:

val a = newTypeVar(TypeBounds.empty)
val b = newTypeVar(TypeBounds.empty)
a <:< b
assert(a frozen_<:< b)

I think that seems potentially problematic. I believe I actually ran across this issue half a year ago, when I was trying to make my exhaustivity checker use Constraint as a constraint solver. That being said, I have 0 idea on how to make TypeComparer enter this case w/o manually triggering it.

EDIT: for clarity, I meant to hand off this PR to @smarter. I think it's useful to know that a potential problem exists and that we already have a fix for it, even if it doesn't impact the codebase yet. It seems to me that if (a <:< b) then a frozen_<:< b is a useful invariant to have.

[abgruszecki]

@odersky can I get a re-review on this? I checked if this code actually impacts the codebase and the answer is it does: see all occurrences of "!!! result affected !!!" in http://dotty-ci.epfl.ch/lampepfl/dotty/11090/4. The problem is that all those occurences that I've checked (and there are 34) seemingly make comparing a type parameter with itself slightly quicker:

            ==> isSubType A <:< A ?
              ==> isSubType A <:< A ?
                ==> isSubType A <:< A ?
                  ==> isSubType Any <:< A ?
                    ==> isSubType Any <:< Int ?
                    <== isSubType Any <:< Int  = false
                  <== isSubType Any <:< A  = false
                  ==> isSubType A <:< Int ?
                    ==> isSubType Any <:< Int ?
                    <== isSubType Any <:< Int  = false
                  <== isSubType A <:< Int  = false
                <== isSubType A <:< A  = true // would otherwise be false
              <== isSubType A <:< A  = true
            <== isSubType A <:< A  = true

Note that if not for this change, the entire operation would still return true, because we'd try to constrain A <: A and then ConstraintHandler would simply return true. Since I couldn't find a proper test case, I added a unit test.

To reiterate from our Monday meeting: I still think we should merge this PR - not having this case can result in very confusing results for someone trying to use frozen_<:< manually, little additional code is necessary and the invariant that this case satisfies (if t <:< u, then also t frozen_<:< u) is a very reasonable one.

[odersky]

Thanks for getting to the bottom of this. I agree it's better to add the code to avoid surprises.