Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine criterion when to widen types #17180

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 29, 2023

Refine criterion when to widen types in match type reduction.

We now do not widen if the compared-against type contains variant named type parameters. This is intended to fix the soundness problem in #17149. It also fixes the regression in #15926. @Decel was right in suggesting these two were related.

Fixes #17149
Fixes #15926

Todos:

  • Check & fix neg test failures
  • Add more tests
  • Also consider approximating abstract types to lower bounds. This is completely missing so far. There are neither tests nor an implementation.
  • Update the docs on match types to explain what goes on here.

@odersky
Copy link
Contributor Author

odersky commented Mar 29, 2023

@Decel, @dwijnand Can one or both of you take this over and address the remaining points? It's a tricky area, I believe quite a bit more work will be needed to arrive at a fully satisfactory solution.

@dwijnand
Copy link
Member

dwijnand commented Mar 29, 2023

@odersky Happy to. @Decel let me know if you want to sync in a call or on the LAMP slack.

Also, what do you think of my approach in #17173? I don't quite get the mechanics of Ranges in ApproximatingTypeMap's, so I wondered if it can be cleaned up further. But generally, it solves the problem within matchCases alone. (The other pickler and tupleElementTypes changes are both Ytest-pickler related.)

@odersky
Copy link
Contributor Author

odersky commented Mar 30, 2023

I left a comment on #17173.

@odersky
Copy link
Contributor Author

odersky commented Mar 30, 2023

One thing where #17180 falls short is error diagnostics. When we do not widen, we essentially get a "Stuck" error saying scrutinee is not a subtype and is not disjoint either. But before we got a "variable cannot be uniquely instantiated" error, which is much better. After all, scrutinee is a subtype of pattern in this case, for some instantiation of type variables.

We could recover the original error diagnostics by doing the widening in widenAbstractOKFor instead of preventing it, but "poisoning" any variant type variables in the other type. Then in redux we can flag such type variable instantiations (but maybe using Range is not the best way to do this). Then we can take these flagged variables and issue a MatchResult.NoInstance message.

So my proposal would be:

  • change widenAbstractOKFor to always return canWidenAbstract but poison variant type variables in the other type.
  • in paramInstance record poisoned and undetermined type variables instead of wrapping with Range.
  • In redux produce a NoInstance error if bad type variables were recorded.

That could lead to much better error messages. And diagnostics is really important for match types, since the algorithms are so complex.

Copy link
Contributor Author

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new neg tests are effectivly less informative than the old ones. So before fixing the check files we should try to improve the diagnostics to a point where they are equivalent.

@dwijnand dwijnand marked this pull request as draft March 30, 2023 11:42
@dwijnand
Copy link
Member

I agree. I marked it draft to convey that even if it's green, it's not to be merged.

@Decel
Copy link
Contributor

Decel commented Apr 18, 2023

Also fixes #16504

@odersky
Copy link
Contributor Author

odersky commented Apr 22, 2023

@Decel @dwijnand What's the status here?

@dwijnand
Copy link
Member

Let's look at it this this coming week (24th).

@Decel
Copy link
Contributor

Decel commented Apr 27, 2023

So, we have managed to find a counterexample with @dwijnand:

trait Show[-A >: Nothing]

type MT1[I <: Show[Nothing], N] = I match
  case Show[a] => N match
    case 0     => a

val a = summon[MT1[Show[Any], 0] =:= Any]

odersky and others added 6 commits May 24, 2023 14:56
Refine criterion when to widen types in match type reduction.

We now do not widen if the compared-against type contains
variant named type parameters. This is intended to fix the
soundness problem in scala#17149.

Fixes scala#17149
Fixes scala#15926

Todos:

 - [ ] Check & fix neg test failures
 - [ ] Add more tests
 - [ ] Also consider approximating abstract types to lower bounds.
       This is completely missing so far. There are neither tests
       nor an implementation.
 - [ ] Update the docs on match types to explain what goes on here.
During match type reduction, if we're going to allow approximating
we must further restrict match types from widening to its bound.
Failing to do so will record GADT constraint bounds that parameters will
be approximated to, which is lossy and leads to a loss of type checking
completion..

For instance, in a variant of i13633 with an added `<: (Boolean,
Boolean)` bound to `PlusTri`, when reducing `PlusTri[a, b, O] match {
case (x, y) => ... }`, widening the expansion of `PlusTri[a, b, O]` to
its bound `(Boolean, Boolean)`, causes the recording the constraints
bounds `x >: Boolean` and `y >: Boolean` and then reduction instantiates
`x` and `y` to Boolean, losing the precision that comes from `a`, `b`,
and `O`.

This came up while implementing bounds checking to match type case
bodies, and requires an important fix to PatternTypeConstrainer
(the use of typeSymbol's) so the tests that require this are part of
that effort.
@dwijnand dwijnand marked this pull request as ready for review May 25, 2023 08:54
@dwijnand dwijnand assigned odersky and unassigned dwijnand and Decel May 25, 2023
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending Martin's review

@dwijnand
Copy link
Member

@odersky I can't request your review of your PR, but could you have a look and say if you're happy with this landing? Thank you.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending Martin's review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants