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

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

Merged
merged 9 commits into from Dec 13, 2019

Conversation

@retronym
Copy link
Member

retronym commented Dec 5, 2019

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
@scala-jenkins scala-jenkins added this to the 2.12.11 milestone Dec 5, 2019
@retronym retronym requested review from som-snytt and adriaanm and removed request for som-snytt Dec 5, 2019
@retronym retronym force-pushed the retronym:review/5220 branch from 6f5ae9e to 16778e7 Dec 5, 2019
@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Dec 5, 2019

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

Copy link
Contributor

som-snytt left a comment

Way to "level up"!

@@ -237,6 +237,7 @@ trait ScalaSettings extends AbsScalaSettings
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")

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

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

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

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

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

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.

This comment has been minimized.

Copy link
@retronym

retronym Dec 6, 2019

Author Member

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

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 6, 2019

Contributor

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.

// 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 = {

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

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)
}

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

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

This comment has been minimized.

Copy link
@retronym

retronym Dec 6, 2019

Author Member

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

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 6, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

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

addLevelChangingImport()
} else {
addWrapperCode()
}

This comment has been minimized.

Copy link
@som-snytt

som-snytt Dec 5, 2019

Contributor

Lots of braces.

@retronym

This comment has been minimized.

Copy link
Member Author

retronym commented Dec 6, 2019

/rebuild 09063fc

@retronym retronym force-pushed the retronym:review/5220 branch from 16778e7 to 2c07d7d Dec 9, 2019
@retronym

This comment has been minimized.

Copy link
Member Author

retronym commented Dec 9, 2019

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.

@retronym retronym force-pushed the retronym:review/5220 branch from ca0d6ee to 6527e07 Dec 10, 2019
retronym added 5 commits Dec 3, 2019
  - 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
@retronym retronym force-pushed the retronym:review/5220 branch from 6527e07 to 952e561 Dec 10, 2019
@retronym retronym force-pushed the retronym:review/5220 branch from 952e561 to cfaf9f8 Dec 10, 2019
This detail changes under -Yrepl-class-based. We prefer tests that operate
under either mode.
@retronym retronym force-pushed the retronym:review/5220 branch from cfaf9f8 to b63b7dc Dec 12, 2019
retronym added 2 commits Dec 9, 2019
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 retronym force-pushed the retronym:review/5220 branch from b63b7dc to 50c74cd Dec 12, 2019
@retronym retronym merged commit 9761528 into scala:2.12.x Dec 13, 2019
4 checks passed
4 checks passed
cla @retronym signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [2527] SUCCESS. Took 68 min.
Details
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Dec 13, 2019

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

This comment has been minimized.

Copy link
Member Author

retronym commented Dec 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.