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.

Sign up for GitHub

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

Jump to bottom

[WIP] Match Type with no cases should not reduce to ErrorType #19953

Closed
wants to merge 4 commits into from
Closed

[WIP] Match Type with no cases should not reduce to ErrorType #19953

wants to merge 4 commits into from

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Mar 15, 2024 •
edited
Loading

When match type reduction fails because none of the cases match,
the MatchReducer reduces to an ErrorType, it does neither of:

  • throwing an internal TypeError exception, or
  • returning NoType which the TypeComparer (among many) would interpret as not reducing.

Since ErrorType extends FlexType, the returned result of the reduction will conform to anything.

Fixes #19949
Fixes #19950

@EugeneFlesselle EugeneFlesselle mentioned this pull request Mar 15, 2024
Merged
@EugeneFlesselle
Copy link
Contributor Author

EugeneFlesselle commented Mar 15, 2024 •
edited
Loading

pos/constvalue-of-failed-match-type.scala fails because the InliningTreeMap does not handle TypeErrors thrown by unreducible match types. We can of course handle that case there, but it would also be solved by #19954 if we consider it.

@sjrd
Copy link
Member

sjrd commented Mar 15, 2024

pos/constvalue-of-failed-match-type.scala fails because the InliningTreeMap does not handle TypeErrors thrown by unreducible match types. We can of course handle that case there, but it would also be solved by #19954 if we consider it.

I added this test because at least one library depended on it. I'm not sure which one off the top of my head, although I could probably dig it up. Anyway, that means we probably need to find a way to bring it back into the green.

@dwijnand
Copy link
Member

Accepting that the resulting unsoundness is bad and needs fixing, I would consider reintroducing throwing exceptions to be throwing (ironically) the baby out with the bath water (👶🏼 💙 ).

Is it not possible to spot how this is isn't being properly propagated in type select and in however its impacting spotted-leopards?

@EugeneFlesselle EugeneFlesselle mentioned this pull request Mar 16, 2024
Closed
@EugeneFlesselle
Copy link
Contributor Author

Is it not possible to spot how this is isn't being properly propagated in type select and in however its impacting spotted-leopards?

I've tried that approach in #19961 but I suspect we are missing other problematic cases.

@EugeneFlesselle EugeneFlesselle mentioned this pull request Mar 16, 2024
Closed
@EugeneFlesselle
Revert "ErrorType instead of throwing in match type "no cases""
b2c544a 
This reverts commit 9ae1598

Note that the changes in Typer:
```
val unsimplifiedType = result.tpe
simplify(result, pt, locked)
result.tpe.stripTypeVar match
  case e: ErrorType if !unsimplifiedType.isErroneous =>
    errorTree(xtree, e.msg, xtree.srcPos)
  case _ => result
```
cannot be reverted yet since the MatchReducer now also reduces to an `ErrorType` for MatchTypeLegacyPatterns, introduced after 9ae1598.
@EugeneFlesselle
Update check-files and remove i18488.scala
ed5ddfe 
i18488.scala was only passing because of the bug in the MatchReducer,
as we can see in the subtyping trace:
```
==> isSubType TableQuery[BaseCrudRepository.this.EntityTable] <:< Query[BaseCrudRepository.this.EntityTable, E[Option]]?
  ==> isSubType Query[BaseCrudRepository.this.EntityTable, Extract[BaseCrudRepository.this.EntityTable]] <:<
                Query[BaseCrudRepository.this.EntityTable, E[Option]] (left is approximated)?
    ==> isSubType E[Option] <:< Extract[BaseCrudRepository.this.EntityTable]?
      ==> isSubType [T[_$1]] =>> Any <:< Extract?
        ==> isSubType Any <:< Extract[T]?
          ==> isSubType Any <:< T match { case AbstractTable[t] => t } <: t (right is approximated)?
            ==> isSubType Any <:< <error Match type reduction failed since selector T
                                   matches none of the cases
                                   case AbstractTable[t] => t> (right is approximated)?
            <== isSubType Any <:< <error Match type reduction failed since selector T
                                   matches none of the cases
                                   case AbstractTable[t] => t> (right is approximated) = true
          <== isSubType Any <:< T match { case AbstractTable[t] => t } <: t (right is approximated) = true
        <== isSubType Any <:< Extract[T] = true
      <== isSubType [T[_$1]] =>> Any <:< Extract = true
      ...
    <== isSubType Extract[BaseCrudRepository.this.EntityTable] <:< E[Option] = true
  <== isSubType Query[BaseCrudRepository.this.EntityTable, Extract[BaseCrudRepository.this.EntityTable]] <:<
                Query[BaseCrudRepository.this.EntityTable, E[Option]] (left is approximated) = true
<== isSubType TableQuery[BaseCrudRepository.this.EntityTable] <:< Query[BaseCrudRepository.this.EntityTable, E[Option]] = true
```
@EugeneFlesselle
Only throw TypeErrors for match types with no cases if ctx.isTyper
be4065e
@EugeneFlesselle EugeneFlesselle mentioned this pull request Mar 17, 2024
Closed
@EugeneFlesselle EugeneFlesselle deleted the MatchTypeNoCases branch April 6, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@EugeneFlesselle @sjrd @dwijnand