-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix infinite loop in Mirror synthesis of unreducible match type #20133
Conversation
This regressed in f7e2e7c (present in 3.4.0).
@@ -379,7 +379,7 @@ class Synthesizer(typer: Typer)(using @constructorOnly c: Context): | |||
// avoid type aliases for tuples | |||
Right(MirrorSource.GenericTuple(types)) | |||
case _ => reduce(tp.underlying) | |||
case tp: MatchType => reduce(tp.normalized) | |||
case tp: MatchType => reduce(tp.tryNormalize.orElse(tp.superType)) |
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.
This is actually not quite correct, if I change the definition of DoesNotReduce to:
type DoesNotReduce[T] = T match
case String => T
Then it infers an upper-bound of T
and tp.superType
ends up being Option[Int]
and so a mirror is found when none should be found. This seems like a more general problem with superType
though, so maybe something we should address after fixing this regression.
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.
Or perhaps we should never summon the mirror if the match type cannot be reduced? Maybe other situations where we use superType
are actually fine. /cc @sjrd
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.
I agree using superType
looks a bit suspicious given that Mirror.Of[_]
isn't covariant.
But then again, it's also used in the catch all right below.
case tp: MatchType => reduce(tp.tryNormalize.orElse(tp.superType)) | ||
case tp: MatchType => | ||
val n = tp.tryNormalize | ||
if n.exists then reduce(n) else Left(i"its subpart `$tp` is an unreducible match 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.
I think we should return the unreduced match type instead.
An instance could have been provided manually for it.
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.
reduce
requires that we return a class symbol it seems. Also the Synthesizer logic should only be used as a fallback if an implicit wasn't found by regular search.
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.
Ah good point, then an error sounds fine to me.
This regressed in f7e2e7c (present in 3.4.0).
The second commit fixes a soundness bug.