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

add Dotty cross-build #113

Merged
merged 2 commits into from
Oct 26, 2020
Merged

add Dotty cross-build #113

merged 2 commits into from
Oct 26, 2020

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Sep 17, 2020

preparations were made in #117, #118, #119, #120

references #80

running notes on the sorts of changes needed:

  • easy
    • Dotty caught some mis-indented code 👍
    • do-while is gone 👋
    • implicit defs need explicit result types ✔️
    • add -language:implicitConversions build-wide 🤷
  • medium
    • weird thing where I had to add empty bodies to some traits
      • need to circle back and figure out if this is a Dotty bug or what
      • as Som figured out, this was better fixed by keeping the self-types of the enclosing templates on the same line
    • new ambiguities where the same identifier is coming from multiple places and scalac would pick one but dotc wants explicit disambiguation
  • hard
    • existential types are gone
      • this was resolvable in every case by adding new type parameters
    • tons of variance errors
      • I'm blindly sprinkling @uncheckedVariance around to make it compile, but need to circle back and consider whether that's acceptable and what's really going on
        • in the end, I decided that although I didn't exhaustively drill down into what was happening in every case, my general sense was that Dotty's feedback here seemed reasonable. it's already established in the 2.13 collections design that @uncheckedVariance is needed for abstracting over mutable collections (invariant) and immutable ones (which may be covariant), so it's quite plausible that the Scala 2 typechecker missed some cases where it should have been stricter all along

@SethTisue
Copy link
Member Author

[error] -- [E057] Type Mismatch Error: /Users/tisue/scala-parallel-collections/core/src/main/scala/scala/collection/generic/ParMapFactory.scala:30:80 
[error] 30 |abstract class ParMapFactory[CC[X, Y] <: ParMap[X, Y] with ParMapLike[X, Y, CC, CC[X, Y], _]]
[error]    |                                                                                ^
[error]    |Type argument CC[X, Y] does not conform to upper bound scala.collection.parallel.ParMapLike[X, Y, scala.collection.parallel.ParMap, 
[error]    |  LazyRef(CC[X, Y])
[error]    |, collection.parallel.ParMapLike[X, Y, CC, CC[X, Y], ?]#Sequential] & 
[error]    |  scala.collection.parallel.ParMap[X, Y]

😱

interesting, and perhaps a clue, that ParSetFactory isn't giving a comparable error

@SethTisue
Copy link
Member Author

SethTisue commented Sep 17, 2020

the core module now compiles (on both 2.13.3 and Dotty) and this much works: 😎

scala> import scala.collection.parallel.CollectionConverters._

scala> List(1,2,3).par
val res0: scala.collection.parallel.immutable.ParSeq[Int] = ParVector(1, 2, 3)

scala> List(1,2,3).par.map(_ * 2).seq
val res1: Seq[Int] = Vector(2, 4, 6)

next step is to try to get the test suite going. I can do scalacheck first, then figure out testmacros, then junit

@SethTisue SethTisue force-pushed the add-dotty branch 3 times, most recently from 89506cc to ba4ef7f Compare October 8, 2020 00:31
@SethTisue
Copy link
Member Author

SethTisue commented Oct 8, 2020

scalacheck/test passes ✨, but with a ScalaCheck 1.15 snapshot because of the notorious BooleanOperators issue (typelevel/scalacheck#668)

@SethTisue
Copy link
Member Author

getting testmacros going was easy, it only had a single macro and it's a one-liner in Dotty

@som-snytt
Copy link
Contributor

Dotty has "inherited member can't shadow definition from enclosing scope" which isn't in scala 2 yet. (I will try to contribute it under -Xsource:3.)

@som-snytt
Copy link
Contributor

Apparently I was doing this while trying bug 12106 in August, I was curious what would dotty do? I wound up cp -R to the sample repo for the bug, which is why I totally forgot about it. Also pandemic.

@SethTisue
Copy link
Member Author

in local testing, all tests are now green except SerializationStabilityTest. I'll keep at it

thx @som-snytt for the comments. I will circle back to them

@SethTisue
Copy link
Member Author

SethTisue commented Oct 12, 2020

the ScalaCheck folks published ScalaCheck 1.15.0-M1 for Dotty 0.27.0-RC1 just now, so now the CI build is on par with what I was seeing locally using a snapshot build of ScalaCheck

@SethTisue SethTisue force-pushed the add-dotty branch 13 times, most recently from 6a2972c to fdf36d4 Compare October 15, 2020 22:58
@SethTisue
Copy link
Member Author

tests are now passing. I still want to see if the unchecked-variance commit can be improved or avoided

@SethTisue SethTisue marked this pull request as ready for review October 15, 2020 23:09
@SethTisue SethTisue force-pushed the add-dotty branch 3 times, most recently from 2ab9160 to 36d61e6 Compare October 16, 2020 00:59
@SethTisue
Copy link
Member Author

Okay, this is ready for review. I don't see a way to improve on the @uncheckedVariance situation.

@SethTisue SethTisue requested a review from lrytz October 16, 2020 01:01
@SethTisue
Copy link
Member Author

I'll merge on Monday (Oct 26) if there's no review feedback by then

@SethTisue
Copy link
Member Author

a couple of folks who might be masochistic enough to have a look: @szeiger @joshlemer

@SethTisue
Copy link
Member Author

note to prospective reviewers: of the other four PRs, #119 is the at-least-slightly-interesting one

@SethTisue SethTisue merged commit 8c129ac into scala:master Oct 26, 2020
@SethTisue SethTisue deleted the add-dotty branch October 26, 2020 19:40
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.

2 participants