-
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
Fix #3989: Fix several unsoundness problems related to variant refinement #4013
Conversation
// - members in other concrete classes, since these have been checked before | ||
// (this is done for efficiency) | ||
// - members in a prefix of inherited parents that all come from Java or Scala2 | ||
// (this is done to avoid false negatives since Scala2's rules for checking are different) |
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.
"false negatives" = false errors from the typechecker? I'm used to "false positives" (which makes sense for error checking).
More importantly, this means that we should test that Scala 2 material indeed follows Scala 2 rules?
val seenClasses = mutable.Set[Symbol]() | ||
def addDecls(cls: Symbol): Unit = | ||
if (!seenClasses.contains(cls)) { | ||
seenClasses.+=(cls) |
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.
Why not seenClasses += cls
? Has idiomatic style changed?
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.
No, hat was an editing breakage.
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4013/ to see the changes. Benchmarks is based on merging with master (668c499) |
That's what I feared - performance is dramatically worse for scala-library, slightly worse for everything else. |
The collection strawman does not compile under this branch because it contains some patterns that are now recognized as unsound. Some have easy fixes. E.g. I have disabled the tests involving strawman until this is fixed. We should re-enable them afterwards. |
test performance please |
performance test scheduled: 1 job(s) in queue, 1 running. |
@julienrf Yes, the ArrayOps changes seem to be already done. But |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4013/ to see the changes. Benchmarks is based on merging with master (9053964) |
@julienrf Also, the later changes here could invalidate some pattern matches. This is because it propagates fewer types from scrutinees to pattern. To fix this, one needs to sometimes write an @unchecked type argument in the pattern. I have noted one such case in |
test performance please |
performance test scheduled: 3 job(s) in queue, 1 running. |
Now all variants of problems I could derive from #3989 seem to be fixed. |
@odersky Just in case, is this example with |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4013/ to see the changes. Benchmarks is based on merging with master (2a019f3) |
@Blaisorblade The |
Regarding performance, I am inclined to not do anything right now. compileStdLib is an outlier, and I see no obviously safe way to reduce the number of checks. |
@olafurpg Here's a problem scala-fix could help with: Insert |
Effect of this on collection strawman (with failures both in the original design and more in the changes I merged yesterday in scala/collection-strawman#468:
|
Once the collection-strawman is fixed, we should check the performance impact there too. |
Seems I dropped the ball on this one... Let me take a look at the new issues. |
This fixes the second half of scala#3989. Some tests had to be updated or re-classified because they were unsound before.
It's more efficient. Let's see whether that makes a difference.
Plugging the soundness hole in scala#3989 unveiled problems in the strawman. These need to be fixed before we can test it again in dotty.
i3989a.scala gives an example which is rejected under the new scheme. This would pass and fail at runtime under the previous scheme.
The problems reported by @liufengyun are now fixed as well. @liufengyun do you want to review the last two commits? |
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, cannot find another counter-example.
Good news! |
This version of Dotty fixes several unsoundness issues and remove the unsound protected[this] escape hatch for variance checking. See scala/scala3#4013
This version of Dotty fixes several unsoundness issues and remove the unsound protected[this] escape hatch for variance checking. See scala/scala3#4013
Does this also close the unsoundness illustrated by @TomasMikula in scala/bug#10822 (comment), where GADT matching is unsoundly allowed because a covariant occurrence of a type param is hidden by a type ascription where it only occurs invariantly? |
@adriaanm I think the unsoundness in scalac comes from allowing this to compile: class Foo[A]
private[this] class Bar[+A] extends Foo[A] Disabling the variance checks for |
Gah, right! I should’ve caught that. Thanks for pointing it out.
…On Fri, Apr 13, 2018 at 14:56 Guillaume Martres ***@***.***> wrote:
@adriaanm <https://github.com/adriaanm> I think the unsoundness in scalac
comes from allowing this to compile:
class Foo[A]private[this] class Bar[+A] extends Foo[A]
Disabling the variance checks for private[this] is not sound here. In
Dotty your example compiles because the variance checks for the extends
clause are completely missing, but once we add them we won't special case
them for private[this]: #2973
<#2973>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4013 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFjyzNnx6_lplbr1fjDn5aiKUGM7wsoks5toKBqgaJpZM4SKzvc>
.
|
This solves a number of unsoundness problems related to variant refinement
Some tests had to be updated or re-classified because they were unsound before. The collection strawman needs to be updated to conform to the new rules.