Skip to content
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 11018 - consistency of trait parameters #11059

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

b-studios
Copy link
Contributor

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.

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]]
Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM!

compiler/src/dotty/tools/dotc/typer/RefChecks.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/RefChecks.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/RefChecks.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/RefChecks.scala Outdated Show resolved Hide resolved
@smarter
Copy link
Member

smarter commented Feb 18, 2021

 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.

@smarter smarter removed their assignment Feb 18, 2021
@smarter
Copy link
Member

smarter commented Mar 12, 2021

I've gone ahead and pushed the fix I mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants