SI-5330, SI-6014 deal with existential self-type #1074
Closed
Conversation
This file contains 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
This has been broken since scala@b7b81ca2#L0L567. The existential rash is treated in a similar manner as in fc24db4. Conceptually, the fix would be `def selfTypeSkolemized = widen.skolemizeExistential.narrow`, but simply widening before narrowing achieves the same thing. Since we're in existential voodoo territory, let's go for the minimal fix: replacing `this.narrow` by `widen.narrow`.
|
/cc @retronym |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/101/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/804/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/101/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/804/ |
|
This looks fishy and will be disastrous from a performance standpoint. Narrow is expensive unless the type is already a singleton type. Widen makes sure it isn't. |
|
In fact, I think it would be OK to outlaw existential self types. |
|
What could be outlawed in this case? |
|
A couple ideas for performance:
I repeat @paulp's comments from /** Equivalent to allOverriddenSymbols.nonEmpty, but more efficient. */
// !!! When if ever will this answer differ from .isOverride?
// How/where is the OVERRIDE flag managed, as compared to how checks
// based on type membership will evaluate?
def isOverridingSymbol = owner.isClass && (
owner.ancestors exists (cls => matchingSymbol(cls, owner.thisType) != NoSymbol)
) |
adriaanm
pushed a commit
to adriaanm/scala
that referenced
this pull request
Nov 14, 2012
This has been broken since scala@b7b81ca2#L0L567. The existential rash is treated in a similar manner as in fc24db4. Conceptually, the fix would be `def selfTypeSkolemized = widen.skolemizeExistential.narrow`, but simply widening before narrowing achieves the same thing. Since we're in existential voodoo territory, let's go for the minimal fix: replacing `this.narrow` by `widen.narrow`. -- Original patch by @retronym in scala#1074, refined by @paulp to only perform widen.narrow incantation if there are existentials present in the widened type, as narrowing is expensive when the type is not a singleton. The result is that compiling the entirety of quick, that code path is hit only 143 times. All the other calls hit .narrow directly as before. It looks like the definition of negligible in the diff of -Ystatistics when compiling src/library/scala/collection: < #symbols : 306315 --- > #symbols : 306320 12c13 < #unique types : 293859 --- > #unique types : 293865 I'm assuming based on the 2/1000ths of a percent increase in symbol and type creation that wall clock is manageable, but I didn't measure it.
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Nov 14, 2012
This has been broken since scala@b7b81ca2#L0L567. The existential rash is treated in a similar manner as in fc24db4. Conceptually, the fix would be `def selfTypeSkolemized = widen.skolemizeExistential.narrow`, but simply widening before narrowing achieves the same thing. Since we're in existential voodoo territory, let's go for the minimal fix: replacing `this.narrow` by `widen.narrow`. -- Original patch by @retronym in scala#1074, refined by @paulp to only perform widen.narrow incantation if there are existentials present in the widened type, as narrowing is expensive when the type is not a singleton. The result is that compiling the entirety of quick, that code path is hit only 143 times. All the other calls hit .narrow directly as before. It looks like the definition of negligible in the diff of -Ystatistics when compiling src/library/scala/collection: < #symbols : 306315 --- > #symbols : 306320 12c13 < #unique types : 293859 --- > #unique types : 293865 I'm assuming based on the 2/1000ths of a percent increase in symbol and type creation that wall clock is manageable, but I didn't measure it.
This was referenced Apr 7, 2017
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.
This has been broken since b7b81ca2#L0L567.
The existential rash is treated in a similar manner as in fc24db4.
Conceptually, the fix would be
def selfTypeSkolemized = widen.skolemizeExistential.narrow,but simply widening before narrowing achieves the same thing. Since we're in existential voodoo territory,
let's go for the minimal fix: replacing
this.narrowbywiden.narrow.review by @odersky