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

GADT patmat doesn't narrow class type parameters #4323

Closed
abgruszecki opened this issue Apr 16, 2018 · 9 comments
Closed

GADT patmat doesn't narrow class type parameters #4323

abgruszecki opened this issue Apr 16, 2018 · 9 comments

Comments

@abgruszecki
Copy link
Contributor

scala> enum Expr[T] {
           case IExpr(value: Int) extends Expr[Int]
           case BExpr(value: Boolean) extends Expr[Boolean]

           def join(other: Expr[T]): Expr[T] = (this, other) match {
             case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
             case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
           }
         }
6 |      case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
  |                                     ^^^^^^^^^^^^^^
  |                                     found:    Expr[Int]
  |                                     required: Expr[T]
  |
7 |      case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
  |                                     ^^^^^^^^^^^^^^
  |                                     found:    Expr[Boolean]
  |                                     required: Expr[T]
  |

This is similar to an old issue where class/trait parameters aren't narrowed.

@allanrenucci
Copy link
Contributor

The issue is not specific to enums:

sealed trait Expr[T] {
  import Expr._

   def join(other: Expr[T]): Expr[T] = (this, other) match {
     case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
     case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
   }
}

object Expr {
  case class IExpr(value: Int) extends Expr[Int]
  case class BExpr(value: Boolean) extends Expr[Boolean]
}
15 |     case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
   |                                    ^^^^^^^^^^^^^^
   |                                    found:    Expr.IExpr
   |                                    required: Expr'[T]
   |                                    
   |                                    where:    Expr  is a object
   |                                              Expr' is a trait
16 |     case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
   |                                    ^^^^^^^^^^^^^^
   |                                    found:    Expr.BExpr
   |                                    required: Expr'[T]
   |                                    
   |                                    where:    Expr  is a object
   |                                              Expr' is a trait

@Blaisorblade
Copy link
Contributor

It seems method params aren't refined, but trait params are. Using this isn't necessary to trigger the error.

For instance, works:

trait Expr[T]
case class IExpr(value: Int) extends Expr[Int]
case class BExpr(value: Boolean) extends Expr[Boolean]
object Foo {
  def foo[T] = {
    def join(one: Expr[T], other: Expr[T]): Expr[T] = (one, other) match {
      case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
      case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
    }
  }
}

Fails:

object Foo {
  sealed trait Expr[T] {
    import Expr._

    def join(one: Expr[T], other: Expr[T]): Expr[T] = (one, other) match {
      case (IExpr(i1), IExpr(i2)) => IExpr(i1 + i2)
      case (BExpr(b1), BExpr(b2)) => BExpr(b1 & b2)
    }
  }

  object Expr {
    case class IExpr(value: Int) extends Expr[Int]
    case class BExpr(value: Boolean) extends Expr[Boolean]
  }
}

Writing Expr' in the error seems a (separate?) bug.

@smarter
Copy link
Member

smarter commented Apr 16, 2018

Writing Expr' in the error seems a (separate?) bug.

Quotes after type names are used in error messages to distinguish between several types with the same name (we could also use colors or Unicode subscripts if supported by the terminal, but quotes work everywhere)

@abgruszecki
Copy link
Contributor Author

@Blaisorblade AFAIU, the first example works because the T used in def join(one: Expr[T], other: Expr[T]): Expr[T] is the one from def foo[T] - it's a method param. In contrast, in the second example the T is the one from sealed trait Expr[T] - it's a trait param.

@Blaisorblade
Copy link
Contributor

Quotes after type names are used in error messages to distinguish between several types with the same name (we could also use colors or Unicode subscripts if supported by the terminal, but quotes work everywhere)

I know, and I'm fine with that for T and T', but the trait here is really called Expr not Expr'.

@AleksanderBG Yes, IIUC, that's what I meant with

It seems method params aren't refined, but trait params are.

@smarter
Copy link
Member

smarter commented Apr 17, 2018

I know, and I'm fine with that for T and T', but the trait here is really called Expr not Expr'.

So how do you propose to distinguish between the object Expr and the trait Expr in error messages?

@LPTK
Copy link
Contributor

LPTK commented Apr 20, 2018

So how do you propose to distinguish between the object Expr and the trait Expr in error messages?

Why would the compiler's error messages not follow the Scala fact that a value and a type sharing the same name are not ambiguous? (They live in different namespaces.) At the very least it should not have to introduce a difference between the name of a class and its companion object.

I propose:

   |                                    where: value Expr is an object
   |                                            type Expr is a trait

@liufengyun liufengyun self-assigned this Apr 20, 2018
liufengyun added a commit to dotty-staging/dotty that referenced this issue Apr 20, 2018
Previously, GADTMap only accepts type bounds, it means we need
to force type parameters in order to get their bounds.
However forcing type parameters creates a cycle:

     constr -------->  bodyIndex
      /\                   |
       |                   |
       |                   |
       |                   |
       |                   |
       |                   |
     type params -----------

We change GADTMap to accept `TermRef` to avoid forcing type params,
which breaks the cycle.
liufengyun added a commit to dotty-staging/dotty that referenced this issue Apr 20, 2018
liufengyun added a commit to dotty-staging/dotty that referenced this issue Apr 20, 2018
@Blaisorblade
Copy link
Contributor

I had a problem with this because Expr' isn't something that exists, and it even looks like actual syntax (which it isn't), and I'm fine with alpha-renaming variables but classes aren't. I think @LPTK's idea is good here, but for cases where it doesn't work I'd maybe use Expr@1 and Expr@2 and write "where Expr@1 is type Expr in ... and Expr@2 is value Expr in ...". This phrasing would arguably be already better now.

Anyway, I opened #4355 for this.

@abgruszecki
Copy link
Contributor Author

Well, this was actually implemented in #11222, we just forgot about this particular issue. Closing now.

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

No branches or pull requests

6 participants