Fix #6199: Use a skolemized prefix in asSeenFrom when needed #6454
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smarter
force-pushed
the
fix/type-member-inf-7
branch
from
5b454e3 to
c768382
Compare
smarter
force-pushed
the
fix/type-member-inf-7
branch
2 times, most recently
from
f556249 to
3c75623
Compare
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
smarter
commented
May 5, 2019
| def accessibleDenot(pre: Type, name: Name, sig: Signature) = { | ||
| val d = pre.member(name).atSignature(sig) | ||
| def accessibleDenot(qualType: Type, name: Name, sig: Signature) = { | ||
| val pre = ctx.typeAssigner.maybeSkolemizePrefix(qualType, name) |
There was a problem hiding this comment.
I'm not super happy with having to recompute whether the prefix needs to be skolemized in the unpickler, but I don't really see a way around it. Maybe asSeenFrom should behave differently when unpickling a tree, but how do you even know if the type on which asSeenFrom is called comes from a tree being unpickled ? And even if we manage to allow unstable prefixes when unpickling, is that really sound when combined with inlining ?
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6454/ to see the changes. Benchmarks is based on merging with master (61ea245) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6454/ to see the changes. Benchmarks is based on merging with master (61ea245) |
Prior to scala@91ee02f, unstable prefixes appearing in selection types were handled in a two-pass approach which can be summarized as: 1. Type the selection with the unstable prefix, if this prefix appears in a position where it cannot be widened away, mark it with a special annotation. 2. If the result of the selection contains this annotation, retype it after having wrapped its prefix in a skolem. This got replaced by the use of an `ApproximatingTypeMap` which can always construct a valid (but potentially approximated) type for the selection. Most of the time this is all we need but there are some cases where skolemizing the prefix is still useful as witnessed by the tests added in this commit. They are now handled by unconditionally skolemizing unstable prefixes. To avoid any potential performance impact from this, we teach `asSeenFrom` to always widen these prefixes first and only make use of the skolem to avoid returning an approximation. Since this almost never occurs (it happens only once when compiling Dotty) this means we incur almost no extra cost (the skolem itself still needs to be allocated unconditionally in advance, this cannot be delegated to `asSeenFrom` because it's supposed to be an idempotent operation and each skolem is unique).
smarter
force-pushed
the
fix/type-member-inf-7
branch
from
3c75623 to
502c7c9
Compare
smarter
added a commit
to smarter/zio
that referenced
this pull request
Jun 26, 2019
These are no longer needed thanks to scala/scala3#6454
ghostdogpr
pushed a commit
to zio/zio
that referenced
this pull request
Jun 27, 2019
…make the CI compile the tests with Dotty too (#1088) * Fix the Dotty build stacktracerJVM and coreJVM were using different versions of dotty, so `++0.16.0-RC3; coreJVM/compile` failed (it worked if you did "++0.16.0-RC3!`, which is what the CI does). * Make compileJVM compile the tests too That way, compileJVM is used by the CI to test Dotty, so this ensures that the tests compile with Dotty. * Avoid passing the same option twice to Dotty This is already set in `extraOptions`. * Remove some Dotty workarounds These are no longer needed thanks to scala/scala3#6454
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to
91ee02f,
unstable prefixes appearing in selection types were handled in a
two-pass approach which can be summarized as:
in a position where it cannot be widened away, mark it with a special
annotation.
after having wrapped its prefix in a skolem.
This got replaced by the use of an
ApproximatingTypeMapwhich canalways construct a valid (but potentially approximated) type for the
selection.
Most of the time this is all we need but there are some cases where
skolemizing the prefix is still useful as witnessed by the tests added
in this commit. They are now handled by unconditionally skolemizing
unstable prefixes.
To avoid any potential performance impact from this, we teach
asSeenFromto always widen these prefixes first and only make use ofthe skolem to avoid returning an approximation. Since this almost never
occurs (it happens only once when compiling Dotty) this means we incur
almost no extra cost (the skolem itself still needs to be allocated
unconditionally in advance, this cannot be delegated to
asSeenFrombecause it's supposed to be an idempotent operation and each skolem is
unique).