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

Remove unnecessary and recursive Space decomposition #19216

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 6, 2023

Space decomposition recently learnt to decompose prefixes. Given a
nested definition like in i19031, aggressively trying to decompose while
intersecting can lead to recursive decompositions (building bigger and
bigger nested prefixes). Turns out the decomposition isn't necessary.

@Decel
Copy link
Contributor

Decel commented Dec 6, 2023

Is it actually possible to have the same behaviour without type projections?

@dwijnand
Copy link
Member Author

dwijnand commented Dec 6, 2023

Which behaviour?

@Decel
Copy link
Contributor

Decel commented Dec 6, 2023

To have an overflow during decomposition.

@Decel
Copy link
Contributor

Decel commented Dec 6, 2023

It seems to me like those decompositions are not unnecessary. We could try something better, but I don't know what.

If the issue concerns only type projections, then it's possible to just special case them.

@dwijnand dwijnand force-pushed the recursive-decompose-prefix branch 2 times, most recently from 98c10c9 to b973cf9 Compare December 12, 2023 18:44
... makes it easier to disable one of them.
Space decomposition recently learnt to decompose prefixes.  Given a
nested definition like in i19031, aggressively trying to decompose while
intersecting can lead to recursive decompositions (building bigger and
bigger nested prefixes).  Turns out the decomposition isn't necessary.
If refineUsingParent/instantiateToSubType is passed a HK then it's
not possible to instantiate a class to that type (at the moment and
perhaps ever).  So it's important we guard against that.

This came up while trying to see if Mark[?] and Foo[Int] (from
pos/i19031.ci-reg1.scala) are provably disjoint - which they should be
reported not to be.  Because they're not applied types of the same type
constructor we end up trying to prove that HK type Mark is disjoint from
HK type Foo.  Because we don't know how to instantiate Foo's subclasses
(e.g Bar2) such that it's a subtype of higher-kinded type "Mark", we end
up discarding all of Foo's subclasses, which implies that Foo & Mark is
uninhabited, thus they are provably disjoint - which is incorrect.

We originally didn't encounter this because we eagerly decomposed in
Space intersection, while now we've dispatched it to provablyDisjoint.

(edit) We allow for some kindness in provablyDisjoint.
Minimising from the `case test.Generic =>` in ParallelTesting, the
anonymous pattern match is expanded, wrapping the match with
`applyOrElse`, which has a type parameter A1 as the scrutinee type, with
an upper bound of the original element type (out.Foo for us).  During
reachability analysis the pattern type, e.g. out.Bar3.type, is
intersected with the scrutinee type, A1 - giving out.Bar3.type & A1.
Then that we attempt to decompose that type.  Previously the abstract A1
in that type lead to 3 WildcardTypes, for the 3 subclasses, which are a
subtype of previous cases.

The fix that by generalising how we recognise the singleton types in the
scrutinee type, so instead of the ownership chain we use the parameter
type info, and we also match term parameters.  For extra correctness
we consider the failure to be a subtype of a mixin as a failure for
instantiating.

Also, make sure to handle and avoid recursion in traverseTp2.
@dwijnand dwijnand requested a review from Decel December 13, 2023 21:13
@dwijnand dwijnand marked this pull request as ready for review December 13, 2023 21:13
Copy link
Contributor

@Decel Decel left a comment

Choose a reason for hiding this comment

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

LGTM!

@dwijnand dwijnand merged commit 6313db6 into scala:main Dec 18, 2023
19 checks passed
@dwijnand dwijnand deleted the recursive-decompose-prefix branch December 18, 2023 08:46
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 26, 2024
#20804)

Backports #19216 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pattern matching on a sealed trait extended by its child leads to stack overflow and crash
3 participants