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

partial application of methods ignores call-by-name parameters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      Consider the following:

      object Test {
        def test(x: => Unit)(y: Unit): String = "done"
      
        def main(args: Array[String]) {
          val t = test(println("never seen"))_
          println(t())
        }
      }
      

      This produces:

      never seen
      done
      

      It should produce just:

      done
      

      scalac -Xprint:typer shows that the definition of t is compiled as:

      val t: (Unit) => String = {
        <synthetic> val eta$$0$$1: Unit = scala.this.Predef.println("never seen");
        ((eta$$1$$1: Unit) => Test.this.test(eta$$0$$1)(eta$$1$$1))
      };
      

      This eta-expansion incorrectly ignores that the first parameter of test is call-by-name.

        Issue Links

          Activity

          Hide
          Martin Odersky added a comment -

          Milestone next_bugfix deleted

          Show
          Martin Odersky added a comment - Milestone next_bugfix deleted
          Hide
          Martin Odersky added a comment -

          No, that's how eta expansion is defined. You don't just wrap a lambda around the expression, you evaluate what you can first.

          Show
          Martin Odersky added a comment - No, that's how eta expansion is defined. You don't just wrap a lambda around the expression, you evaluate what you can first.
          Hide
          Matt Hellige added a comment -

          Replying to [comment:4 odersky]:
          > No, that's how eta expansion is defined. You don't just wrap a lambda around the expression, you evaluate what you can first.

          I'm sorry, but I really strongly disagree with this interpretation. I have no problem with your definition of eta expansion, but I disagree with what it means to "evaluate" a by-name parameter.

          My expectation is that the parameter should be fully evaluated to a thunk, but the thunk should not be entered unless (and until) the name is actually mentioned in the body.

          Your interpretation has a number of consequences that I find bizarre:

          1. A by-name parameter may be evaluated even if it is never mentioned in the body of the function.

          2. A by-name parameter may be evaluated only once even if it's mentioned several times (or in a loop). This makes it impossible to use the by-name parameter to define new control structures, since you cannot control when or how often the thunk is entered.

          3. The side effects of a function may differ depending on whether it is saturated in one application or after being partially applied: given the above code, test("never seen")("done") prints only "done", while

          {val f = test("never seen")_; f("done")}

          prints both. I find it hard to believe that this is desirable.

          So in short, I agree that you should "evaluate what you can," but I really feel that in this case, you are speculatively evaluating too much. It is not safe to re-order the evaluation of by-name parameters, or even to assume that they will eventually be evaluated at all.

          My concerns are not primarily theoretical. I am first and foremost worried about the above consequences. If you insist on leaving this as is, would you consider changing the language to allow by-name parameters only in the final parameter list? The current behavior is extremely confusing.

          Show
          Matt Hellige added a comment - Replying to [comment:4 odersky] : > No, that's how eta expansion is defined. You don't just wrap a lambda around the expression, you evaluate what you can first. I'm sorry, but I really strongly disagree with this interpretation. I have no problem with your definition of eta expansion, but I disagree with what it means to "evaluate" a by-name parameter. My expectation is that the parameter should be fully evaluated to a thunk, but the thunk should not be entered unless (and until) the name is actually mentioned in the body. Your interpretation has a number of consequences that I find bizarre: 1. A by-name parameter may be evaluated even if it is never mentioned in the body of the function. 2. A by-name parameter may be evaluated only once even if it's mentioned several times (or in a loop). This makes it impossible to use the by-name parameter to define new control structures, since you cannot control when or how often the thunk is entered. 3. The side effects of a function may differ depending on whether it is saturated in one application or after being partially applied: given the above code, test("never seen")("done") prints only "done", while {val f = test("never seen")_; f("done")} prints both. I find it hard to believe that this is desirable. So in short, I agree that you should "evaluate what you can," but I really feel that in this case, you are speculatively evaluating too much. It is not safe to re-order the evaluation of by-name parameters, or even to assume that they will eventually be evaluated at all. My concerns are not primarily theoretical. I am first and foremost worried about the above consequences. If you insist on leaving this as is, would you consider changing the language to allow by-name parameters only in the final parameter list? The current behavior is extremely confusing.
          Hide
          Matt Hellige added a comment -

          Here is another example, with some comments. I hope this makes my concerns clear and concrete:

          object Test {
            def repeat(x: => Unit)(times: Int) = for (_ <- 1 to times) x
          
            def test() {
              // prints foo 5 times
              repeat(println("foo"))(5)
              
              // prints foo once
              val r = repeat(println("foo")) _
              // does not print anything
              r(5)
            }
          }
          

          If you don't find this behavior at all surprising, maybe you could provide some intuition for why it is correct?

          Show
          Matt Hellige added a comment - Here is another example, with some comments. I hope this makes my concerns clear and concrete: object Test { def repeat(x: => Unit)(times: Int) = for (_ <- 1 to times) x def test() { // prints foo 5 times repeat(println("foo"))(5) // prints foo once val r = repeat(println("foo")) _ // does not print anything r(5) } } If you don't find this behavior at all surprising, maybe you could provide some intuition for why it is correct?
          Hide
          Martin Odersky added a comment -

          You'd need to propose another spec for eta-expansion then. The compiler is faithful to the spec in 6.25.5. Note that mandating that cbn parameters themselves are lifted out with def instead of val is not enough. You'd still need to figure out how to do something like

          val t = test(identity(println("never seen")))_
          

          Reassigning to you, because I don't know a better solution.

          Show
          Martin Odersky added a comment - You'd need to propose another spec for eta-expansion then. The compiler is faithful to the spec in 6.25.5. Note that mandating that cbn parameters themselves are lifted out with def instead of val is not enough. You'd still need to figure out how to do something like val t = test(identity(println("never seen")))_ Reassigning to you, because I don't know a better solution.
          Hide
          Matt Hellige added a comment -

          Here's one possibility. Considering section 6.25.5, in the result of eta-conversion, change:

          { val x1 = e1;
            ...
            val xm = em;
            (y1:T1,...,yn:Tn) => e'(y1,...,yn)
          }
          

          with

          { val x1 = (() => e1);
            ...
            val xml = (() => em);
            ...
          }
          

          and in e', replace each ei by xi() rather than xi.

          This has the effect of explicitly thunking each parameter, and then allowing the usual method call compilation to determine whether or not to unthunk them at call time.

          If it is possible at expansion-time to inspect the type of the target method and determine which ei are in "by-name position" or whatever you want to call it, then this explicit thunking can be performed only for by-name parameters, eliminating the unnecessary overhead in the common case.

          Also, if there is some other way of declaring first-class "by-name values," that might be preferred to thunks. IIRC there is no way of doing so, but perhaps internally to the compiler there is some trick available.

          What do you think? I may try to have a go at implementing this, but it would be my first compiler change, so I won't make any promises. Also, if there is a better place to discuss this, please let me know.

          Show
          Matt Hellige added a comment - Here's one possibility. Considering section 6.25.5, in the result of eta-conversion, change: { val x1 = e1; ... val xm = em; (y1:T1,...,yn:Tn) => e'(y1,...,yn) } with { val x1 = (() => e1); ... val xml = (() => em); ... } and in e', replace each ei by xi() rather than xi. This has the effect of explicitly thunking each parameter, and then allowing the usual method call compilation to determine whether or not to unthunk them at call time. If it is possible at expansion-time to inspect the type of the target method and determine which ei are in "by-name position" or whatever you want to call it, then this explicit thunking can be performed only for by-name parameters, eliminating the unnecessary overhead in the common case. Also, if there is some other way of declaring first-class "by-name values," that might be preferred to thunks. IIRC there is no way of doing so, but perhaps internally to the compiler there is some trick available. What do you think? I may try to have a go at implementing this, but it would be my first compiler change, so I won't make any promises. Also, if there is a better place to discuss this, please let me know.
          Hide
          Matt Hellige added a comment -

          Or maybe this is the same as lifting to defs rather than vals. I'm not sure I understand your identity example... Assuming identity is:
          def identity[A](a: => A) = a
          the def solution seems to work as I'd expect.

          Show
          Matt Hellige added a comment - Or maybe this is the same as lifting to defs rather than vals. I'm not sure I understand your identity example... Assuming identity is: def identity [A] (a: => A) = a the def solution seems to work as I'd expect.
          Hide
          Johannes Rudolph added a comment -

          I thought a bit about this and it seems clear that the current behavior is wrong because it violates the expectation that

          (test { println("Test" } _)()
          

          has the same result as

          test { println("Test" }()
          

          I.e. that you can either apply a function piece-wise partially or directly with all arguments and get the same result.

          I think the solution as given by Matt is pretty good: at eta-expansion by-name parameters are lifted to vals of a thunk value (of type () => T). However, this is not a valid type to pass as a by-name argument so the application has to change from x1 to x1.apply() as well. If you do this naively the compiler will then (in uncurry) create another redundant thunk out of this application. I therefore propose another change where an expression f.apply() with f being an instance of Function0 in by-name-argument position isn't thunked but instead the application is removed and f passed directly. This should be a safe optimization in all cases because the generated thunk never does more than simple thunking.

          The hard part of producing proper legalese for the scripture maybe isn't so hard at all because we have a precedent by now. This already is in "§6.6 Function Applications":

          The behavior of by-name parameters is preserved if the application is transformed into a block due to named or default arguments. In this case, the local value for that parameter has the form val y_i = () => e and the argument passed to the function is y_i ().

          We just have to amend the first sentence by "or eta-expansion". Problem solved.

          If we can reach a consensus that this is the right approach I can put together a pull request with an implementation proposal.

          Show
          Johannes Rudolph added a comment - I thought a bit about this and it seems clear that the current behavior is wrong because it violates the expectation that (test { println("Test" } _)() has the same result as test { println("Test" }() I.e. that you can either apply a function piece-wise partially or directly with all arguments and get the same result. I think the solution as given by Matt is pretty good: at eta-expansion by-name parameters are lifted to vals of a thunk value (of type () => T). However, this is not a valid type to pass as a by-name argument so the application has to change from x1 to x1.apply() as well. If you do this naively the compiler will then (in uncurry) create another redundant thunk out of this application. I therefore propose another change where an expression f.apply() with f being an instance of Function0 in by-name-argument position isn't thunked but instead the application is removed and f passed directly. This should be a safe optimization in all cases because the generated thunk never does more than simple thunking. The hard part of producing proper legalese for the scripture maybe isn't so hard at all because we have a precedent by now. This already is in "§6.6 Function Applications": The behavior of by-name parameters is preserved if the application is transformed into a block due to named or default arguments. In this case, the local value for that parameter has the form val y_i = () => e and the argument passed to the function is y_i (). We just have to amend the first sentence by "or eta-expansion". Problem solved. If we can reach a consensus that this is the right approach I can put together a pull request with an implementation proposal.
          Hide
          Johannes Rudolph added a comment -

          See also SI-1247 for the optimization I proposed.

          Show
          Johannes Rudolph added a comment - See also SI-1247 for the optimization I proposed.
          Hide
          Robert Gibson added a comment -

          Duplicate of SI-5610 I guess.

          Show
          Robert Gibson added a comment - Duplicate of SI-5610 I guess.
          Hide
          Matt Hellige added a comment -

          Considering that this bug is five years and 5300 bugs older, I'd think that would go the other way...

          Show
          Matt Hellige added a comment - Considering that this bug is five years and 5300 bugs older, I'd think that would go the other way...
          Hide
          Lukas Rytz added a comment -

          fixed together with SI-5610

          Show
          Lukas Rytz added a comment - fixed together with SI-5610

            People

            • Assignee:
              Lukas Rytz
              Reporter:
              Matt Hellige
              TracCC:
              Johannes Rudolph, Lachlan O'Dea, Seth Tisue
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development