-
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
Conversation
(削除) I'd like to point out that this fix was 100% generated by Claude and passes all compilation tests locally. Let's see if it passes a human review. (削除ここまで)
Claude was used for the initial version of this fix, and I was quite impressed that it identified the correct place and a working bugfix (that was not quite satisfactory in the end) on its own.
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]
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.
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.
17876ff to
f50529f
Compare
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 Nn1ドル 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 DesugarEnums could be used instead, and it suggested on its own inferredResultType in Namer, with the following addition
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.
I think the correct desugaring would be moving the anonymous class out from the rhs body:
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: object A extends F[A.type]. Maybe someone else can comment on the spec.
Uh oh!
There was an error while loading. Please reload this page.
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.