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

Another for-comprehension refutability regression #6968

Closed
scabug opened this issue Jan 13, 2013 · 11 comments
Closed

Another for-comprehension refutability regression #6968

scabug opened this issue Jan 13, 2013 · 11 comments

Comments

@scabug
Copy link

@scabug scabug commented Jan 13, 2013

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.

@scabug
Copy link
Author

@scabug scabug commented Jan 13, 2013

Imported From: https://issues.scala-lang.org/browse/SI-6968?orig=1
Reporter: @retronym
Affected Versions: 2.10.0
See #6646

@scabug
Copy link
Author

@scabug scabug commented Jan 13, 2013

@retronym said:
I see no alternative than to revert the change to the parser that considers patterns consisting of Tuples as irrefutable.

@scabug
Copy link
Author

@scabug scabug commented Jan 13, 2013

@scabug
Copy link
Author

@scabug scabug commented Jan 17, 2013

@scabug
Copy link
Author

@scabug scabug commented Jan 21, 2013

@retronym said:
There is another problem: the refutability checking in RefChecks, albeit somewhat ineffectual and now sometimes obviated by the (slightly overeager) checking in the parser, no longer works at all.

It suffers from two problems:

     case Apply(
        Select(qual, nme.filter | nme.withFilter),
        List(Function(
          List(ValDef(_, pname, tpt, _)),
          Match(_, CaseDef(pat1, _, _) :: _))))
        if ((pname startsWith nme.CHECK_IF_REFUTABLE_STRING) &&
            isIrrefutable(pat1, tpt.tpe) && (qual.tpe <:< tree.tpe)) =>

          transform(qual)

It doesn't handle TypeApply, the sort of problem that is now easily solved with treeInfo.Applied.
match nodes are eliminated by patmat under the new pattern matcher.
I've got a fix coming that uses the results of pattern matcher unreachability analysis (re-run with different assumptions about nullability) to make this work again.

@scabug
Copy link
Author

@scabug scabug commented Jan 21, 2013

@retronym said:
Take two: scala/scala#1945

@scabug
Copy link
Author

@scabug scabug commented Jan 23, 2013

@retronym said:
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 }

@scabug
Copy link
Author

@scabug scabug commented Jan 28, 2013

@paulp said:
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)

@scabug
Copy link
Author

@scabug scabug commented Apr 19, 2013

@odersky said:
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.

@scabug
Copy link
Author

@scabug scabug commented Apr 22, 2013

@adriaanm said:
Reopening to take Martin's input into account.

@scabug
Copy link
Author

@scabug scabug commented Nov 21, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants