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

partial application of methods ignores call-by-name parameters #302

Closed
scabug opened this issue Dec 21, 2007 · 13 comments
Closed

partial application of methods ignores call-by-name parameters #302

scabug opened this issue Dec 21, 2007 · 13 comments
Assignees

Comments

@scabug
Copy link

@scabug scabug commented Dec 21, 2007

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.

@scabug
Copy link
Author

@scabug scabug commented Dec 21, 2007

Imported From: https://issues.scala-lang.org/browse/SI-302?orig=1
Reporter: Matt Hellige (hellige)

Loading

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2009

@odersky said:
Milestone next_bugfix deleted

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

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

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

Matt Hellige (hellige) said:
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.

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

Matt Hellige (hellige) said:
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?

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

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

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

Matt Hellige (hellige) said:
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.

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 13, 2009

Matt Hellige (hellige) said:
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.

Loading

@scabug
Copy link
Author

@scabug scabug commented May 8, 2012

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

Loading

@scabug
Copy link
Author

@scabug scabug commented May 8, 2012

@jrudolph said:
See also #1247 for the optimization I proposed.

Loading

@scabug
Copy link
Author

@scabug scabug commented May 31, 2012

Robert Gibson (rgibson) said:
Duplicate of #5610 I guess.

Loading

@scabug
Copy link
Author

@scabug scabug commented May 31, 2012

Matt Hellige (hellige) said:
Considering that this bug is five years and 5300 bugs older, I'd think that would go the other way... ;)

Loading

@scabug
Copy link
Author

@scabug scabug commented Jul 12, 2012

@lrytz said:
fixed together with #5610

Loading

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