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

Rework MatchType recursion in collectParts #19867

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Mar 4, 2024

This patch fixes a recursion situation in collectParts, by
reimplementing a previous fix. Recursion is already attempted to be
cutoff with partSeen, and handleRecursion is also used to prevent
any unhandled recursion situations (like the one fixed here) from
crashing the compiler.

For context, AppliedType aren't hash-consed (i.e. the flyweight pattern)
which means that every time you apply the same type arguments to the
same type constructor you get a fresh AppliedType. Using i18171 as an
example, the sequence of events where this matters is:

  1. When typing BAZ, so much earlier than the collectParts call, because
    the MatchType on the rhs of BAZ reduces at definition site, the RHS
    is reduced to the DFVal[BAZREC[T]], which means that BAZ's info is
    a TypeAlias rather than a MatchAlias, meaning it can dealias.
  2. BAZREC[Any] is extracted by MatchType.InDisguise, which applies the
    Any to return a fresh MatchType
  3. Tuple.Map[Any, BAZ] is also extracted by MatchType.InDisguise,
    which returns its own fresh MatchType
  4. BAZ[h] dealiases to a fresh DFVal[BAZREC[h]] instance (see step 0)
  5. BAZREC[h] is extracted by MatchType.InDisguise, repeating the cycle

The reason that the cases with MatchType.InDisguise and MatchType were
introduced is i17395. Looking back, it seems the only need is to
capture any parts that are in the reduction of an applied MatchType.
With this patch applied in the case of i18171, this now cuts off
quickly, as BAZREC[Any] doesn't reduce to anything. In the case of
i17395, ExtractPart[ValuePartHolder] reduces to Value, so Value is
successfully recorded as a part.

Fixes #19857
Fixes #18171

This patch fixes a recursion situation in collectParts, by
reimplementing a previous fix.  Recursion is already attempted to be
cutoff with `partSeen`, and `handleRecursion` is also used to prevent
any unhandled recursion situations (like the one fixed here) from
crashing the compiler.

For context, AppliedType aren't hash-consed (i.e. the flyweight pattern)
which means that every time you apply the same type arguments to the
same type constructor you get a fresh AppliedType.  Using i18171 as an
example, the sequence of events where this matters is:

0. When typing BAZ, so much earlier than the collectParts call, because
   the MatchType on the rhs of BAZ reduces at definition site, the RHS
   is reduced to the `DFVal[BAZREC[T]]`, which means that BAZ's info is
   a TypeAlias rather than a MatchAlias, meaning it can dealias.
1. `BAZREC[Any]` is extracted by MatchType.InDisguise, which applies the
   Any to return a fresh MatchType
2. `Tuple.Map[Any, BAZ]` is also extracted by MatchType.InDisguise,
   which returns its own fresh MatchType
3. `BAZ[h]` dealiases to a fresh `DFVal[BAZREC[h]]` instance (see step 0)
4. `BAZREC[h]` is extracted by MatchType.InDisguise, repeating the cycle

The reason that the cases with MatchType.InDisguise and MatchType were
introduced is i17395.  Looking back, it seems the only need is to
capture any parts that are in the reduction of an applied MatchType.
With this patch applied in the case of i18171, this now cuts off
quickly, as `BAZREC[Any]` doesn't reduce to anything.  In the case of
i17395, `ExtractPart[ValuePartHolder]` reduces to `Value`, so `Value` is
successfully recorded as a part.
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

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

I'm not sure what you meant about the AppliedTypes not being hash-consed, we have the same problem with recursive match types in collectParts either way right ?

Otherwise looks good to me

@dwijnand
Copy link
Member Author

dwijnand commented Mar 6, 2024

I'm not sure what you meant about the AppliedTypes not being hash-consed, we have the same problem with recursive match types in collectParts either way right ?

We do have the problem either way. I was just providing it as context, for why we've got the problem, despite partSeen, which stores every seen type. To extend that context: as soon as there's a LazyRef, it breaks the caching, as LazyRef's aren't cached.

@dwijnand dwijnand merged commit cf968a3 into scala:main Mar 6, 2024
19 checks passed
@odersky
Copy link
Contributor

odersky commented Mar 6, 2024

But AppliedTypes are hash-consed, so there must be something else going on:

  object AppliedType {
    def apply(tycon: Type, args: List[Type])(using Context): AppliedType = {
      assertUnerased()
      ctx.base.uniqueAppliedTypes.enterIfNew(tycon, args)
    }
  }

@dwijnand dwijnand deleted the recursive-mt branch March 7, 2024 00:43
@dwijnand
Copy link
Member Author

dwijnand commented Mar 7, 2024

Yes, it's the type constructor that's different, because in it is a fresh LazyRef.

@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants