From 83633a9d311869037547e16ffe7217d79d022930 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 1 Dec 2021 22:21:43 +0000 Subject: [PATCH] Fix REPL clashing with CWD artefacts This reverts the change that moves all the REPL's artefacts into the "repl$" package, moving everything back to the empty package. Then it fixes the class name and package name shadowing issue, by simulating what the batch compiler does when it compiles a source file without a package statement: it considers the sources to be in the empty package. Then the imports for the previous runs and the user-defined imports will be in sub-contexts of the empty package. The reason the working directory artefacts were shadowing the REPL artefacts was because before the imports were being added to sub-contexts of the root package, afterwhich the REPL tree was being wrapped in the empty package. That meant that the classes found in the empty package (i.e. classfiles in the working directory, or even directories) had precedence over the wildcard imports for the previous runs. The tests now cascade up into contexts that don't have the current compilation unit stored on them, so I had to safeguard against that in Typer. Also, I developed a variant of ShadowingTests, using the batch compiler, to automate the expectations of how the shadowing should and shouldn't work. --- .../src/dotty/tools/dotc/core/StdNames.scala | 1 - .../src/dotty/tools/dotc/typer/Typer.scala | 2 +- compiler/src/dotty/tools/repl/Rendering.scala | 4 +- .../src/dotty/tools/repl/ReplCompiler.scala | 12 ++-- .../src/dotty/tools/repl/ScriptEngine.scala | 2 +- .../dotc/reporting/ErrorMessagesTest.scala | 2 +- .../tools/repl/ShadowingBatchTests.scala | 71 +++++++++++++++++++ .../dotty/tools/repl/ShadowingTests.scala | 4 -- 8 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 compiler/test/dotty/tools/repl/ShadowingBatchTests.scala diff --git a/compiler/src/dotty/tools/dotc/core/StdNames.scala b/compiler/src/dotty/tools/dotc/core/StdNames.scala index 2e0b229ca42c..494f08688bc7 100644 --- a/compiler/src/dotty/tools/dotc/core/StdNames.scala +++ b/compiler/src/dotty/tools/dotc/core/StdNames.scala @@ -133,7 +133,6 @@ object StdNames { val OPS_PACKAGE: N = "" val OVERLOADED: N = "" val PACKAGE: N = "package" - val REPL_PACKAGE: N = "repl$" val ROOT: N = "" val SPECIALIZED_SUFFIX: N = "$sp" val SUPER_PREFIX: N = "super$" diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 6177faf6b6b0..30e0d813320c 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -298,7 +298,7 @@ class Typer extends Namer */ def isDefinedInCurrentUnit(denot: Denotation)(using Context): Boolean = denot match { case MultiDenotation(d1, d2) => isDefinedInCurrentUnit(d1) || isDefinedInCurrentUnit(d2) - case denot: SingleDenotation => denot.symbol.source == ctx.compilationUnit.source + case denot: SingleDenotation => ctx.compilationUnit != null && denot.symbol.source == ctx.compilationUnit.source } /** Is `denot` the denotation of a self symbol? */ diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index 085df909d71e..3c0db3f5ff6a 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -158,7 +158,7 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) { //at repl$.rs$line$2$.(rs$line$2:1) //at repl$.rs$line$2.res1(rs$line$2) def isWrapperInitialization(ste: StackTraceElement) = - ste.getClassName.startsWith(nme.REPL_PACKAGE.toString + ".") // d.symbol.owner.name.show is simple name + ste.getClassName.startsWith(REPL_WRAPPER_NAME_PREFIX) // d.symbol.owner.name.show is simple name && (ste.getMethodName == nme.STATIC_CONSTRUCTOR.show || ste.getMethodName == nme.CONSTRUCTOR.show) cause.formatStackTracePrefix(!isWrapperInitialization(_)) @@ -170,7 +170,7 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) { } object Rendering { - final val REPL_WRAPPER_NAME_PREFIX = s"${nme.REPL_PACKAGE}.${str.REPL_SESSION_LINE}" + final val REPL_WRAPPER_NAME_PREFIX = str.REPL_SESSION_LINE extension (s: Symbol) def showUser(using Context): String = { diff --git a/compiler/src/dotty/tools/repl/ReplCompiler.scala b/compiler/src/dotty/tools/repl/ReplCompiler.scala index d80e9871fdee..9249172b5d4d 100644 --- a/compiler/src/dotty/tools/repl/ReplCompiler.scala +++ b/compiler/src/dotty/tools/repl/ReplCompiler.scala @@ -47,7 +47,7 @@ class ReplCompiler extends Compiler { def importPreviousRun(id: Int)(using Context) = { // we first import the wrapper object id - val path = nme.REPL_PACKAGE ++ "." ++ objectNames(id) + val path = nme.EMPTY_PACKAGE ++ "." ++ objectNames(id) val ctx0 = ctx.fresh .setNewScope .withRootImports(RootRef(() => requiredModuleRef(path)) :: Nil) @@ -59,9 +59,9 @@ class ReplCompiler extends Compiler { importContext(imp)(using ctx)) } - val rootCtx = super.rootContext - .withRootImports // default root imports - .withRootImports(RootRef(() => defn.EmptyPackageVal.termRef) :: Nil) + val rootCtx = super.rootContext.fresh + .setOwner(defn.EmptyPackageClass) + .withRootImports (1 to state.objectIndex).foldLeft(rootCtx)((ctx, id) => importPreviousRun(id)(using ctx)) } @@ -147,7 +147,7 @@ class ReplCompiler extends Compiler { val module = ModuleDef(objectTermName, tmpl) .withSpan(span) - PackageDef(Ident(nme.REPL_PACKAGE), List(module)) + PackageDef(Ident(nme.EMPTY_PACKAGE), List(module)) } private def createUnit(defs: Definitions, span: Span)(using Context): CompilationUnit = { @@ -249,7 +249,7 @@ class ReplCompiler extends Compiler { val wrapper = TypeDef("$wrapper".toTypeName, tmpl) .withMods(Modifiers(Final)) .withSpan(Span(0, expr.length)) - PackageDef(Ident(nme.REPL_PACKAGE), List(wrapper)) + PackageDef(Ident(nme.EMPTY_PACKAGE), List(wrapper)) } ParseResult(sourceFile)(state) match { diff --git a/compiler/src/dotty/tools/repl/ScriptEngine.scala b/compiler/src/dotty/tools/repl/ScriptEngine.scala index 9c0ced8e5fa0..501dbd0263d7 100644 --- a/compiler/src/dotty/tools/repl/ScriptEngine.scala +++ b/compiler/src/dotty/tools/repl/ScriptEngine.scala @@ -37,7 +37,7 @@ class ScriptEngine extends AbstractScriptEngine { val vid = state.valIndex state = driver.run(script)(state) val oid = state.objectIndex - Class.forName(s"${nme.REPL_PACKAGE}.${str.REPL_SESSION_LINE}$oid", true, rendering.classLoader()(using state.context)) + Class.forName(s"${Rendering.REPL_WRAPPER_NAME_PREFIX}$oid", true, rendering.classLoader()(using state.context)) .getDeclaredMethods.find(_.getName == s"${str.REPL_RES_PREFIX}$vid") .map(_.invoke(null)) .getOrElse(null) diff --git a/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTest.scala b/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTest.scala index cc7d1eac2004..76f06802c79c 100644 --- a/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTest.scala +++ b/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTest.scala @@ -8,7 +8,7 @@ import org.junit.Assert._ trait ErrorMessagesTest extends DottyTest { - private def newContext = { + protected def newContext = { val rep = new StoreReporter(null) with UniqueMessagePositions with HideNonSensicalMessages initialCtx.setReporter(rep).setSetting(ctx.settings.color, "never") diff --git a/compiler/test/dotty/tools/repl/ShadowingBatchTests.scala b/compiler/test/dotty/tools/repl/ShadowingBatchTests.scala new file mode 100644 index 000000000000..bcfe1c3251a7 --- /dev/null +++ b/compiler/test/dotty/tools/repl/ShadowingBatchTests.scala @@ -0,0 +1,71 @@ +package dotty.tools +package repl + +import java.io.File +import java.nio.file.Files + +import org.junit.{ After, AfterClass, BeforeClass, Ignore, Test } +import org.junit.Assert._ +import io.{ Directory, PlainDirectory } +import dotc.core.Contexts._ +import dotc.reporting.{ ErrorMessagesTest, StoreReporter } +import vulpix.TestConfiguration + +object ShadowingBatchTests: + val dir = Directory(Files.createTempDirectory("batch-shadow")) + + @BeforeClass def suiteStarting: Unit = dir.createDirectory() + @AfterClass def suiteFinished: Unit = dir.deleteRecursively() + +class ShadowingBatchTests extends ErrorMessagesTest: + import ShadowingBatchTests._ + + @After def testFinished: Unit = dir.list.foreach(_.deleteRecursively()) + + val compiler = new dotc.Compiler() + + override def initializeCtx(ictx: FreshContext) = inContext(ictx) { + super.initializeCtx(ictx) + val settings = ictx.settings; import settings._ + ictx.setSetting(outputDir, new PlainDirectory(dir)) + ictx.setSetting(classpath, classpath.value + File.pathSeparator + dir.jpath.toAbsolutePath) + } + + @Test def file = + checkMessages("class C(val c: Int)").expectNoErrors + checkMessages("object rsline1 {\n def line1 = new C().c\n}").expect { (_, msgs) => + assertMessageCount(1, msgs) + assertEquals("missing argument for parameter c of constructor C in class C: (c: Int): C", msgs.head.message) + } + checkMessages("object rsline2 {\n def line2 = new C(13).c\n}").expectNoErrors + checkMessages("object rsline3 {\n class C { val c = 42 }\n}").expectNoErrors + checkMessages("import rsline3._\nobject rsline4 {\n def line4 = new C().c\n}").expectNoErrors + + @Test def directory = + checkMessages("package foo\nclass C").expectNoErrors + checkMessages("object rsline1 {\n def line1 = foo\n}").expect { (_, msgs) => + assertMessageCount(1, msgs) + assertEquals("package foo is not a value", msgs.head.message) + } + checkMessages("object rsline2 {\n val foo = 2\n}").expectNoErrors + checkMessages("import rsline2._\nobject rsline3 {\n def line3 = foo\n}").expectNoErrors + + @Test def directoryJava = + checkMessages("object rsline1 {\n def line1 = java\n}").expect { (_, msgs) => + assertMessageCount(1, msgs) + assertEquals("package java is not a value", msgs.head.message) + } + checkMessages("object rsline2 {\n val java = 2\n}").expectNoErrors + checkMessages("import rsline2._\nobject rsline3 {\n def line3 = java\n}").expectNoErrors + + def checkMessages(source: String): Report = + ctx = newContext + val run = compiler.newRun(using ctx.fresh) + run.compileFromStrings(List(source)) + val runCtx = run.runContext + if runCtx.reporter.hasErrors then + val rep = runCtx.reporter.asInstanceOf[StoreReporter] + val msgs = rep.removeBufferedMessages(using runCtx).map(_.msg).reverse + new Report(msgs, runCtx) + else new EmptyReport +end ShadowingBatchTests diff --git a/compiler/test/dotty/tools/repl/ShadowingTests.scala b/compiler/test/dotty/tools/repl/ShadowingTests.scala index 4284c44c58de..43c182b9ae5b 100644 --- a/compiler/test/dotty/tools/repl/ShadowingTests.scala +++ b/compiler/test/dotty/tools/repl/ShadowingTests.scala @@ -95,7 +95,6 @@ class ShadowingTests extends ReplTest(options = ShadowingTests.options): |""".stripMargin ) - @Ignore("not yet fixed") @Test def `shadow subdirectories on classpath` = // NB: Tests of shadowing of subdirectories on the classpath are only valid // when the subdirectories exist prior to initialization of the REPL driver. @@ -128,9 +127,6 @@ class ShadowingTests extends ReplTest(options = ShadowingTests.options): ShadowingTests.createSubDir("util") testScript(name = "", """|scala> import util.Try - |1 | import util.Try - | | ^^^ - | | value Try is not a member of util | |scala> object util { class Try { override def toString = "you've gotta try!" } } |// defined object util