scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.82k stars 1.05k forks source link

Document migration steps for changed rules for given prioritisation under 3.6 #20843

Closed WojciechMazur closed 1 month ago

WojciechMazur commented 3 months ago

Scala 3.5 introduces gradual changes to rules prioritization https://github.com/scala/scala3/pull/19300 , these now (3.5) warn about selected implicit, and would start yielding errors in the future (3.6). These changes lead to multiple warnings/errors in real-world projects like lichess-org/lila. We should identify the patterns in which warnings occur and provide a guide/migration steps on how to deal with them. Especially how to deal with changed resolved targets.

soronpo commented 3 months ago

The main issue with this change is that it's a problem that manifests down to the application level. We cannot simply just silence the warnings at the library level. Meaning that any library that experiences this, now needs its own migration guide for its users.

lenguyenthanh commented 2 months ago

I "successful" migrated lila to 3.5.0-RC3 with -source:3.6-migration: https://github.com/lichess-org/lila/pull/15664. But I'm quite afraid that #19300 will cause a lot of trouble to the community/industry as @soronpo said above.

I believe that there are a lot of projects out there depend on the rule of choosing the most specific implicit (before #19300). Which is kinda intuitive in OOP sense.

I really think that We have to have some migration guide before 3.5 release.

Gedochao commented 2 months ago

cc @odersky

odersky commented 2 months ago

@lenguyenthanh We did tests in the open CB and there was only a tiny minority of projects that broke (something like 5 out of 1500). I was myself very surprised by this and that was the main reason why we decided that we could do the change. After all the change is a huge improvement, without it you cannot really do serious typeclass-based development.

Most of the breakage traced back to Reactive Mongo which is also the root error for Lichess. There were more warnings before but since our latest fixes to avoid spurious warnings it should be much better.

We should do a careful analysis of ReactiveMongo and advise downstream projects of possible breakages. The implicits in Reactive Mongo's BSON handling that cause the switch in priority are unfortunately extremely complex and inscrutable (at least to my eyes).

Gedochao commented 1 month ago

@bracevac this would be covered by https://github.com/scala/scala-lang/pull/1675, right?

lenguyenthanh commented 1 month ago

@lenguyenthanh We did tests in the open CB and there was only a tiny minority of projects that broke (something like 5 out of 1500). I was myself very surprised by this and that was the main reason why we decided that we could do the change. After all the change is a huge improvement, without it you cannot really do serious typeclass-based development.

I'm 100% behind this change. It does indeed improve Scala given semantic and I'm very optimistic that this change combining with https://github.com/scala/improvement-proposals/pull/81 It'll easier for new comers understand and use contextual abstraction. Which is one of the power of Scala IMHO.

Most of the breakage traced back to Reactive Mongo which is also the root error for Lichess. There were more warnings before but since our latest fixes to avoid spurious warnings it should be much better.

We should do a careful analysis of ReactiveMongo and advise downstream projects of possible breakages. The implicits in Reactive Mongo's BSON handling that cause the switch in priority are unfortunately extremely complex and inscrutable (at least to my eyes).

My concern is mostly about the migration works for the industry. If the errors is "obvious" like in reactive-mongo, We (the industry) can solve it (maybe somewhat difficult but it's solvable, I solved it in lila by reading the PR and reactive-mongo code). But sometime, We make it compiles, no warning anymore, but the business logic is reversed. And I think this kind of errors is the one We should focus on the migration documentation.

Here are some issues We have had after 3.5.0-RC4 (with given prioritisation change) migrations:

p/s: I agree that the way reactive-mongo and lila using implicit and given are over complicated (I would say "abusing") but I guess that not to abnormal in the industry.

edited to fix typo and add some more information.

bracevac commented 1 month ago

@bracevac this would be covered by scala/scala-lang#1675, right?

Yes. The blog post is live now.

@lenguyenthanh It would be great if you could further minimize your examples and solutions. They’re a bit hard to follow as is, so I’m not sure if there’s something in them that isn’t already covered by our recommendations. If we can boil them down to their essence, we can easily add them to the blog post.

Gedochao commented 1 month ago

https://github.com/scala/scala-lang/pull/1675 has been merged, closing this. If a follow-up is necessary, feel free to open a new issue.