Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: Scala 2.11.1-RC1
    • Component/s: Misc Compiler
    • Labels:
      None
    • Environment:

      for

      Description

      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))
          }  
        }
      

        Issue Links

          Activity

          Hide
          Geoffrey Alan Washburn added a comment -

          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.

          Show
          Geoffrey Alan Washburn added a comment - 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.
          Hide
          Tiark Rompf added a comment -

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

          Show
          Tiark Rompf added a comment - Output of -Xprint:superaccessors still show filter call, while -Xprint:refchecks does not. So the optimization apparently happens there.
          Hide
          Michael Nischt added a comment -

          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)
          
          Show
          Michael Nischt added a comment - 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)
          Hide
          Paul Phillips added a comment -

          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.

          Show
          Paul Phillips added a comment - 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.
          Hide
          Paul Phillips added a comment -

          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.

          Show
          Paul Phillips added a comment - 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.
          Hide
          Paul Phillips added a comment -

          Oops, looks like my browser remembered my comment lo these many months.

          Show
          Paul Phillips added a comment - Oops, looks like my browser remembered my comment lo these many months.
          Hide
          Paul Phillips added a comment -

          Fixed in c82ecabad6 .

          Show
          Paul Phillips added a comment - Fixed in c82ecabad6 .
          Hide
          Jason Zaugg added a comment -

          Reopening after this revert: https://github.com/scala/scala/pull/1893

          Show
          Jason Zaugg added a comment - Reopening after this revert: https://github.com/scala/scala/pull/1893

            People

            • Assignee:
              Unassigned
              Reporter:
              Michael Nischt
              TracCC:
              Johannes Rudolph, Paul Phillips, Seth Tisue
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:

                Development