-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix 11018 - consistency of trait parameters #11059
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
Conversation
val baseClasses: List[ClassSymbol] = clazz.info.baseClasses | ||
|
||
// tracks types that we have seen for a particular base class in baseClasses | ||
val seenTypes = mutable.Map.empty[Symbol, List[Type]] |
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.
The original Scala 2 code used an Array since baseClasses where stored in a Seq
and allowed indexing into the array. Do you think it is a performance problem to use a Map
here instead?
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.
Performance of register
was mentioned here: scala/scala@42fb66a
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.
We have a specialized MutableSymbolMap that you probably want to use here.
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.
An alternative to using a map here would be to use a set which should be good enough to detect one error and stop.
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.
Looks like you tried something like this but ended up reverting it: https://github.com/lampepfl/dotty/compare/2201e3da859ced8edb9da7a71a931cd2bc5cc395..fe73b0b5371d5b4181c1a98be2dc881981c0b526, was there an issue with it?
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.
Yes, it actually was not semantics preserving and caused some regressions -- So I just reverted it.
// ported from Scala 2: | ||
// https://github.com/scala/scala/blob/9bb659e62a9239c01aec14c171f8598bb1a576fe/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala#L834-L883 | ||
def validateBaseTypes(): Unit = { | ||
val tpe = clazz.thisType // in Scala 2 this was clazz.tpe |
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.
Here is also a difference from Scala 2, where I use thisType
and Scala 2 used .tpe
.
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.
Otherwise LGTM!
Error: -- [E006] Not Found Error: /__w/dotty/dotty/compiler/src/dotty/tools/dotc/typer/RefChecks.scala:820:12
Error: 820 | unreachable()
Error: | ^^^^^^^^^^^
Error: | Not found: unreachable Change the first line of the file from: package dotty.tools.dotc to: package dotty.tools
package dotc to make the package object of dotty.tools where unreachable is defined in scope. |
I've gone ahead and pushed the fix I mentioned. |
Check conformance of trait and class parameters of base types. This PR attempts to solve #11018 by incorporating an additional check.
However, it might be too conservative as it also rules out a positive example of #3989.