-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix cyclicity error for self-referential enum case #24138
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
base: main
Are you sure you want to change the base?
Conversation
|
Generally, I think the problematic example should be accepted. If we look at the desugarings in the issue discussion, all of them (even the one that did not) compile in the current nightly. Also, if I try to manually approximate the compiler's desugaring after typer, it is accepted: object Desugaring:
sealed abstract class Opt[T]:
import Opt.Nn
def compareTo(nn: Nn.type) = 0
object Opt:
case object Nn extends Opt[Nothing] with Comparable[Nn.type] |
// No self-reference, let the type be inferred normally | ||
TypeTree() | ||
|
||
val vdef = ValDef(name, tpt, New(impl1)).withMods(mods.withAddedFlags(EnumValue, span)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we always add an explicit type without checking parent types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because we have a constructor, not a type when we desugar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best explicit annotation at this point seems to be the intersection of the parents, but only if all parent trees are known to be types. For example, all bets are off with constructor calls, because we have not yet inferred types.
// No self-reference, let the type be inferred normally | ||
TypeTree() | ||
|
||
val vdef = ValDef(name, tpt, New(impl1)).withMods(mods.withAddedFlags(EnumValue, span)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because we have a constructor, not a type when we desugar.
Fixes scala#11443 When an enum case references itself in a parent type (e.g., `case Nn extends Opt[Nothing] with Comparable[Nn.type]`), the compiler previously reported a cyclic reference error during type inference. The fix attempts to provide an explicit type annotation (the intersection of all parents) for parameterless enum cases early at desugaring time.
I've resorted to a different, simpler solution. Claude was still very useful in the process and helped me save time. |
I think the correct desugaring would be moving the anonymous class out from the rhs body: enum Opt[T]:
case Nn extends Opt[Nothing] with Comparable[Nn.type]
// currently
val Nn: Opt[Nothing] & Comparable[Nn.type] =
class $anno extends Opt[Nothing] with Comparable[Nn.type], EnumValue // error
new $anno
// to
val Nn: Nn$1 = new Nn$1()
final class Nn$1 extends Opt[Nothing] with Comparable[Nn.type], EnumValue |
This was a fun first experience including Claude to work on issue tickets. It's impressive how much it can understand and it helps you identify suitable places in the codebase. Also, it's quite good at explaining stuff about the compiler code (e.g., what's the purpose of a given tree type, etc.) Though in the end, I found myself playing a bit of whack-a-mole, and conclude that the Claud-proposed solution, while passing all tests, is too brittle and specific, and my other solution cannot be made to work with all of them. Interestingly, I asked it if a later phase after if mdef.isInstanceOf[ValDef] then
val vd = mdef.asInstanceOf[ValDef]
if vd.tpt.isEmpty && vd.mods.isAllOf(EnumValue) && sym.owner.exists then
vd.rhs match
case New(tpl: Template) if tpl.parents.nonEmpty =>
// Check if any parent references this enum case name
def containsNameRef(tree: untpd.Tree): Boolean = tree match
case untpd.Ident(n) if n == sym.name => true
case untpd.Select(qual, _) => containsNameRef(qual)
case untpd.Apply(fn, args) => containsNameRef(fn) || args.exists(containsNameRef)
case untpd.AppliedTypeTree(tpt, args) => containsNameRef(tpt) || args.exists(containsNameRef)
case untpd.SingletonTypeTree(ref) => containsNameRef(ref)
case _ => false
if tpl.parents.exists(containsNameRef) then
// Check if the first parent is the enum class (or an applied version of it)
// The enum owner might be a module class (Opt$) but the tree references the source name (Opt)
val enumSourceName = sym.owner.name.stripModuleClassSuffix
def isEnumParent(tree: untpd.Tree): Boolean = tree match
case untpd.Ident(n) => n == enumSourceName || n == sym.owner.name
case untpd.Select(_, n) => n == enumSourceName || n == sym.owner.name
case untpd.AppliedTypeTree(tpt, _) => isEnumParent(tpt)
case untpd.Apply(fn, _) => isEnumParent(fn)
case _ => false
if isEnumParent(tpl.parents.head) then
// Extract type arguments from the first parent and apply them to the enum class
def getTypeArgs(tree: untpd.Tree): List[untpd.Tree] = tree match
case untpd.AppliedTypeTree(_, args) => args
case untpd.Apply(fn, _) => getTypeArgs(fn)
case _ => Nil
val typeArgs = getTypeArgs(tpl.parents.head)
// Get the enum class (the companion class of the module)
val enumClass = if sym.owner.is(ModuleClass) then sym.owner.companionClass else sym.owner
if typeArgs.nonEmpty then
// Type the arguments and apply them to the enum class type
val typedArgs = typeArgs.map(typedAheadType(_).tpe)
return enumClass.typeRef.appliedTo(typedArgs)
else
return enumClass.typeRef
case _ => That is again in the spirit of the very first Claude solution and passes all tests, but it's unclear if it's really complete. As @odersky said: this just increases the entropy in the compiler. |
Good point. But are we free to just change it like that? Isn't the enum-desugaring somewhere spec-ed? |
The reason I propose this is from desugaring object: |
Fixes #11443
When an enum case references itself in a parent type (e.g.,
case Nn extends Opt[Nothing] with Comparable[Nn.type]
), thecompiler previously reported a cyclic reference error during type
inference.
The fix attempts to provide an explicit type annotation
(the intersection of all parents) for parameterless enum cases
early at desugaring time.