SPARK-571: forbid return statements in cleaned closures

[willb]

This patch checks top-level closure arguments to ClosureCleaner.clean for return statements and raises an exception if it finds any. This is mainly a user-friendliness addition, since programs with return statements in closure arguments will currently fail upon RDD actions with a less-than-intuitive error message.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14858/

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[andrewor14]

nit: This function is pretty simple. Mind moving it into run() directly?

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14859/

[willb]

Thanks for the quick feedback, @andrewor14! I'll make those changes. I'll also look to see if I can figure out why that repartition test is failing; oddly, it's working for me locally.

[pwendell]

@willb the re-partition issue is not related to your patch. There was an inadvertent check-in in master that caused it. It has been fixed.

[willb]

Thanks for the clarification, @pwendell.

@andrewor14, I've grouped my imports and collapsed that function. Thanks again.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[rxin]

Why do closures with return fail?

[rxin]

Ah never mind my quesiton. "return" exits the outer level function instead of the closure.

[rxin]

add a space after if (also for the if below)

[rxin]

I left couple comments on style. Thanks for leading this.

[willb]

@rxin, yes! (I was firing up a debugger in case you wanted to know exactly where it was returning to.)

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14932/

[rxin]

@willb can you also add a test case where an inline function is using return statement in its closure to make sure that is allowed? e.g.

sc.parallelize(1 to 100).map { i =>
  def foo(): Int = { return 1; 2 }
  foo()
}

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[willb]

@rxin, the latest commit adds such a test.

I'll note that the code I have only finds toplevel return statements, so it won't spuriously reject code like that example, but it also won't reject code like this, which is an invalid use of return:

nums.map {x => 
  // this return is invalid since it will transfer control outside the closure
  val foo = {y: Int => return 2; 1 }
  foo(x)
}

I thought identifying toplevel return statements in closures represented an acceptable tradeoff between complexity and user-friendliness in order to provide better feedback in the common case (but not necessarily to exhaustively flag all bogus closures, no matter how pathological). If this isn't the right tradeoff, I could adapt the code to reject a higher percentage of closures with nonlocal returns.

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14946/

[rxin]

I think the current approach is fine. I'm going to merge this.

[willb]

Thanks, @rxin!

[rxin]

BTW I am merging this only into master (not branch-1.0) since it is too late for changes into Spark core for 1.0 unless they are critical bug fixes.