-
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
More aggressive reduction of type selection (fixes parboiled2) #14987
Conversation
test performance please |
performance test scheduled: 6 job(s) in queue, 1 running. |
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? |
This comment was marked as resolved.
This comment was marked as resolved.
a7beb3e
to
be9c8be
Compare
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.
0fbf014
to
e0cce9a
Compare
…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.
I think the root of the confusion here is the definition of toBounds: |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/14987/ to see the changes. Benchmarks is based on merging with main (119e3d7) |
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.
LGTM. I think that was an overlook when MatchAliases were introduced to not also consider them in lookupRefined.
This is now fixed by scala#14987 (and that PR includes the test as a pos test)
…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.
Previously, when reducing
a.T
we checked if the type ofa
was a subtype ofRefinedType(.., T, TypeAlias(...))
, now we extend this check to handlerefinements where the
info
is aTypeBounds
where both bounds are equal.This solves two big issues at once:
The check for abstract types introduced by Don't follow BaseType of abstract binders in MT reduction #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 (Compilation broken against current dotty nightlies and likely for upcoming Scala 3.1.3 sirthias/parboiled2#365).
caching skolems (as in the alternative fix Cache skolem types #14909). Again, this is due to the
type containing skolem being reducible to a simpler type and therefore cacheable.