scalacenter / scala3-migrate

A tool to help migrating from Scala 2 to Scala 3
https://docs.scala-lang.org/scala3/guides/migration/scala3-migrate.html
Apache License 2.0
110 stars 24 forks source link

confusion around necessity of adding explicit result types and implicits #334

Open OlegYch opened 2 years ago

OlegYch commented 2 years ago

https://github.com/scalacenter/scala3-migrate#fix-syntax-incompatibilities does not mention adding result types to anything, but it currently does via ExplicitResultTypes rule it looks like it should not because adding result types is not necessary to fix syntax moreover afaik it is only necessary to add result types to implicits to compile with scala3, adding them everywhere is noisy i guess it would be enough to just remove ExplicitResultTypes usage, since migrate would later invoke InferTypes anyway

and is ExplicitImplicitsRule really necessary?

mlachkar commented 2 years ago

You're right. It's not mentioned there. The readme is a outdated a little, the most recent documentation is here https://docs.scala-lang.org/scala3/guides/migration/scala3-migrate.html (which doesn't mention the explicitResultType rule neither) In this post we explain why it's important to have public api typed: link

First step: Make sure that all methods/functions of the public API have an explicit result type. 
If not, we require to run Explicit ResultType rule, which will type explicitly public and protected methods. 
Those types will be kept even after scala 3 migration. This is a good practice anyway, even in the context 
of a single Scala version, so that the APIs do not change based on the whims of the compiler’s 
type inference. It will ensure that the following steps do not alter the public API.

this step is done in the migrate-syntax, maybe you can skip it ?

OlegYch commented 2 years ago

@mlachkar i realize having explicit return types is a good practice, but it is not related to migration and actually hinders it

adpi2 commented 2 years ago

@OlegYch Having explicit return types is a necessary prerequisite of the migrate step because migrate is incremental: it migrates one file at a time. The migration of one file should not impact the migration of other files. In other words the migrate task should only operate on private types.

OlegYch commented 2 years ago

@adpi2 hm, can adding explicit result types be moved to migrate or separate step then? because currently migrate is not working on one of my projects (consuming > 20gb of heap) and does nothing on another, but id like to use migrate-syntax for other things it does

adpi2 commented 2 years ago

Yes, maybe we could try to move the adding result types into the migrate step. Would you like to contribute?

Or, as an alternative, you can add the sbt-scalafix plugin in your project and execute the rules used by migrate-syntax.

OlegYch commented 2 years ago

@adpi2 i'm not sure if there is a separate rule for adding result types just to implicit definitions, can you perhaps give a hint?

adpi2 commented 2 years ago

I don't think there is unfortunately. An "implicit only" configuration would be a good feature to add to the ExplicitResultTypes rule.