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

spec requires type checking of for-comprehension to consider refutability #1336

Open
scabug opened this issue Sep 13, 2008 · 13 comments
Open

spec requires type checking of for-comprehension to consider refutability #1336

scabug opened this issue Sep 13, 2008 · 13 comments
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Sep 13, 2008

The following only compiles with the 'filter' method, however it is never called:

  object Foo
  {
    def foreach( f : ((Int,Int)) => Unit ) 
    {
      println("foreach")
      f(1,2)
    }
//    def filter ( f : ((Int,Int)) => Boolean ) =
//    {
//      println("filter")
//      this
//    }
  
    for( (a,b) <- this )
    {
      println((a,b))
    }  
  }
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 13, 2008

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 13, 2008

Geoffrey Alan Washburn (washburn) said:
Firstly, if you read the specification the first thing that happens during desugaring of pattern matching is that,

for ( (a,b) <- this )

becomes

for ( (a,b) <- this.filter( case (a,b) => true; case _ => false } )

And if you indeed look at the AST following typechecking, this is indeed the case. However, sometime during the superaccessors or refchecks phase, filter is eliminated. Therefore, this seems to be a bug, but only because something is being too aggressive about optimizing out the call to filter, which is the identity other than being side-effecting.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 15, 2008

@TiarkRompf said:
Output of -Xprint:superaccessors still show filter call, while -Xprint:refchecks does not. So the optimization apparently happens there.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 15, 2008

@michael-nischt said:
Actually, I do understand the Spec differently (might be misunderstanding it though):

The translation scheme is as follows. In a first step, every generator p <- e, where
p is not irrefutable (�8.1) for the type of e is replaced by
p <- e .filter { case p => true; case _ => false }

A pattern p is irrefutable for a type T, if one of the following applies:

3. p is a constructor pattern c(p 1 , . . . , p n ), the type T is an instance of class c, the primary constructor (�5.3) of type T has argument types T1 , . . . , Tn , and p i is irrefutable for Ti .

Isn't Tuple2 irrefutable, but filter is only used for non-irrefutable types?

Everything else I would consider a performance bug as the following would copy the array by calling its filter method:

val pairs : Array[(Int,Int)] = ...

for((first,second) <- pairs) printf("First: %4d\t Second: %4d\n", first, second)
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 29, 2009

@paulp said:
I have read the spec and looked at the code and I assess the situation the same as gestalt. The generator "for ((a,b) <- this)" is irrefutable and the fact that filter is not called is dictated by the spec. The bug is that ((a,b)) is not recognized by the desugaring process as irrefutable, and so a call to filter is inserted. The filter call is then removed during refchecks, but if the method isn't there compilation fails before it gets around to removing it.

It's not obvious to me what is the best way to fix it, because the desugaring is done so early I don't think you can tell yet if a pattern other than a variable pattern is irrefutable.

1 similar comment
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 23, 2009

@paulp said:
I have read the spec and looked at the code and I assess the situation the same as gestalt. The generator "for ((a,b) <- this)" is irrefutable and the fact that filter is not called is dictated by the spec. The bug is that ((a,b)) is not recognized by the desugaring process as irrefutable, and so a call to filter is inserted. The filter call is then removed during refchecks, but if the method isn't there compilation fails before it gets around to removing it.

It's not obvious to me what is the best way to fix it, because the desugaring is done so early I don't think you can tell yet if a pattern other than a variable pattern is irrefutable.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 23, 2009

@paulp said:
Oops, looks like my browser remembered my comment lo these many months.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Mar 20, 2012

@paulp said:
Fixed in c82ecabad6 .

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Mar 5, 2013

@retronym said:
Reopening after this revert: scala/scala#1893

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 5, 2014

@gkossakowski said:
The 2.11.2 is out so I'm rescheduling the issue for 2.11.3.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Nov 4, 2014

@retronym said:
Updating fix-by version to 2.11.5.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 25, 2015

@adriaanm said:
Either update the spec to reflect current implementation (withFilter is always emitted) or drop the call to withFilter when type checking withFilter calls that result from for-comprehension desugaring.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 18, 2015

Paul Draper (draperp) said (edited on Sep 18, 2015 7:03:43 PM UTC):
@jason, since #1893 (the revert) was not accepted, is this still an issue?
If so, what caused it to regress?

Scala 2.11.5 has been released.

Also, this should be linked to #5589 (which is perhaps a even more common occurrence of this bug).

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

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.