SI-6623 -Yrepl-use-magic-imports avoids nesting $iw wrappers #8576

retronym · 2019-12-05T07:23:42Z

Rather than adding a wrapper object for each import in the session history,
just use a single wrapper preceded by the imports which have been
interspersed with a magic import to bump context depth.

Code is still ordinarily wrapped in a $read object.

This is a step toward 6623-like transparency.

retronym takes the blame for this innovation.
adriaanm collaborated in its commission.
somsnytt batted clean-up.

The following stress test now completes; before it exhausted memory after 70 iterations.

for i in {1..1000}; do printf 'object bar { val foo = '$i'}; \nimport bar.foo\n foo\n val foo = ""\n foo\n'; done | ./build/pack/bin/scala -Yrepl-use-magic-imports

lrytz · 2019-12-05T10:29:48Z

Nice idea! Maybe give it a test run in combination with -Yrepl-class-based?

som-snytt

Way to "level up"!

som-snytt · 2019-12-05T18:31:55Z

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

  val YmacroFresh     = BooleanSetting    ("-Ymacro-global-fresh-names", "Should fresh names in macros be unique across all compilation units")
  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 YreplMagicImport = BooleanSetting    ("-Yrepl-use-magic-imports", "In the code the wraps REPL snippes, use magic imports to rather than nesting wrapper object/classes")


sorry to snipe at snippes, which I guess is the French.

In the code that wraps REPL snippets, use magic imports rather

som-snytt · 2019-12-05T18:42:34Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

+      tree match {
+        case Import(expr, selector :: Nil) =>
+          // Just a syntactic check to avoid forcing typechecking of imports
+          selector.name.string_==(nme.INTERPRETER_IMPORT_LEVEL_UP) && owner.enclosingTopLevelClass.isInterpreterWrapper


My other use case was to submit a directory of source files as a single compilation unit, with files wrapped in LEVEL_UP and LEVEL_DOWN and then concatenated in a single source, so that top-level statements would not collide. That would not be in a repl wrapper. If double brace isn't magical enough, how about ${{?

Maybe also call it WRAPL.

What's the user-level use case for that? Something to do with sealed?

Yes, on that thread they were complaining that their files get too big when they cram everything in. That also covers companionship. Sometimes I feel like I'm crammed in with my companion.

som-snytt · 2019-12-05T19:15:55Z

src/compiler/scala/tools/nsc/typechecker/Namers.scala

+      // warn proactively if specific import loses to definition in scope,
+      // since it may result in desired implicit not imported into scope.
+      def checkNotRedundant(pos: Position, from: Name, to0: Name): Unit = {
+        def check(to: Name): Unit = {


Um. I guess since we're talking import magic. I know the check used to be wrong, false positive. Now I'm not sure what the user story is. Oh, this is covered by unused import. So is this check only guarding against old bugs in name binding? Including implicit shadowing?

object X { val global = 0 import concurrent.ExecutionContext.global println(global) }

Yes, I see bug 2866 is due to Jason: "I have five unanswered questions"... after grepping for permanently hidden again in the check files...

Yeah, I dropped your changes to this code from #5220 as I didn't understand the original code or the change. 🤷‍♂

I'm not sure it's very meaningful, even if it had a bug-compatible meaning once.

som-snytt · 2019-12-05T19:18:43Z

src/repl/scala/tools/nsc/interpreter/Imports.scala

-    def addWrapper() {
-      import nme.{ INTERPRETER_IMPORT_WRAPPER => iw }
+    def addWrapperCode(): Unit = {
+      import nme.{INTERPRETER_IMPORT_WRAPPER => iw}


A lot has changed in four years, including spaces in braces.

som-snytt · 2019-12-05T19:19:07Z

src/repl/scala/tools/nsc/interpreter/Imports.scala

+        addLevelChangingImport()
+      } else {
+        addWrapperCode()
+      }


Lots of braces.

retronym · 2019-12-06T00:42:07Z

/rebuild 09063fc

retronym · 2019-12-09T06:09:52Z

Nice idea! Maybe give it a test run in combination with -Yrepl-class-based?

I've enabled -Yrepl-class-based by default for a temporary commit to survey the test failures.

I eliminated some of these failures by:

fixing a bug in tab completions under this mode
aligning the line numbers in the two strategies to avoid benign but annoying differences in parse errors (in 2.12.x we don't have the logic to subtract the pre-amble span from the position of reported errors which makes these tests brittle with respect to the encoding)

The remaining problems are:

Can't define value classes and macro implementations in the REPL. These constructs need to be wrapped in an object.
-Yrepl-class-based disables wildcard of imports from previous lines when the current line defines a class. Those imports might might contribute necessary implicits, for instance language.dynamics is needed in this test. Fixing it without regressing Spark will take a separate change to do some dead-code elimination to avoid unneeded capture which is bad for serialization (either makes serialization slow or makes it fail.)

We need some sort of story for these use cases before we could enable class-based mode by default.

- Make `ImportContext` a nested class - Avoid the constructor of the superclass `Context` needing to call methods on the unitialized subclass ImportContext by computing `isRootContext` / `depth` externally in the factory method.

Rather than adding a wrapper object for each import in the session history, just use a single wrapper preceded by the imports which have been interspersed with a magic import to bump context depth. Code is still ordinarily wrapped in a `$read` object. This is a step toward 6623-like transparency. `retronym` takes the blame for this innovation. `adriaanm` collaborated in its commission. `somsnytt` batted clean-up.

To be reverted.

… bug By default, the parser tries to heal incomplete source files by inserting missing braces. Compilation will still error out, but any subsequent parser/type errors make more sense to the user when this healing works. The healing uses indentation to figure out the intent of the code. Wrapped REPL snippets aren't properly indented, and in the test case I added the healing seems counterproductive. This commit disables it in REPL tab completion, in line with the way that IMain parses the wrapped code for real compilation.

- add comments describing a problem I discovered with imports and a possible solution - Make strip wrappers from REPL output for the wrappers generated in this mode

This detail changes under -Yrepl-class-based. We prefer tests that operate under either mode.

Revert "Enable magic imports unconditionally to survey test results" This reverts commit b02f890. Revert "Enable -Yrepl-class-based to survey test failures" This reverts commit d64af1a.

som-snytt · 2019-12-13T04:21:57Z

I was about to rebase when I realized this is on an old version of Scala. I was just doing :javap in power mode, which exacerbates the in-your-faceness. Congrats on bringing this back. It's like those seeds that lie dormant in the soil until a wildfire. Or something.

retronym · 2019-12-13T05:38:46Z

I'll do the hand-woven forward port of this to 2.13.x: https://github.com/scala/scala/compare/2.13.x...retronym:topic/repl-magic-forward-port?expand=1

scala-jenkins added this to the 2.12.11 milestone Dec 5, 2019

retronym requested review from adriaanm and som-snytt and removed request for som-snytt December 5, 2019 07:23

retronym force-pushed the review/5220 branch from 6f5ae9e to 16778e7 Compare December 5, 2019 07:29

som-snytt approved these changes Dec 5, 2019

View reviewed changes

retronym force-pushed the review/5220 branch from 16778e7 to 2c07d7d Compare December 9, 2019 05:47

retronym force-pushed the review/5220 branch from ca0d6ee to 6527e07 Compare December 10, 2019 02:24

retronym added 5 commits December 10, 2019 13:11

Refactor Context creation

dca6f5a

- Make `ImportContext` a nested class - Avoid the constructor of the superclass `Context` needing to call methods on the unitialized subclass ImportContext by computing `isRootContext` / `depth` externally in the factory method.

Enable magic imports unconditionally to survey test results

9e146e8

To be reverted.

Small improvements to REPL -Yrepl-class-based

1659fec

- add comments describing a problem I discovered with imports and a possible solution - Make strip wrappers from REPL output for the wrappers generated in this mode

retronym force-pushed the review/5220 branch from 6527e07 to 952e561 Compare December 10, 2019 03:11

:javap in REPL deals better with nested modules

ff991f5

retronym force-pushed the review/5220 branch from 952e561 to cfaf9f8 Compare December 10, 2019 11:34

Stabilize output in REPL tests between indy and inline lambdas

f4cb764

This detail changes under -Yrepl-class-based. We prefer tests that operate under either mode.

retronym force-pushed the review/5220 branch from cfaf9f8 to b63b7dc Compare December 12, 2019 03:35

retronym added 2 commits December 12, 2019 15:31

Enable -Yrepl-class-based to survey test failures

cba5394

Revert exploratory changes to default REPL wrappers

50c74cd

Revert "Enable magic imports unconditionally to survey test results" This reverts commit b02f890. Revert "Enable -Yrepl-class-based to survey test failures" This reverts commit d64af1a.

retronym force-pushed the review/5220 branch from b63b7dc to 50c74cd Compare December 12, 2019 05:32

retronym merged commit 9761528 into scala:2.12.x Dec 13, 2019

retronym mentioned this pull request Dec 17, 2019

Forward port REPL wrapper changes to 2.13.x #8600

Merged

SethTisue added the release-notes worth highlighting in next release notes label Dec 17, 2019

som-snytt mentioned this pull request Jan 1, 2020

REPL cheats me scala/bug#6623

Open

dwijnand mentioned this pull request Feb 6, 2020

REPL wrap-up? scala/scala-dev#119

Closed

retronym mentioned this pull request Feb 13, 2020

Adapt to REPL changes planned for Scala 2.12.11 retronym/spark#1

Open

SethTisue added the tool:REPL Changes to the Scala REPL shell label Mar 9, 2020

lrytz added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Mar 16, 2020

dwijnand mentioned this pull request Mar 26, 2020

Default true -Yrepl-{class-based,use-magic-imports} #8748

Merged

SI-6623 -Yrepl-use-magic-imports avoids nesting $iw wrappers #8576

SI-6623 -Yrepl-use-magic-imports avoids nesting $iw wrappers #8576

Uh oh!

Conversation

retronym commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Dec 5, 2019

Uh oh!

som-snytt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym commented Dec 6, 2019

Uh oh!

retronym commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Dec 13, 2019

Uh oh!

retronym commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

retronym commented Dec 5, 2019 •

edited

Loading

retronym commented Dec 9, 2019 •

edited

Loading

retronym commented Dec 13, 2019 •

edited

Loading