Respect configuration in ExplicitReturnTypes

[gabro]

Following an offline chat with @dotta at Scala Italy, adding an explicit return type should be within the realm of the possible and it could greatly improve compile times.

-def foo(x: Int) = if (x > 3) Some(x) else None
+def foo(x: Int): Option[Int] = if (x > 3) Some(x) else None

This kind of mimics the idea of ExplicitImplicit.

@olafurpg do you see any blocker for this one?

[dotta]

My reasons why this kind of refactoring would be useful:

1) It's an established coding style to annotate non-private members with their return type. My reasons for this are:

• Can greatly improve readability (avoids having to jump to different places just to know what is the return type).
• Avoid unintended binary incompatibilities (this is particularly relevant for libraries). 2) It seems it can yield substantial compile time benefits, e.g., https://github.com/JetBrains/intellij-scala/commit/5b61302e6d11375ace2972a570b2200efe508437.

If this ticket gets accepted, I think it would be great if users could have a bit of flexibility and choose between making the type explicit for all members, or only for non-private ones.

By the way, I would rename the ticket to "Explicit return type for members" (instead of methods)

[gabro]

If this ticket get accepted, I think it would be great if users could have a bit of flexibility and choose between making the type explicit for all members, or only for non-private ones.

That's what I was thinking about too. I would like to choose between private/public and def/values.

I also believe some cases would be a tad awkward, like

-val someValue = "something"
+val someValue: String = "something"

but I'm not exactly sure of where to draw the line here.

[olafurpg]

This is a great idea! I am all on the benefits of explicit type annotations, this rewrite been a personal dream of mine for a long time :)

This rewrite will pretty much be the same rewrite as ExplicitImplicit modulo some configuration bits.

The main reason behind https://github.com/scalameta/scalameta/pull/848 was to experiment with using the new denotations section in the Scalameta semanticdb to implement this rewrite. However there are still some remaining issues

• denotations are only strings at the moment until https://github.com/scalameta/scalameta/issues/822 is fixed. This means for now the inserted type annotations will be fully qualified, not pretty.
• the semanticdb only contains denotations for definitions that are compiled with the scalahost compiler plugin, not dependencies from the classpath. Our options here are either to
  1. produce denotations on the fly for symbols on the classpath.
  2. compile the whole universe with scalahost, but that won't work for java sources so we probably need to do (1) anyways.

[olafurpg]

Hold that thought, wrote my response above in a rush. For explicit type annotations, the second bullet is not a problem since the type annotation is inserted at definition site, which we compile with scalahost. The first problem is still valid, but we can already start exploring implementing this rewrite under the assumption that we will later have the tools necessary to pretty-print the fully qualified references.

[gabro]

Thanks for the analysis, sounds promising!

About the configuration part, do you have any plan on including a config per-rewrite? Or should we just explode the cartesian product of rewrites x options?

[olafurpg]

On rewrite config I've considered reserving the path custom.*, or something like that. Rewrites could access the config as a ctx.config.custom: metaconfig.Conf. Then they can optionally parse it into a case class or just access specific fields directly. I'll have to update the docs on this.

[olafurpg]

I just merged https://github.com/scalacenter/scalafix/pull/163 which upgraded to the latest meta and re-enabled ExplicitImplicit.

I'm thinking of renaming ExplicitImplicit to ExplicitTypes and then open an ExplicitTypeSettings (or similar) with options like

explicitTypes.what = Implicits/Public/Protected/Private/..
explicitTypes.skipLiterals = true # skips stuff like `val x = "blah"`
...

Suggestions for reasonable configurations options are welcome! Please include original and expected output.

[xeno-by]

Maybe ExplicitReturnTypes?

[dotta]

@olafurpg Awesome stuff! I'll definitely try it out. About the configuration options, I think what you have now is already pretty great. Maybe, I'd consider splitting explicitTypes.what into explicitTypes.memberVisibility and explicitTypes.memberKind, e.g.,

explicitTypes.memberVisibility = All/Public/Protected/Private # this could be cumulative, e.g., Public+Protected should be possible
explicitTypes.memberKind = All/Implicit/Field/Method # Also this could be cumulative

[olafurpg]

Configuration with roughly that shape @dotta has been merged into master, see https://github.com/scalacenter/scalafix/blob/db6152304aaef19c4a8ba4fc77b732264bf52fec/scalafix-core/src/main/scala/scalafix/config/ExplicitReturnTypesConfig.scala However, ExplicitReturnTypes does not respect this configuration at the moment. It only works for implicit vals.

I've renamed the title of this issue to be about making ExplicitReturnTypes respect this configuration.

I believe this would be a great issue for someone who wants to contribute and learn more about scalafix. Don't hesitate to ask me if you have any questions.

[taisukeoe]

I'm so sorry for making issue reference dirty! I sent a PR for this :)

[ShaneDelmore]

I ran into an odd edge case yesterday, not related to this pr but worth keeping in mind. I was calling a library api that had a public builder for a private type. In this case type inference works, but type annotation fails.

[taisukeoe]

@ShaneDelmore Mind you if I ask you to provide your edge case sample codes with me? I'll check it on my PR branch.

[ShaneDelmore]

@taisukeoe It would be my pleasure, thanks for your awesome work on this so far. Here is a simple scastie reproduction, let me know if you need more than that.
https://scastie.scala-lang.org/ShaneDelmore/9nCslzmcTqKTPo5BWwPupg/1

The problem comes from this pattern: There is a builder, which definitely has the type InstrumentFactory https://github.com/kamon-io/Kamon/blob/kamon-1.0-develop/kamon-core/src/main/scala/kamon/metric/InstrumentFactory.scala#L79

and a class which is private https://github.com/kamon-io/Kamon/blob/kamon-1.0-develop/kamon-core/src/main/scala/kamon/metric/InstrumentFactory.scala#L29

I actually think this is a bug in Scala, but as this rewrite is used with the compilers we have now I thought you should know about it.

[taisukeoe]

@ShaneDelmore Thanks for providing info. Though the Scastie reproduction doesn't have either type inference nor type annotation (and successfully runs), what should I do to reproduce?

Just in case, I confirmed ExplicitReturnType rewrite works for a private class and a builder, as follows. Please let me know if this test case isn't relevant for your case.

https://github.com/taisukeoe/scalafix/blob/069017c3492e26103e962d5341f382939fb01bad/scalafix-tests/input/src/main/scala/test/ExplicitReturnTypes.scala#L82-L87

https://github.com/taisukeoe/scalafix/blob/069017c3492e26103e962d5341f382939fb01bad/scalafix-tests/output/src/main/scala/test/ExplicitReturnTypes.scala#L77-L82

[ShaneDelmore]

Oh wow, that is the completely wrong scastie link. Oops, give me a few minutes and I will recreate a proper reproduction.

[ShaneDelmore]

Hopefully this scastie works, sometimes it doesn't appear to save my code so in case this scastie link is also broken I will put the reproduction code below as well:

https://scastie.scala-lang.org/wdskxrQQRbeB2ZHkDdXaoA

app.scala: import kamon.metric.InstrumentFactory import com.typesafe.config._

object Main extends App { val thisWorks = InstrumentFactory.fromConfig(ConfigFactory.load()) val thisDoesNotWork: InstrumentFactory = InstrumentFactory.fromConfig(ConfigFactory.load()) println("Finished") }

build.sbt: scalaVersion := "2.12.2" scalacOptions ++= Seq( "-deprecation", "-encoding", "UTF-8", "-feature", "-unchecked" )

resolvers += Resolver.bintrayRepo("kamon-io", "snapshots")

libraryDependencies += "com.typesafe" % "config" % "1.3.1" libraryDependencies += "io.kamon" %% "kamon-core" % "1.0.0-alpha1-b2b43016b2b652ebcd686959048e3c5fe427e649"

[taisukeoe]

@ShaneDelmore Thanks! I could reproduce it as follows:

package factory{
  private [factory] class Factory
  object Factory{
    def f(): factory.Factory = new Factory()
  }
}
object G {
 //type annotation fails to compile!
  val f: factory.Factory = factory.Factory.f()
}

While I would also think it's a Scala problem, it would be great if there is a way NOT to avoid rewriting this case in ExplicitReturnType rewrite. No one doesn't want to rewrite anything into not compile-able source codes!

However, I'm wondering if there is any smart way to check if it's dennotated type is declared in Private or not. Should I search mirror.database.denotations everytime?

It seems we may want to report this issue into scala/bug issue tracker for now.

[ShaneDelmore]

Bug filed at https://github.com/scala/bug/issues/10366