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

More aggressive reduction of type selection (fixes parboiled2) #14987

Merged
merged 2 commits into from Apr 21, 2022

Conversation

smarter
Copy link
Member

@smarter smarter commented Apr 20, 2022

Previously, when reducing a.T we checked if the type of a was a subtype of
RefinedType(.., T, TypeAlias(...)), now we extend this check to handle
refinements where the info is a TypeBounds where both bounds are equal.

This solves two big issues at once:

@smarter
Copy link
Member Author

smarter commented Apr 20, 2022

test performance please

@dottybot
Copy link
Member

performance test scheduled: 6 job(s) in queue, 1 running.

@smarter
Copy link
Member Author

smarter commented Apr 20, 2022

If this is accepted, I think it's worth backporting to 3.1.3 since it fixes a regression and would benefit parboiled2 as well as downstream libraries like akka-http, wdyt?

@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Apr 20, 2022
@griggt

This comment was marked as resolved.

Previously, when reducing `a.T` we checked if the type of `a` was a subtype of
`RefinedType(.., T, TypeAlias(...))`, now we extend this check to handle
refinements where the `info` is a `TypeBounds` where both bounds are equal.

This solves two big issues at once:
- We can restore tests/pos/13491.scala to its original form from before scala#13780.
  The check for abstract types introduced by scala#13780 for soundness reasons is no
  longer hit because the type selection is reduced before we get to that point.
  This is important because parboiled2 relies on this and is therefore currently
  broken on 3.1.3-RC1 and main (sirthias/parboiled2#365).
- This fixes scala#14903 (slow compilation issue affecting parboiled2) without
  caching skolems (as in the alternative fix scala#14909). Again, this is due to the
  type containing skolem being reducible to a simpler type and therefore cacheable.
@smarter smarter force-pushed the aggressive-reduction branch 2 times, most recently from 0fbf014 to e0cce9a Compare April 20, 2022 21:00
…ual bounds

It turns out that the problematic cases fixed in the previous commit only occurs
because we create type aliases with info of type MatchAlias instead of TypeAlias
if their rhs is an applied match type (Namer#TypeDefCompleter#typeSig calls
`toBounds` on the rhs which does `if (self.isMatch) MatchAlias(self)`).

I'm not sure if there is a good reason for that (and if so, do we need to be
careful to avoid loops when dealiasing MatchAlias?) or if we should change the
logic in Namer to return a TypeAlias instead.
@smarter
Copy link
Member Author

smarter commented Apr 20, 2022

I think the root of the confusion here is the definition of toBounds:
https://github.com/lampepfl/dotty/blob/119e3d76a5b4476d1e0cfc282bb81d163d0de7cd/compiler/src/dotty/tools/dotc/core/TypeApplications.scala#L410-L416
self.isMatch will return true for the rhs of Out in https://github.com/dotty-staging/dotty/blob/aggressive-reduction/tests/pos/i14903a.scala#L15 because the rhs is an applied match type, even though Out itself is just a plain type alias, so we end up typing it as a MatchAlias. Perhaps toBounds should check if the rhs tree is a MatchTypeTree instead? But then I don't know what to do about the unpickling logic for MatchAlias which also relies on the type of the alias itself: https://github.com/lampepfl/dotty/blob/119e3d76a5b4476d1e0cfc282bb81d163d0de7cd/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala#L374-L377
Maybe we need to actually store whether we're pickling a MatchAlias or TypeAlias in Tasty? But that fix wouldn't be backportable to 3.1.x

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/14987/ to see the changes.

Benchmarks is based on merging with main (119e3d7)

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM. I think that was an overlook when MatchAliases were introduced to not also consider them in lookupRefined.

@odersky odersky merged commit e2e2885 into scala:main Apr 21, 2022
@odersky odersky deleted the aggressive-reduction branch April 21, 2022 09:05
odersky added a commit to dotty-staging/dotty that referenced this pull request Apr 21, 2022
This is now fixed by scala#14987 (and that PR includes the test as a pos test)
smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 21, 2022
…ed2)

This backports scala#14987 which fixes a regression introduced in 3.1.3-RC1 that
prevented parboiled2 from compiling and also fixes a performance issue affecting
parboiled2.
@smarter smarter added backport:done This PR was successfully backported. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Apr 26, 2022
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow compilation with chained dependent match types
5 participants