Fix comment in isAsSpecificValue type and add test #4329
Conversation
Fix comment in isAsSpecificValue to match the code, drop a probably spurious case from the code which referred to the old encoding of applied types in terms of refinements, and add a test that exercises some of the behavior.
| else { | ||
| val flip = new TypeMap { | ||
| def apply(t: Type) = t match { | ||
| case t: TypeBounds => t |
There was a problem hiding this comment.
Is this PR just about changing comments or also about changing behavior?
There was a problem hiding this comment.
Is the dropping of "top-level" intentional? I'm not sure I see that change reflected in the code.
There was a problem hiding this comment.
@smarter @milessabin The PR essentially adapts the comment to what the code now does. Dropping the TypeBounds case should have no effect except for wildcard arguments. When we still used refinements for type parameters it did have the effect of restricting to top-level. But with the change to direct parameter encoding that effect was lost, so that change caused a change in behavior.
I am actually quite unsure what the right behavior would be. Toplevel or not? What does toplevel mean, precisely? I don't have a good testcase that would demonstrate why restricting to toplevel is important. So I opted for the solution that's simpler to specify.
smarter
left a comment
smarter
left a comment
There was a problem hiding this comment.
Dropping the top-level restriction seems more intuitive to me, so LGTM, but @milessabin implementation will need to be updated to conform to this.
They should be sufficient to pin down the behavior.
4 participants
Fix comment in isAsSpecificValue to match the code, drop a probably spurious case from
the code which referred to the old encoding of applied types in terms of refinements,
and add a test that exercises some of the behavior.