From e300188eb4a2760bef91cdd75b22b4c72124f5de Mon Sep 17 00:00:00 2001 From: odersky Date: Sun, 17 Dec 2023 19:51:52 +0100 Subject: [PATCH 1/6] Avoid diagnostic message forcing crashing the compiler (#19113) Using issue #18650 as the reference (but issue #18999 is another option) building on the fix in PR #18719 (refined in PR #18727) as well as the fix in PR #18760, I'm trying to make a more root change here by making sure that message forcing only occurs with `hasErrors`/`errorsReported` is true, so as to avoid assertion errors crashing the compiler. (cherry picked from commit 10f2c101b7a4863bad6325ded98cf2a0d810dca0) --- compiler/src/dotty/tools/dotc/report.scala | 1 + .../tools/dotc/reporting/HideNonSensicalMessages.scala | 6 +++--- compiler/src/dotty/tools/dotc/reporting/Reporter.scala | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/report.scala b/compiler/src/dotty/tools/dotc/report.scala index 75261fb6890e..3f3435347dbd 100644 --- a/compiler/src/dotty/tools/dotc/report.scala +++ b/compiler/src/dotty/tools/dotc/report.scala @@ -158,6 +158,7 @@ object report: | An unhandled exception was thrown in the compiler. | Please file a crash report here: | https://github.com/lampepfl/dotty/issues/new/choose + | For non-enriched exceptions, compile with -Yno-enrich-error-messages. | |$info1 |""".stripMargin diff --git a/compiler/src/dotty/tools/dotc/reporting/HideNonSensicalMessages.scala b/compiler/src/dotty/tools/dotc/reporting/HideNonSensicalMessages.scala index 9b6a3c75ba5d..74cc6b99d2e6 100644 --- a/compiler/src/dotty/tools/dotc/reporting/HideNonSensicalMessages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/HideNonSensicalMessages.scala @@ -13,8 +13,8 @@ trait HideNonSensicalMessages extends Reporter { */ override def isHidden(dia: Diagnostic)(using Context): Boolean = super.isHidden(dia) || { - dia.msg.isNonSensical && - hasErrors && // if there are no errors yet, report even if diagnostic is non-sensical - !ctx.settings.YshowSuppressedErrors.value + hasErrors // if there are no errors yet, report even if diagnostic is non-sensical + && dia.msg.isNonSensical // defer forcing the message by calling hasErrors first + && !ctx.settings.YshowSuppressedErrors.value } } diff --git a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala b/compiler/src/dotty/tools/dotc/reporting/Reporter.scala index f5aadac27296..5a692749669b 100644 --- a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala +++ b/compiler/src/dotty/tools/dotc/reporting/Reporter.scala @@ -154,8 +154,6 @@ abstract class Reporter extends interfaces.ReporterResult { case w: Warning if ctx.settings.XfatalWarnings.value => w.toError case _ => dia if !isHidden(d) then // avoid isHidden test for summarized warnings so that message is not forced - markReported(d) - withMode(Mode.Printing)(doReport(d)) d match { case _: Warning => _warningCount += 1 case e: Error => @@ -166,6 +164,8 @@ abstract class Reporter extends interfaces.ReporterResult { case _: Info => // nothing to do here // match error if d is something else } + markReported(dia) + withMode(Mode.Printing)(doReport(dia)) end issueUnconfigured def issueIfNotSuppressed(dia: Diagnostic)(using Context): Unit = From 54fbe32a7e1aefa430f0b1787eb1eca51f071ac6 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 21 Feb 2024 17:51:37 +0000 Subject: [PATCH 2/6] Test case for REPL bad symbolic reference --- .../test/dotty/tools/repl/ReplCompilerTests.scala | 14 ++++++++++++++ compiler/test/dotty/tools/repl/ReplTest.scala | 4 ++++ .../test/dotty/tools/repl/TabcompleteTests.scala | 4 ---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala index ecdfeb512e1b..41c0b4c44568 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -353,6 +353,20 @@ class ReplCompilerTests extends ReplTest: @Test def `i13097 expect template after colon` = contextually: assert(ParseResult.isIncomplete("class C:")) + @Test def i15562: Unit = initially { + val s1 = run("List(1, 2).filter(_ % 2 == 0).foreach(println)") + assertEquals("2", storedOutput().trim) + s1 + } andThen { s1 ?=> + val comp = tabComplete("List(1, 2).filter(_ % 2 == 0).fore") + assertEquals(List("foreach"), comp.distinct) + s1 + } andThen { + val s2 = run("List(1, 2).filter(_ % 2 == 0).foreach(println)") + assertEquals("2", storedOutput().trim) + s2 + } + object ReplCompilerTests: private val pattern = Pattern.compile("\\r[\\n]?|\\n"); diff --git a/compiler/test/dotty/tools/repl/ReplTest.scala b/compiler/test/dotty/tools/repl/ReplTest.scala index 34cad747fde6..6b97c72e1cb7 100644 --- a/compiler/test/dotty/tools/repl/ReplTest.scala +++ b/compiler/test/dotty/tools/repl/ReplTest.scala @@ -40,6 +40,10 @@ extends ReplDriver(options, new PrintStream(out, true, StandardCharsets.UTF_8.na def contextually[A](op: Context ?=> A): A = op(using initialState.context) + /** Returns the `(, )`*/ + def tabComplete(src: String)(implicit state: State): List[String] = + completions(src.length, src, state).map(_.value).sorted + extension [A](state: State) infix def andThen(op: State ?=> A): A = op(using state) diff --git a/compiler/test/dotty/tools/repl/TabcompleteTests.scala b/compiler/test/dotty/tools/repl/TabcompleteTests.scala index 910584a9b5e7..359114aee941 100644 --- a/compiler/test/dotty/tools/repl/TabcompleteTests.scala +++ b/compiler/test/dotty/tools/repl/TabcompleteTests.scala @@ -8,10 +8,6 @@ import org.junit.Test /** These tests test input that has proved problematic */ class TabcompleteTests extends ReplTest { - /** Returns the `(, )`*/ - private def tabComplete(src: String)(implicit state: State): List[String] = - completions(src.length, src, state).map(_.value).sorted - @Test def tabCompleteList = initially { val comp = tabComplete("List.r") assertEquals(List("range"), comp.distinct) From bb89b168062eddf7175df1a8c4a59e245824166e Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 21 Feb 2024 22:08:06 +0000 Subject: [PATCH 3/6] Add a test case which still fails --- .../test/dotty/tools/repl/ReplCompilerTests.scala | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala index 41c0b4c44568..63da0e277bb4 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -367,6 +367,20 @@ class ReplCompilerTests extends ReplTest: s2 } + @Test def i15562b: Unit = initially { + val s1 = run("List(1, 2).filter(_ % 2 == 0).foreach(println)") + assertEquals("2", storedOutput().trim) + s1 + } andThen { s1 ?=> + val comp = tabComplete("val x = false + true; List(1, 2).filter(_ % 2 == 0).fore") + assertEquals(List("foreach"), comp.distinct) + s1 + } andThen { + val s2 = run("List(1, 2).filter(_ % 2 == 0).foreach(println)") + assertEquals("2", storedOutput().trim) + s2 + } + object ReplCompilerTests: private val pattern = Pattern.compile("\\r[\\n]?|\\n"); From 493a216242aac67685f48dda8d37e4125f306d96 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Fri, 23 Feb 2024 17:22:37 +0000 Subject: [PATCH 4/6] Avoid losing the symbols denotation on update --- compiler/src/dotty/tools/dotc/core/Symbols.scala | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index 7cad8bd0e250..90108a15a816 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -110,16 +110,23 @@ object Symbols { private def computeDenot(lastd: SymDenotation)(using Context): SymDenotation = { util.Stats.record("Symbol.computeDenot") val now = ctx.period + val prev = checkedPeriod checkedPeriod = now - if (lastd.validFor contains now) lastd else recomputeDenot(lastd) + if lastd.validFor.contains(now) then + lastd + else + val newd = recomputeDenot(lastd) + if newd.exists then + lastDenot = newd + else + checkedPeriod = prev + newd } /** Overridden in NoSymbol */ protected def recomputeDenot(lastd: SymDenotation)(using Context): SymDenotation = { util.Stats.record("Symbol.recomputeDenot") - val newd = lastd.current.asInstanceOf[SymDenotation] - lastDenot = newd - newd + lastd.current.asSymDenotation } /** The original denotation of this symbol, without forcing anything */ From 57c4127df52188b9abcbbc6413dae439c4dbfed6 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 26 Feb 2024 13:59:52 +0100 Subject: [PATCH 5/6] Some tweaks to denotation updates - Be more specific when we go into the special case of not updating checkedPeriod - Always update lastDenotation. - Keep computeDenot small, move work to recomputeDenot (cherry picked from commit e79b2e99800d2d06696e04192a63ce0e6f22bff8) --- .../src/dotty/tools/dotc/core/Symbols.scala | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index 90108a15a816..437f579ed972 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -108,25 +108,28 @@ object Symbols { } private def computeDenot(lastd: SymDenotation)(using Context): SymDenotation = { + // Written that way so that it comes in at 32 bytes and is therefore inlineable for + // the JIT (reputedly, cutoff is at 35 bytes) util.Stats.record("Symbol.computeDenot") val now = ctx.period - val prev = checkedPeriod checkedPeriod = now - if lastd.validFor.contains(now) then - lastd - else - val newd = recomputeDenot(lastd) - if newd.exists then - lastDenot = newd - else - checkedPeriod = prev - newd + if lastd.validFor.contains(now) then lastd else recomputeDenot(lastd) } /** Overridden in NoSymbol */ protected def recomputeDenot(lastd: SymDenotation)(using Context): SymDenotation = { util.Stats.record("Symbol.recomputeDenot") - lastd.current.asSymDenotation + val newd = lastd.current.asInstanceOf[SymDenotation] + lastDenot = newd + if !newd.exists && lastd.initial.validFor.firstPhaseId > ctx.phaseId then + // We are trying to bring forward a symbol that is defined only at a later phase + // (typically, a nested Java class, invisible before erasure). + // In that case, keep the checked period to the previous validity, which + // means we will try another bring forward when the symbol is referenced + // at a later phase. Otherwise we'd get stuck on NoDenotation here. + // See #15562 and test i15562b in ReplCompilerTests + checkedPeriod = lastd.initial.validFor + newd } /** The original denotation of this symbol, without forcing anything */ @@ -761,7 +764,7 @@ object Symbols { cls: ClassSymbol, name: TermName = nme.WILDCARD, selfInfo: Type = NoType)(using Context): TermSymbol = - newSymbol(cls, name, SelfSymFlags, selfInfo orElse cls.classInfo.selfType, coord = cls.coord) + newSymbol(cls, name, SelfSymFlags, selfInfo.orElse(cls.classInfo.selfType), coord = cls.coord) /** Create new type parameters with given owner, names, and flags. * @param boundsFn A function that, given type refs to the newly created @@ -928,7 +931,7 @@ object Symbols { */ def getPackageClassIfDefined(path: PreName)(using Context): Symbol = staticRef(path.toTypeName, isPackage = true, generateStubs = false) - .disambiguate(_ is PackageClass).symbol + .disambiguate(_.is(PackageClass)).symbol def requiredModule(path: PreName)(using Context): TermSymbol = { val name = path.toTermName From fb18037f32aa83039b32b45207ef62e28df14e54 Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 26 Feb 2024 15:58:30 +0100 Subject: [PATCH 6/6] Avoid setting lastDenot to NoDenotation in the forward reference scenario This brings back an element of the original solution. (cherry picked from commit 5631d76f247c3d02e21eeab135b7917f30f21394) --- compiler/src/dotty/tools/dotc/core/Symbols.scala | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index 437f579ed972..4359ee4046d9 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -120,15 +120,16 @@ object Symbols { protected def recomputeDenot(lastd: SymDenotation)(using Context): SymDenotation = { util.Stats.record("Symbol.recomputeDenot") val newd = lastd.current.asInstanceOf[SymDenotation] - lastDenot = newd - if !newd.exists && lastd.initial.validFor.firstPhaseId > ctx.phaseId then + if newd.exists || lastd.initial.validFor.firstPhaseId <= ctx.phaseId then + lastDenot = newd + else // We are trying to bring forward a symbol that is defined only at a later phase // (typically, a nested Java class, invisible before erasure). - // In that case, keep the checked period to the previous validity, which - // means we will try another bring forward when the symbol is referenced - // at a later phase. Otherwise we'd get stuck on NoDenotation here. + // In that case, keep lastDenot as it was and set the checked period to lastDenot's + // previous validity, which means we will try another bring forward when the symbol + // is referenced at a later phase. Otherwise we'd get stuck on NoDenotation here. // See #15562 and test i15562b in ReplCompilerTests - checkedPeriod = lastd.initial.validFor + checkedPeriod = lastd.validFor newd }