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

Use CyclicRef trace to pick sym in message #10680

Merged
merged 1 commit into from May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/compiler/scala/tools/nsc/Global.scala
Expand Up @@ -90,6 +90,8 @@ class Global(var currentSettings: Settings, reporter0: Reporter)

override def settings = currentSettings

override def isSymbolLockTracingEnabled = settings.cyclic.value

private[this] var currentReporter: FilteringReporter = null
locally { reporter = reporter0 }

Expand Down
1 change: 1 addition & 0 deletions src/compiler/scala/tools/nsc/settings/ScalaSettings.scala
Expand Up @@ -563,6 +563,7 @@ trait ScalaSettings extends StandardScalaSettings with Warnings { _: MutableSett
*/
val Vhelp = BooleanSetting("-V", "Print a synopsis of verbose options.")
val browse = PhasesSetting("-Vbrowse", "Browse the abstract syntax tree after") withAbbreviation "-Ybrowse"
val cyclic = BooleanSetting("-Vcyclic", "Debug cyclic reference error.")
val debug = BooleanSetting("-Vdebug", "Increase the quantity of debugging output.") withAbbreviation "-Ydebug" withPostSetHook (s => if (s.value) StatisticsStatics.enableDebugAndDeoptimize())
val YdebugTasty = BooleanSetting("-Vdebug-tasty", "Increase the quantity of debugging output when unpickling tasty.") withAbbreviation "-Ydebug-tasty"
val VdebugTypeError = BooleanSetting("-Vdebug-type-error", "Print the stack trace when any error is caught.") withAbbreviation "-Ydebug-type-error"
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/scala/tools/nsc/typechecker/ContextErrors.scala
Expand Up @@ -1340,7 +1340,7 @@ trait ContextErrors extends splain.SplainErrors {

def TypeSigError(tree: Tree, ex: TypeError) = {
ex match {
case CyclicReference(_, _) if tree.symbol.isTermMacro =>
case CyclicReference(_, _, _) if tree.symbol.isTermMacro =>
// say, we have a macro def `foo` and its macro impl `impl`
// if impl: 1) omits return type, 2) has anything implicit in its body, 3) sees foo
//
Expand All @@ -1353,8 +1353,8 @@ trait ContextErrors extends splain.SplainErrors {
// hence we (together with reportTypeError in TypeDiagnostics) make sure that this CyclicReference
// evades all the handlers on its way and successfully reaches `isCyclicOrErroneous` in Implicits
throw ex
case CyclicReference(sym, info: TypeCompleter) =>
issueNormalTypeError(tree, typer.cyclicReferenceMessage(sym, info.tree) getOrElse ex.getMessage)
case CyclicReference(sym, info: TypeCompleter, trace) =>
issueNormalTypeError(tree, typer.cyclicReferenceMessage(sym, info.tree, trace, tree.pos).getOrElse(ex.getMessage))
case _ =>
contextNamerErrorGen.issue(TypeErrorWithUnderlyingTree(tree, ex))
}
Expand Down
52 changes: 29 additions & 23 deletions src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala
Expand Up @@ -840,19 +840,29 @@ trait TypeDiagnostics extends splain.SplainDiagnostics {
def permanentlyHiddenWarning(pos: Position, hidden: Name, defn: Symbol) =
context.warning(pos, "imported `%s` is permanently hidden by definition of %s".format(hidden, defn.fullLocationString), WarningCategory.OtherShadowing)

private def symWasOverloaded(sym: Symbol) = sym.owner.isClass && sym.owner.info.member(sym.name).isOverloaded
private def cyclicAdjective(sym: Symbol) = if (symWasOverloaded(sym)) "overloaded" else "recursive"

/** Returns Some(msg) if the given tree is untyped apparently due
* to a cyclic reference, and None otherwise.
*/
def cyclicReferenceMessage(sym: Symbol, tree: Tree) = condOpt(tree) {
case ValDef(_, _, TypeTree(), _) => s"recursive $sym needs type"
case DefDef(_, _, _, _, TypeTree(), _) => s"${cyclicAdjective(sym)} $sym needs result type"
case Import(expr, selectors) =>
"""encountered unrecoverable cycle resolving import.
|Note: this is often due in part to a class depending on a definition nested within its companion.
|If applicable, you may wish to try moving some members into another object.""".stripMargin
def cyclicReferenceMessage(sym: Symbol, tree: Tree, trace: Array[Symbol], pos: Position) = {
def symWasOverloaded(sym: Symbol) = sym.owner.isClass && sym.owner.info.member(sym.name).isOverloaded
def cyclicAdjective(sym: Symbol) = if (symWasOverloaded(sym)) "overloaded" else "recursive"

val badsym = if (!sym.isSynthetic) sym else {
val organics = trace.filter(!_.isSynthetic)
if (organics.length == 0) sym
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding something to the error message here? $sym is synthetic; use -Vcyclic to find which definition needs an explicit type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently, I followed that suggestion.

else if (organics.length == 1) organics(0)
else organics.find(_.pos.focus == pos.focus).getOrElse(organics(0))
}
def help = if (!badsym.isSynthetic || settings.cyclic.value) "" else
s"; $badsym is synthetic; use -Vcyclic to find which definition needs an explicit type"
condOpt(tree) {
case ValDef(_, _, TypeTree(), _) => s"recursive $badsym needs type$help"
case DefDef(_, _, _, _, TypeTree(), _) => s"${cyclicAdjective(badsym)} $badsym needs result type$help"
case Import(_, _) =>
sm"""encountered unrecoverable cycle resolving import.
|Note: this is often due in part to a class depending on a definition nested within its companion.
|If applicable, you may wish to try moving some members into another object."""
}
}

// warn about class/method/type-members' type parameters that shadow types already in scope
Expand Down Expand Up @@ -887,20 +897,16 @@ trait TypeDiagnostics extends splain.SplainDiagnostics {
if (settings.isDebug) ex.printStackTrace()

ex match {
case CyclicReference(sym, info: TypeCompleter) =>
if (context0.owner.isTermMacro) {
// see comments to TypeSigError for an explanation of this special case
throw ex
} else {
val pos = info.tree match {
case Import(expr, _) => expr.pos
case _ => ex.pos
}
context0.error(pos, cyclicReferenceMessage(sym, info.tree) getOrElse ex.getMessage())

if (sym == ObjectClass)
throw new FatalError("cannot redefine root "+sym)
// see comments to TypeSigError for an explanation of this special case
case _: CyclicReference if context0.owner.isTermMacro => throw ex
case CyclicReference(sym, info: TypeCompleter, trace) =>
val pos = info.tree match {
case Import(expr, _) => expr.pos
case _ => ex.pos
}
context0.error(pos, cyclicReferenceMessage(sym, info.tree, trace, pos).getOrElse(ex.getMessage))

if (sym == ObjectClass) throw new FatalError(s"cannot redefine root $sym")
case _ =>
context0.error(ex.pos, ex.msg)
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/Typers.scala
Expand Up @@ -6306,7 +6306,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
case ex: TypeError =>
tree.clearType()
// The only problematic case are (recoverable) cyclic reference errors which can pop up almost anywhere.
typingStack.printTyping(tree, "caught %s: while typing %s".format(ex, tree)) //DEBUG
typingStack.printTyping(tree, s"caught $ex: while typing $tree")
reportTypeError(context, tree.pos, ex)
setError(tree)
case ex: Exception =>
Expand Down
2 changes: 2 additions & 0 deletions src/reflect/scala/reflect/internal/SymbolTable.scala
Expand Up @@ -95,6 +95,8 @@ abstract class SymbolTable extends macros.Universe

def settings: MutableSettings

def isSymbolLockTracingEnabled: Boolean = isDeveloper

/** Override with final implementation for inlining. */
def debuglog(msg: => String): Unit = if (settings.isDebug) log(msg)

Expand Down
75 changes: 46 additions & 29 deletions src/reflect/scala/reflect/internal/Symbols.scala
Expand Up @@ -18,12 +18,12 @@ package scala
package reflect
package internal

import scala.collection.immutable
import scala.collection.mutable.ListBuffer
import util.{ ReusableInstance, Statistics, shortClassOfInstance }
import Flags._
import scala.annotation.tailrec
import scala.collection.mutable.{ArrayBuffer, ListBuffer}
import scala.reflect.io.{AbstractFile, NoAbstractFile}

import util.{ReusableInstance, Statistics, shortClassOfInstance}
import Flags._
import Variance._

trait Symbols extends api.Symbols { self: SymbolTable =>
Expand All @@ -36,14 +36,16 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
protected def nextId() = { ids += 1; ids }

/** Used to keep track of the recursion depth on locked symbols */
private[this] var _recursionTable = immutable.Map.empty[Symbol, Int]
private[this] var _recursionTable = Map.empty[Symbol, Int]
def recursionTable = _recursionTable
def recursionTable_=(value: immutable.Map[Symbol, Int]) = _recursionTable = value
def recursionTable_=(value: Map[Symbol, Int]) = _recursionTable = value

private[this] var _lockedCount = 0
def lockedCount = this._lockedCount
def lockedCount_=(i: Int) = _lockedCount = i

private[this] val _lockingTrace = ArrayBuffer.empty[Symbol]
private[this] val lockTracing: Boolean = self.isSymbolLockTracingEnabled

@deprecated("Global existential IDs no longer used", "2.12.1")
private[this] var existentialIds = 0
Expand Down Expand Up @@ -553,19 +555,23 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
// True if the symbol is unlocked.
// True if the symbol is locked but still below the allowed recursion depth.
// False otherwise
private[scala] def lockOK: Boolean = {
((_rawflags & LOCKED) == 0L) ||
((settings.Yrecursion.value != 0) &&
(recursionTable get this match {
case Some(n) => (n <= settings.Yrecursion.value)
case None => true }))
}
private[scala] def lockOK: Boolean = (
(_rawflags & LOCKED) == 0L || {
val limit = settings.Yrecursion.value
limit != 0 && (
recursionTable.get(this) match {
case Some(n) => n <= limit
case None => true
})
}
)

// Lock a symbol, using the handler if the recursion depth becomes too great.
private[scala] def lock(handler: => Unit): Boolean = {
if (lockTracing) _lockingTrace.addOne(this)
if ((_rawflags & LOCKED) != 0L) {
if (settings.Yrecursion.value != 0) {
recursionTable get this match {
recursionTable.get(this) match {
case Some(n) =>
if (n > settings.Yrecursion.value) {
handler
Expand All @@ -578,21 +584,25 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
recursionTable += (this -> 1)
true
}
} else { handler; false }
} else {
handler
false
}
} else {
_rawflags |= LOCKED
true
}
}

// Unlock a symbol
private[scala] def unlock() = {
private[scala] def unlock(): Unit =
if ((_rawflags & LOCKED) != 0L) {
_rawflags &= ~LOCKED
if (lockTracing && !_lockingTrace.isEmpty)
_lockingTrace.remove(index = _lockingTrace.size - 1, count = 1) // dropRightInPlace(1)
if (settings.Yrecursion.value != 0)
recursionTable -= this
}
}

// ----- tests ----------------------------------------------------------------------

Expand Down Expand Up @@ -1553,9 +1563,16 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
if ((_rawflags & LOCKED) != 0L) { // rolled out once for performance
lock {
setInfo(ErrorType)
throw CyclicReference(this, tp)
val trace =
if (lockTracing) {
val t = _lockingTrace.toArray
_lockingTrace.clear()
t
} else CyclicReference.emptyTrace
throw CyclicReference(this, tp, trace)
}
} else {
if (lockTracing) _lockingTrace.addOne(this)
_rawflags |= LOCKED
}
val current = phase
Expand All @@ -1568,18 +1585,15 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
unlock()
phase = current
}
} else {
// In runtime reflection, there is only on phase, so don't mutate Global.phase which would lead to warnings
// of data races from when using TSAN to assess thread safety.
try {
tp.complete(this)
} finally {
unlock()
}
}
else
// In runtime reflection, there is only one phase, so don't mutate Global.phase
// which would lead to warnings of data races from when using TSAN to assess thread safety.
try tp.complete(this)
finally unlock()
} catch {
case ex: CyclicReference =>
devWarning("... hit cycle trying to complete " + this.fullLocationString)
devWarning(s"... hit cycle trying to complete $fullLocationString")
throw ex
}

Expand Down Expand Up @@ -3839,10 +3853,13 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
else closestEnclMethod(from.owner)

/** An exception for cyclic references of symbol definitions */
case class CyclicReference(sym: Symbol, info: Type)
extends TypeError("illegal cyclic reference involving " + sym) {
case class CyclicReference(sym: Symbol, info: Type, trace: Array[Symbol] = CyclicReference.emptyTrace)
extends TypeError(s"illegal cyclic reference involving $sym") {
if (settings.isDebug) printStackTrace()
}
object CyclicReference {
val emptyTrace: Array[Symbol] = Array.empty[Symbol]
}

/** A class for type histories */
private final case class TypeHistory protected (private var _validFrom: Period, private var _info: Type, private var _prev: TypeHistory) {
Expand Down
4 changes: 4 additions & 0 deletions test/files/neg/t7808.check
@@ -0,0 +1,4 @@
t7808.scala:5: error: recursive value ls needs type
val (ls, rs) = z match {
^
1 error
18 changes: 18 additions & 0 deletions test/files/neg/t7808.scala
@@ -0,0 +1,18 @@
//> using options -Vcyclic
class C {
type OI = Option[Int]
def f(z: OI, ls: List[OI], rs: List[OI]): (List[OI], List[OI]) = {
val (ls, rs) = z match {
case Some(_) => (z::ls, rs)
case _ => (ls, z::rs)
}
(ls, rs)
}
}

/*
t7808.scala:5: error: recursive value x$1 needs type
val (ls, rs) = z match {
^
1 error
*/
4 changes: 4 additions & 0 deletions test/files/neg/t7808b.check
@@ -0,0 +1,4 @@
t7808b.scala:5: error: recursive value x$1 needs type; value x$1 is synthetic; use -Vcyclic to find which definition needs an explicit type
val (ls, rs) = z match {
^
1 error
18 changes: 18 additions & 0 deletions test/files/neg/t7808b.scala
@@ -0,0 +1,18 @@

class C {
type OI = Option[Int]
def f(z: OI, ls: List[OI], rs: List[OI]): (List[OI], List[OI]) = {
val (ls, rs) = z match {
case Some(_) => (z::ls, rs)
case _ => (ls, z::rs)
}
(ls, rs)
}
}

/*
t7808.scala:5: error: recursive value x$1 needs type
val (ls, rs) = z match {
^
1 error
*/