SI-9076 REPL wrap is App #4246
SI-9076 REPL wrap is App #4246
Conversation
|
Removing the extra wrapper means error message lines are off by one, and messages reporting the fully qualified name are shorter. I won't push an --update-check until someone likes this PR on Facebook. Are BippyBups those candies they used to hand out at Halloween? |
|
Looks really promising. I've performed some cursory testing but haven't managed to break it just yet. I would suggest adding a setting to restore the old behaviour in case of unforseen problems. I know that Spark and ScalaLab uses the REPL in a slightly custom manner. /cc @dragos @heathermiller |
|
P.S. Nice use of |
|
This breaks
Manually, it works with Edit: it also fails pasted raw, so something is broken: |
|
Pushed with updated check files, a fix for evaluation by script engine, and a user option to select the template, |
dc02a88 to
c49dee9
Compare
|
More check file updates and a test. |
Rebasing on #4249 should fix this one. |
|
If you rebase, could you give an example of the sort of class initialization trap in the commit comment. The |
Avoid class initialization traps in the object wrapper by
extending App.
Previously, the following line would hang in the REPL
because the parallel operation was started from the constructor
of the wrapping object, i.e., from the static initializer of
the class. Class-loading the closure would deadlock on loading
the wrapper, which in turn blocks on completion of the par.map.
```
scala> def i = 42 ; List(1, 2, 3).par.map(x => x + i)
```
Any user code that starts a thread could deadlock, including
innocent experiments with Futures in for comprehensions.
The App object is initialized in the usual place, namely
the print method of the eval object, by invoking main.
(A lazy `compute` accommodates two paths of evaluation:
`print` for REPL, `result` for script engine.)
A compiler option `-YreplWrap` takes values "class", "object"
and "app" (default) to select the wrapper template.
```
scala> 42 // show
object $read extends scala.AnyRef {
def <init>() = {
super.<init>;
()
};
object $iw extends App {
def <init>() = {
super.<init>;
()
};
val res0 = 42
};
}
[[syntax trees at end of typer]] // <console>
package $line3 {
object $read extends scala.AnyRef {
def <init>(): $line3.$read.type = {
$read.super.<init>();
()
};
object $iw extends AnyRef with App {
def <init>(): type = {
$iw.super.<init>();
()
};
private[this] val res0: Int = 42;
<stable> <accessor> def res0: Int = $iw.this.res0
}
}
}
[[syntax trees at end of typer]] // <console>
package $line3 {
object $eval extends scala.AnyRef {
def <init>(): $line3.$eval.type = {
$eval.super.<init>();
()
};
<stable> <accessor> lazy def compute: Unit = $line3.$read.$iw.main(null);
lazy private[this] var $result: Int = _;
<stable> <accessor> lazy def $result: Int = {
$eval.this.$result = {
$eval.this.compute;
$line3.$read.$iw.res0
};
$eval.this.$result
};
lazy private[this] var $print: String = _;
<stable> <accessor> lazy def $print: String = {
$eval.this.$print = {
$eval.this.compute;
"res0: Int = ".+(scala.runtime.ScalaRunTime.replStringOf($line3.$read.$iw.res0, 1000))
};
$eval.this.$print
}
}
}
res0: Int = 42
```
|
Rebased. Sorry for the bother, Jenkins, I neglected to amend the commit message, which I've now done. I hope I didn't put you to any trouble, there's a good chap. |
| @@ -197,7 +197,14 @@ trait ScalaSettings extends AbsScalaSettings | |||
| val Ymacroexpand = ChoiceSetting ("-Ymacro-expand", "policy", "Control expansion of macros, useful for scaladoc and presentation compiler", List(MacroExpand.Normal, MacroExpand.None, MacroExpand.Discard), MacroExpand.Normal) | |||
| val Ymacronoexpand = BooleanSetting ("-Ymacro-no-expand", "Don't expand macros. Might be useful for scaladoc and presentation compiler, but will crash anything which uses macros and gets past typer.") withDeprecationMessage(s"Use ${Ymacroexpand.name}:${MacroExpand.None}") withPostSetHook(_ => Ymacroexpand.value = MacroExpand.None) | |||
| val Yreplsync = BooleanSetting ("-Yrepl-sync", "Do not use asynchronous code for repl startup") | |||
| val Yreplclassbased = BooleanSetting ("-Yrepl-class-based", "Use classes to wrap REPL snippets instead of objects") | |||
| val Yreplclassbased = BooleanSetting ("-Yrepl-class-based", "Use classes to wrap REPL snippets instead of objects") withPostSetHook (self => if (self) YreplWrap.value = "class") | |||
There was a problem hiding this comment.
I see you don't set replWrap on -Yrepl-class-based:false. Boolean flags used to be so much simpler to reason about.
|
Tests pass, but IDE tackled me short of the goal line. |
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 70583165) |
|
/cc @gzm0 |
| val Yreplclassbased = BooleanSetting ("-Yrepl-class-based", "Use classes to wrap REPL snippets instead of objects") | ||
| val Yreplclassbased = BooleanSetting ("-Yrepl-class-based", "Use classes to wrap REPL snippets instead of objects") withPostSetHook (self => if (self) YreplWrap.value = "class") | ||
| val YreplWrap = ChoiceSetting( | ||
| name = "-YreplWrap", |
|
I think we should go for the more conservative approach of introducing this as a non-default wrapper in 2.11.6, and after gathering some experience switching to it by default in 2.12.0. As an example of the sort of thing that people might rely on: I remember once using |
|
Do we expect the generated code to have changed from 2.11.5 to |
|
Conservative The commit removes (presumably extraneous) wrappers; I couldn't find a use case for it. I'd have to refresh my memory if the "force initialization" changes, too. IIRC, it was a fix at some point to make repl line numbers correspond to the source. |
|
now #4311 |
Avoid class initialization traps in the object wrapper by
extending App.
The App object is initialized in the usual place, namely
the print method of the eval object, by invoking main.