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

scala 3: ExplicitResultTypes #1583

Open
mlachkar opened this issue Mar 30, 2022 · 11 comments
Open

scala 3: ExplicitResultTypes #1583

mlachkar opened this issue Mar 30, 2022 · 11 comments

Comments

@mlachkar
Copy link
Collaborator

mlachkar commented Mar 30, 2022

Adapt Explicit result types for Scala 3

The current state

ExplicitResultTypes rule calls the scala 2 presentation compiler to infer types for each val, def or var. To make this rule available for scala 3, we need to re-write it to interface this time with the Scala 3 presentation compiler, in other words, ExplicitResultTypes rule for Scala 3, need to be written in Scala 3.

How to implement ?

flowchart LR
A(Scalafix-cli) -->|depends on| B(Scalafix-rules)
Loading

The actual design of Scalafix requires that the module scalafix-rules uses the same Scala Version than the Scalafix-cli module. To be able to use the Scala 3 presentation compiler, the rule needs to be rewritten in Scala 3 and therefore scalafix-cli needs to be able to load a Scala 3 rule.

Most likely implementation

  1. Create a Java interface for ExplicitResultTypes that provides the minimum needed methods for it to work. The interface would look like below:
public interface ExplictResultTypesInterface {
  List<PositionWithString> fix(Optional<Object>: global, Path: relativePathToSemanticDbFile)
  Object createGlobal(scalacClasspath, scalacOptions)
}
  • Currently ExplicitResultTypes rule needs a SemanticDocument, that cannot be passed as a parameter to the java method, so each implementation of this rule will need to recreate it from its relative path.
  • The fix method would return a list of Position and String, which should be enough to create a scalafix.Patch.
  • The global instance needs also to be created differently depending on the Scala Version. We can maybe stop using ScalafixGlobal and use directly scala.tools.nsc.interactive.Global (in Scala 2) and dotty.tools.dotc.interactive.InteractiveDriver (in Scala 3). ScalafixGlobal is almost only wrapping the Global from the presentation compiler. In this Java interface, it would be an Object, and then each implementation would cast it as a Global instance or InteractiveDriver instance depending on the Scala version.

Here is an example of the interface implemented by Metals to deal with the same issue. Their global is called PresentationCompiler, and in their case, they only infer the type for one position, whereas ExplicitResultTypes needs to do it for the entire file.

  1. The module Scalafix-cli or Scalafix-rules (still need to be defined) would need some specific code to be able to classload the right jar of ExplicitResultTypes , in other words either the version written in Scala 2 or Scala 3. (link to metals that does that, or other project that does that too)

  2. Write the new ExplicitResultTypes for scala 3, that implements the ExplictResultTypesInterface interface. We can definitely get inspired by Metals implementation for inferType code action (link)

Other possible implementation

If we are able to cross compiler scalafix-core, scalafix-cli in Scala 3 , it should be possible to write a specific version of Explicit Result types for Scala 3.

@mlachkar mlachkar mentioned this issue Mar 30, 2022
5 tasks
@mlachkar mlachkar added this to the Scala 3 support milestone Mar 30, 2022
@mlachkar
Copy link
Collaborator Author

With a minimal build:

val scala3Version = "3.1.2"
lazy val root = project
  .in(file("."))
  .settings(
    scalaVersion := scala3Version,
    libraryDependencies += "org.scalameta" %% "scalameta" % "4.5.6" cross CrossVersion.for3Use2_13
  )

I m able to compile and run the following code

import scala.meta.*

@main def hello: Unit =
  val program = """object Main extends App { print("Hello!") }"""
  val tree = program.parse[Source].get
  println(Term.Apply(Term.Name("function"), List(Term.Name("argument"))))
  println(s"$tree")

@bjaglin bjaglin changed the title Adapt ExplicitResultTypes for Scala 3 Adapt ExplicitResultTypes for Scala 3 (GSoC) Jul 15, 2022
@rvacaru
Copy link
Contributor

rvacaru commented Jul 15, 2022

Hi all, as part of GSoC 2022 I'm currently working on cross compiling scalafix-core, scalafix-rules, scalafix-reflect and scalafix-cli to scala 3, hence allowing for the implementation of ExplicitResultTypes afterwards.

@bjaglin bjaglin changed the title Adapt ExplicitResultTypes for Scala 3 (GSoC) scala 3: ExplicitResultTypes Oct 2, 2022
@bjaglin bjaglin added the scala3 label Oct 2, 2022
@bjaglin bjaglin removed this from the Scala 3 support milestone Oct 2, 2022
@bjaglin
Copy link
Collaborator

bjaglin commented Oct 2, 2022

To clarify, we've gone towards the "Other possible implementation", so this is therefore blocked by #1680

@guizmaii
Copy link

guizmaii commented Oct 9, 2023

Any news maybe? It's been a year since the last comment

@bjaglin
Copy link
Collaborator

bjaglin commented Oct 14, 2023

Hi @guizmaii ! Unfortunately there hasn't been any work on that topic since the GSoC last year. I am the only maintainer with limited time and no longer use Scala in my day-to-day job, so the last few months have been merely about keeping Scalafix up to date with the ecosystem. Contributions are of course very welcome!

@tgodzik
Copy link
Contributor

tgodzik commented Oct 16, 2023

Potentially, could we reuse the presentation compiler module that will come for 3.3.2 with a potential fallback on the metlas mtags?

Did we discuss that at any point? That is a binary compatible interface https://github.com/scalameta/metals/blob/63a65d297c0c70173869603e8f1cf0950a665cd6/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java#L136

@bjaglin
Copy link
Collaborator

bjaglin commented Nov 10, 2023

could we reuse the presentation compiler module that will come for 3.3.2

TIL: https://contributors.scala-lang.org/t/stable-presentation-compiler-api/6139 & scala/scala3#17528 💪 🤩

Did we discuss that at any point? That is a binary compatible interface https://github.com/scalameta/metals/blob/63a65d297c0c70173869603e8f1cf0950a665cd6/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java#L136

We haven't, I was not aware that the mtags implementation had effectively been upstreamed - it looks like a somewhat low-hanging fruit to integrate it to scalafix!

I am unsure about the need for a Java API wrapper (thinking about the classloading penalty mostly), as we already build Scala 3 artifacts thanks to the 2022 GSoC. There are a few items to iron out before publishing, but there is nothing particularly complex. The biggest challenge was obviously the lack of scalameta quasiquote support in Scala 3, preventing many community rules to easily cross-build and therefore requiring support for loading 2.13 quasiquotes-backed community rules with scalafix-cli_3 if we want a smooth transition to scalafixBinaryScalaVersion := 3 (driven by ExplicitResultType and the usage of the PC). But I believe there is and ongoing effort on porting the macros to Scala 3?

@tgodzik
Copy link
Contributor

tgodzik commented Nov 10, 2023

I am unsure about the need for a Java API wrapper (thinking about the classloading penalty mostly),

We don't have to do a separate classloader for sure, we needed it in Metals mostly for supporting multiple Scala versions at the same time.

But I believe there is and ongoing effort on porting the macros to Scala 3?

Yes! I almost managed to get it cross publishing, I should get back to it soon.

@TonioGela
Copy link

Any news on this front @tgodzik ?

@tgodzik
Copy link
Contributor

tgodzik commented Jul 19, 2024

I will take a look at a PoC if I have time on the weekend.

@tgodzik
Copy link
Contributor

tgodzik commented Jul 21, 2024

So this PoC seems to work -> https://github.com/scalacenter/scalafix/compare/main...tgodzik:scalafix:add-inferred-type?expand=1
with the major caveat:

  • imports are added in a random order, but that should probably be handled by organize imports rule later on (not sure how that works together) This can be especially seen in this file or we can fix it on the compiler side.

Some further work:

  • I need to tidy up and make sure code is not duplicated
  • download specific version of the presentation compiler for each version using coursier
  • some minor code was commented out, I need to get it working again
  • some fixes might be needed on the compiler side

@bjaglin What do you think about this approach? Is there anything disqualifying about it?

Currently the only thing that is really changed is defnType method and the constructor

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

No branches or pull requests

6 participants