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

Overly-Eager Evaluation of By-Name Parameter in Inferred Partial Application #5610

Closed
scabug opened this Issue Mar 26, 2012 · 15 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Mar 26, 2012

object Bug extends App {
  class Result(_str: => String) {
    lazy val str = _str
  }
  
  def foo(str: => String)(i: Int) = new Result(str)
  
  def bar(f: Int => Result) = f(42)
  
  var test: String = null
  val result = bar(foo(test))
  test = "bar"
  
  if (result.str == null) {
    println("Destroy ALL THE THINGS!!!")
  } else {
    println("Stroke a kitten")
  }
}

We would expect the output of this code would be "Stroke a kitten", since the value of test is given a non-null value before the str lazy val is realized. However, the actual printout is "Destroy ALL THE THINGS!!!", which isn't a polite thing to do.

The problem lies in the bytecode generated for bar(foo(test)). More specifically, the issue is in the lifting of the test actual for the foo invocation. Bytecode (from Bug$delayedInit$body.apply:()Ljava/lang/Object;):

      18: aload_0       
      19: getfield      #12                 // Field $outer:LBug$;
      22: invokevirtual #22                 // Method Bug$.test:()Ljava/lang/String;
      25: astore_1      
      26: new           #24                 // class Bug$$anonfun$1
      29: dup           
      30: aload_1       
      31: invokespecial #27                 // Method Bug$$anonfun$1."<init>":(Ljava/lang/String;)V

Notice instruction 22. We're getting a raw (un-thunked) value for test and then lifting it into the requisite closure for the foo(test) invocation.

Using an explicit thunk (of type () => String) rather than a by-name parameter for str avoids this issue.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Mar 26, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5610?orig=1
Reporter: @djspiewak
Affected Versions: 2.9.1
See #1980
Duplicates #302

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Mar 26, 2012

@odersky said:
Frankly, I think the sooner we put DelayedInit to rest the better. I was responsible for its introduction, mostly to fix Application, but now I think we have exchanged pest for cholera. App should be a very simple macro that simply lifts its body into a method.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Mar 26, 2012

@retronym said:
This one's not related to DelayedInit.

object Bug {
  def main(args: Array[String]) {
    var test: String = null
    val result = bar(foo(test))
    test = "bar"
  
    if (result.str == null) {
      println("Destroy ALL THE THINGS!!!")
    } else {
      println("Stroke a kitten")
    }
  }

  class Result(_str: => String) {
    lazy val str = _str
  }
  
  def foo(str: => String)(i: Int) = new Result(str)
  
  def bar(f: Int => Result) = f(42)   
}
@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 13, 2012

@lrytz said (edited on May 13, 2012 9:56:32 AM UTC):
This is actually an eta-expansion problem, very nifty:

object Bug {
  def main(args: Array[String]): Unit = {
    var test: String = null
    val fun1: Int => () => Unit = foo(test) _
    val fun2: Int => () => Unit = foo(test)(_)
    test = "some string"
    fun1(1)()
    fun2(1)()
  }
  
  def foo(s: => String)(dummy: Int) = () => println(s)
}

This prints

null
some string

because the two functions f1 and f2 are constructed differently (-Xprint:typer)

val fun1: Int => (() => Unit) = {
  <synthetic> val eta$0$1: String = test;
  ((dummy: Int) => Bug.this.foo(eta$0$1)(dummy))
};
val fun2: Int => (() => Unit) = ((x$1: Int) => Bug.this.foo(test)(x$1));

I guess the synthetic value should be lazy.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 13, 2012

@retronym said:
Very similar to #1980.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 13, 2012

@lrytz said:
Fix pending in [https://github.com/lrytz/scala/tree/t5610].

However, this issue is blocked by #5796. Another option is to use a Function0 instead of a lazy val, i.e.

val fun1: Int => (() => Unit) = {
  <synthetic> val eta$0$1: () => String = () => test;      // Create Function0 value
  ((dummy: Int) => Bug.this.foo(eta$0$1())(dummy))         // Apply the function here
};
val fun2: Int => (() => Unit) = ((x$1: Int) => Bug.this.foo(test)(x$1));

This is what is currently done for named arguments, i.e. given def f(a: Int, b: => Int), a call f(b = expr, a = 1) is transformed to

{
  val x$1: () => Int = () => expr
  val x$2: Int = 1
  f(x$2, x$1)
}
@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 13, 2012

@retronym said:
Arguably a Function0 is simpler in this case, and thanks to #1247 there is no performance penalty.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 14, 2012

@lrytz said:
cool, that will also help the named arguments transformation.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 16, 2012

@lrytz said:
fixed in [https://github.com/scala/scala/commit/e33901084242b4d7cd76a9279430d0cbc9b9a152]

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 28, 2012

Robert Gibson (rgibson) said:
Are you also going to change §6.26.5 of the spec, which specified the old, eager, behaviour in case of eta expansion?

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 29, 2012

@lrytz said:
right... will look at the spec

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 29, 2012

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jun 1, 2012

@jrudolph said:
Funny, I missed this one completely. I had a similar patch in work for #308 but since the block was on how to change the spec I first wanted to make my homework to propose a reasonable addition for the 'Eta-Expansion' section of the spec. Good to see it fixed.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jul 10, 2012

@SethTisue said:
shouldn't this ticket and #302 both be closed now? the new behavior is already in M4.

(is it because the spec change is still pending?)

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jul 12, 2012

@lrytz said:
pending spec change logged in #6069

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