Scala Programming Language
  1. Scala Programming Language
  2. SI-6968

Another for-comprehension refutability regression

    Details

      Description

      object Test {
        def main(args: Array[String]) {
          val mixedList = List(1,(1,2),4,(3,1),(5,4),6)
          for((a,b) <- mixedList) yield a
        }
      }
      

      No call to withFilter is generated.

        Issue Links

          Activity

          Hide
          Jason Zaugg added a comment -

          A few notes from discussion with Martin:

          • the introduction of withFilter really seems to limit the usefuless of refutability checking
          • we don't really want to go down the path of typechecking both variations pending analysis of the patterns.
          • perhaps, though, we could re-spec things to say the withFilter is always included, but, the compiler
            will pass a specially tagged closure (e.g. it would have a marker trait `Constant` as a a parent),
            so that the withFilter implementation could avoid calling it.
          class Option[+A] { self =>
            class WithFilter(p: A => Boolean) {
              private val isConstant = p.isInstanceOf[ConstantFunction]
              def map[B](f: A => B): Option[B] = if (isConstant) self map f else self filter p map f
              ...
            }
          }
          
          • Treatment of `null` causes headaches: we ignore the possibility of null in the refutability checking (otherwise any pattern involving a type test would be refutable). Perhaps pattern generated should be changed:
            for ((x, y) <- null: Option[(Int, Int)]) yield x
          
            scala> val n = Some(null): Option[(Int, Int)]
            n: Option[(Int, Int)] = Some(null)
          
            scala> for (Tuple2(x, y) <- n) yield x
            scala.MatchError: null
          	at $anonfun$1.apply(<console>:9)
          	at $anonfun$1.apply(<console>:9)
          	at scala.Option.map(Option.scala:145)
          
            scala> n withFilter { case (x, y) => true; case _ => false } map { case (x, y) => x }
            res4: Option[Int] = None
          
          
            // if refutability checking worked as originally intended, and the `withFilter` was elided.
            // will throw a NPE
            scala> n map { case (x, y) => x }
            scala.MatchError: null
          
            // Tentative proposal: the predicate should admit null.
            scala> n withFilter { case (x, y) => true; case null =>; case _ => false } map { case (x, y) => x }
          
          Show
          Jason Zaugg added a comment - A few notes from discussion with Martin: the introduction of withFilter really seems to limit the usefuless of refutability checking we don't really want to go down the path of typechecking both variations pending analysis of the patterns. perhaps, though, we could re-spec things to say the withFilter is always included, but, the compiler will pass a specially tagged closure (e.g. it would have a marker trait `Constant` as a a parent), so that the withFilter implementation could avoid calling it. class Option[+A] { self => class WithFilter(p: A => Boolean) { private val isConstant = p.isInstanceOf[ConstantFunction] def map[B](f: A => B): Option[B] = if (isConstant) self map f else self filter p map f ... } } Treatment of `null` causes headaches: we ignore the possibility of null in the refutability checking (otherwise any pattern involving a type test would be refutable). Perhaps pattern generated should be changed: for ((x, y) <- null: Option[(Int, Int)]) yield x scala> val n = Some(null): Option[(Int, Int)] n: Option[(Int, Int)] = Some(null) scala> for (Tuple2(x, y) <- n) yield x scala.MatchError: null at $anonfun$1.apply(<console>:9) at $anonfun$1.apply(<console>:9) at scala.Option.map(Option.scala:145) scala> n withFilter { case (x, y) => true; case _ => false } map { case (x, y) => x } res4: Option[Int] = None // if refutability checking worked as originally intended, and the `withFilter` was elided. // will throw a NPE scala> n map { case (x, y) => x } scala.MatchError: null // Tentative proposal: the predicate should admit null. scala> n withFilter { case (x, y) => true; case null =>; case _ => false } map { case (x, y) => x }
          Hide
          Paul Phillips added a comment -

          Speaking of "val isConstant" take a look at: https://github.com/paulp/scala/tree/wip/predicates

          At the time I was pursuing unnecessary allocations of Function1 (especially for _ => true and _ => false) but there are more uses, as you see.

          final class Predicate[@specialized T](val f: T => Boolean, val isFlipped: Boolean) extends (T => Boolean) 
          ...
          
            fun match {
              // Rewrite _ => true and _ => false to reuse preallocated constant predicates
              case Function(vparam :: Nil, Literal(Constant(v: Boolean))) =>
                enterSym(context, vparam)
                if (context.retyping) context.scope enter vparam.symbol
                val formal    = typedValDef(vparam).symbol.tpe
                val funtpe    = appliedType(clazz, formal, BooleanClass.tpe)
                val predicate = termMember(FunctionModule, "Predicate" + v.toString.capitalize)
          
                typed(gen.mkNullaryCall(predicate, formal :: Nil), funtpe)
          
          Show
          Paul Phillips added a comment - Speaking of "val isConstant" take a look at: https://github.com/paulp/scala/tree/wip/predicates At the time I was pursuing unnecessary allocations of Function1 (especially for _ => true and _ => false) but there are more uses, as you see. final class Predicate[@specialized T](val f: T => Boolean, val isFlipped: Boolean) extends (T => Boolean) ... fun match { // Rewrite _ => true and _ => false to reuse preallocated constant predicates case Function(vparam :: Nil, Literal(Constant(v: Boolean))) => enterSym(context, vparam) if (context.retyping) context.scope enter vparam.symbol val formal = typedValDef(vparam).symbol.tpe val funtpe = appliedType(clazz, formal, BooleanClass.tpe) val predicate = termMember(FunctionModule, "Predicate" + v.toString.capitalize) typed(gen.mkNullaryCall(predicate, formal :: Nil), funtpe)
          Hide
          Martin Odersky added a comment -

          I agree that parser checking is useless and has to go. We could mark the closure as Jason suggests, but I would loath the fact that then

          for (x <- xs) yield x * x

          requires more code than

          xs map (x => x * x)

          An alternative would be to embed redundancy checking and elimination in the pattern matcher. Also agreed that null needs to be speced to be part of the filter.

          Show
          Martin Odersky added a comment - I agree that parser checking is useless and has to go. We could mark the closure as Jason suggests, but I would loath the fact that then for (x <- xs) yield x * x requires more code than xs map (x => x * x) An alternative would be to embed redundancy checking and elimination in the pattern matcher. Also agreed that null needs to be speced to be part of the filter.
          Hide
          Adriaan Moors added a comment -

          Reopening to take Martin's input into account.

          Show
          Adriaan Moors added a comment - Reopening to take Martin's input into account.
          Hide
          Jason Zaugg added a comment -

          re-closing this one as this ticket was about the refutability regression, not about the inefficiency of irrefutable filters when matching on tuples.

          Show
          Jason Zaugg added a comment - re-closing this one as this ticket was about the refutability regression, not about the inefficiency of irrefutable filters when matching on tuples.

            People

            • Assignee:
              Jason Zaugg
              Reporter:
              Jason Zaugg
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development