Skip to content

Regression in typelevel/spotted-leopards for extension methods #19950

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

Closed
WojciechMazur opened this issue Mar 15, 2024 · 0 comments · Fixed by #19954
Closed

Regression in typelevel/spotted-leopards for extension methods #19950

WojciechMazur opened this issue Mar 15, 2024 · 0 comments · Fixed by #19954
Assignees
Labels
area:implicits related to implicits area:typer itype:bug regression This worked in a previous version but doesn't anymore
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Compiler version

Last good release: 3.3.2-RC1-bin-20230623-171849f-NIGHTLY
First bad release: 3.3.2-RC1-bin-20230624-be42772-NIGHTLY
Bisect points to 9ae1598

Minimized code

trait Apply[F[_]]:
  extension [T <: NonEmptyTuple](tuple: T)(using toMap: Tuple.IsMappedBy[F][T])
    def mapN[B](f: Tuple.InverseMap[T, F] => B): F[B] = ???

given Apply[Option] = ???
given Apply[List] = ???
given Apply[util.Try] = ???

@main def Repro = (Option(1), Option(2), Option(3)).mapN(_ + _ + _)

Output

-- [E007] Type Mismatch Error: /Users/wmazur/projects/scala3/bisect/main.scala:10:18 
10 |@main def Repro = (Option(1), Option(2), Option(3)).mapN(_ + _ + _)
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |Found:    (Option[Int], Option[Int], Option[Int])
   |Required: ?{ mapN: ? }
   |Note that implicit extension methods cannot be applied because they are ambiguous;
   |both given instance given_Apply_Option and given instance given_Apply_List provide an extension method `mapN` on (Option[Int], Option[Int], Option[Int])
   |

Expectation

Should compile

@WojciechMazur WojciechMazur added itype:bug area:typer area:implicits related to implicits regression This worked in a previous version but doesn't anymore labels Mar 15, 2024
odersky added a commit that referenced this issue Mar 27, 2024
Match type reduction can fail for any of the following reasons:
- EmptyScrutinee: would be unsound to reduce
- Stuck: selector does not match a case and is not provably disjoint
from it either
- NoInstance: selector does not uniquely determine params captures in
pattern
- NoMatches: selector matches none of the cases
- LegacyPattern: match type contains an illegal case and sourceVersion
>= 3.4

Out of those, only Stuck and NoInstance, *could* get reduced in a
refined context.

## Status quo

The match reducer returns:
- `ErrorType` for NoMatches and LegacyPattern,
- `NoType`, which implies the match type is left unreduced, in all other
cases.

In addition, the implementation has an issue where the `ErrorType`s can
be left unreported, then entering the flexible type logic, thereby
conforming to anything.

## Proposed changes

In addition to fixing the aforementioned bug, this PR proposes to leave
all unreducible match types as unreduced.
Of course the reduction may be needed at a later point for conformance,
in which case the error message will still contain the same explanations
from the `MatchTypeTrace`.

Fixes #19949 
Fixes #19950 

## Discussion

All cases of failed match type reductions which we know will never
reduce, even with refined scrutinee, should have a consistent behaviour.
So NoMatches and EmptyScrutinee should either both be an error or both
be left unreduced.

The current implementation attempts to do the former approach (but only
for NoMatches), which has some limitations as discussed below (I'm not
saying I can do better, hence the latter approach).

### Undesirable errors

We dont always want an error for a NoMatches failed reduction, for
example if we just need `Nothing` to conform to it:
```scala 3
trait TupleWrap[T <: Tuple]:  
  def head: Tuple.Head[T]  
  
object EmptyTupleWrap extends TupleWrap[EmptyTuple]:  
  def head = throw NoSuchElementException() // Error:
// |      ^  
// |      Match type reduction failed since selector EmptyTuple  
// |      matches none of the cases
```
But we could do `def head: Nothing = ...` to avoid the error here. 

Generally speaking, places where the bounds of the match type suffice
can still get a reduction error, and adding an ascription to avoid an
inferred match type doesn't always do the trick.

Another refused example could be:
```scala 3
type Default[N <: Int] = N match  
  case 0 => 'a' | 'c'
  case 1 => 'b' | 'd'
  
def default(n: Int): Option[Default[n.type]] = n match  
  case _: (0 | 1) => Some[Default[n.type]]:  
    n match  
      case _: 0 => 'a' 
      case _: 1 => 'b'
  case _ => None  
  
default(2): Option[Char] // Error  
// |   ^  
// |   Match type reduction failed since selector (2 : Int)  
// |   matches none of the cases
```
even though the function looks reasonable and type-checking would be
sound.

### Missed errors

Also note in the `EmptyTupleWrap` example, we get a reduction error from
a match type application which does not appear in the source code. A
valid question might be when and for what exactly these conditions are
checked ?

The goal is to report a type error early on for a NoMatches application
right, but we are actually only doing so if we happen to do
`tryNormalize` and end up in the `MatchReducer`.

Here is an example where were a match type with NoMatches is accepted
```scala 3
trait A:
  type X
  type R = X match
    case 0 => 'a'
    case 1 => 'b'

trait B extends A:
  type S = 2

type R1 = B#R // no error
```

Generally speaking, the NoMatches error can be circumvented with:
```scala 3
type AllowNoMatchesM[X] = {
  type X1 = X
  type R = X1 match
    case 0 => 'a'
    case 1 => 'b'
}#R
type R2 = AllowNoMatchesM[2] // no error
```

Also note the projections are used in the examples for simplicity but
are not necessary, `R` *can be* used within `B` as unreduced without a
reported error.

See #19799 for another example of inconsistent errors
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment