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.

Sign up for GitHub

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

Break circularity in TypeBounds with LazyRef wraps #14288

Closed
wants to merge 1 commit into from
Closed

Break circularity in TypeBounds with LazyRef wraps #14288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,20 @@ trait ConstraintHandling {
* of some param when comparing types might lead to infinite recursion. Consider `bounds` instead.
*/
def fullBounds(param: TypeParamRef)(using Context): TypeBounds =
nonParamBounds(param).derivedTypeBounds(fullLowerBound(param), fullUpperBound(param))
Copy link
Member

Choose a reason for hiding this comment

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

This is likely still not the correct place to fix this, constraints shouldn't be cyclic (although the assert for this is disabled by default for performance: https://github.com/lampepfl/dotty/blob/3c8200671995ea75b97b493081cc4e51109cfd04/compiler/src/dotty/tools/dotc/config/Config.scala#L21-L24), it's possible the logic in GadtConstraint.scala violates this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll check.

Copy link
Member Author

@dwijnand dwijnand Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Enabling doesn't seem to trigger anything. Either it fails without my "half answer" workaround or it passes with - no check error. Are we sure it's a cycle in the constraints? Need to investigate more.

Copy link
Member Author

Choose a reason for hiding this comment

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

So inference first creates the type refs while typing the patterns (in maximizeType via typedUnApply) as F$1 <: F and F$2 <: F$1. Just a little later runs over the patterns again in indexPattern where it calls fullBounds leading to F$1(param)1 >: F$2 <: F and F$2(param)1 <: F$1.

So is it circular constraints? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, either gadt.fullBounds should not produce cycles like this, or we shouldn't use it here, wdyt @abgruszecki ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I see it, fullLowerBound aren't wrong in their answers. But put together they lead to a mutual reference. So fixing this in fullBounds by inspecting the results seems reasonable to me.

Hopefully I've fixed the test suite now.

Copy link
Member

Choose a reason for hiding this comment

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

But can this be triggered with non-gadt constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. 🤷🏼‍♂️ But GadtConstraint redefines its fullLowerBound and fullUpperBound so possibly not?

val lo = wrapRecursiveInLazy(fullLowerBound(param))
val hi = wrapRecursiveInLazy(fullUpperBound(param))
nonParamBounds(param).derivedTypeBounds(lo, hi)

def wrapRecursiveInLazy(tp: Type)(using Context) =
if IsRecursiveAccumulator()(false, tp) then LazyRef.of(tp)
else tp

class IsRecursiveAccumulator(using Context) extends TypeAccumulator[Boolean]:
private val seen = scala.collection.mutable.HashSet.empty[Type]
def apply(x: Boolean, tp: Type): Boolean = x || tp.match
case tp: LazyRef => false
case tp @ TypeRef(NoPrefix, _) => if seen.add(tp) then apply(false, tp.info) else true
case _ => foldOver(false, tp)

/** An approximating map that prevents types nested deeper than maxLevel as
* well as WildcardTypes from leaking into the constraint.
Expand Down
15 changes: 15 additions & 0 deletions tests/pos-deep-subtype/i14287.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
enum Free[+F[_], A]:
case Return(a: A)
case Suspend(s: F[A])
case FlatMap[F[_], A, B](
s: Free[F, A],
f: A => Free[F, B]) extends Free[F, B]

def flatMap[F2[x] >: F[x], B](f: A => Free[F2,B]): Free[F2,B] =
FlatMap(this, f)

@annotation.tailrec
final def step: Free[F, A] = this match
case FlatMap(FlatMap(fx, f), g) => fx.flatMap(x => f(x).flatMap(y => g(y))).step
case FlatMap(Return(x), f) => f(x).step
case _ => this
6 changes: 6 additions & 0 deletions tests/pos/i14287.min.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
enum Foo[+H[_]]:
case Bar[F[_]](f: Foo[F]) extends Foo[F]

def test: Foo[H] = this match
case Bar(Bar(f)) => Bar(f)
case _ => this