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.

Sign up for GitHub

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

Jump to bottom

Small refactoring of explicitResultTypes #1439

Merged
merged 1 commit into from
Mar 16, 2022
Merged

Small refactoring of explicitResultTypes #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions scalafix-core/src/main/scala/scalafix/internal/v1/LazyValue.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,6 @@ object LazyValue {
def later[T](e: () => T): LazyValue[T] = new LazyValue[T](e)
def fromUnsafe[T](e: () => T): LazyValue[Option[T]] =
new LazyValue[Option[T]](() => Try(e()).toOption)
def from[T](e: () => Try[T]): LazyValue[Option[T]] =
new LazyValue[Option[T]](() => e().toOption)
}
21 changes: 15 additions & 6 deletions scalafix-rules/src/main/scala/scala/meta/internal/pc/ScalafixGlobal.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package scala.meta.internal.pc

import java.io.File
import java.{util => ju}

import scala.collection.mutable
import scala.reflect.internal.{Flags => gf}
import scala.reflect.io.VirtualDirectory
Expand All @@ -11,9 +10,9 @@ import scala.tools.nsc.interactive.Global
import scala.tools.nsc.reporters.StoreReporter
import scala.util.control.NonFatal
import scala.{meta => m}

import scala.meta.internal.semanticdb.scalac.SemanticdbOps
import scala.meta.io.AbsolutePath
import scala.util.{Failure, Success, Try}
// used to cross-compile
import scala.collection.compat._ // scalafix:ok

Expand All @@ -22,7 +21,7 @@ object ScalafixGlobal {
cp: List[AbsolutePath],
options: List[String],
symbolReplacements: Map[String, String]
): ScalafixGlobal = {
): Try[ScalafixGlobal] = {
val classpath = cp.mkString(File.pathSeparator)
val vd = new VirtualDirectory("(memory)", None)
val settings = new Settings
Expand All @@ -35,9 +34,19 @@ object ScalafixGlobal {
}
val (isSuccess, unprocessed) =
settings.processArguments(options, processAll = true)
require(isSuccess, unprocessed)
require(unprocessed.isEmpty, unprocessed)
new ScalafixGlobal(settings, new StoreReporter(), symbolReplacements)
(isSuccess, unprocessed) match {
case (true, Nil) =>
Try(
new ScalafixGlobal(settings, new StoreReporter(), symbolReplacements)
)
case (isSuccess, unprocessed) =>
Failure(
new Exception(
s"newGlobal failed while processing Arguments. " +
s"Status is $isSuccess, unprocessed arguments are $unprocessed"
)
)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions scalafix-rules/src/main/scala/scalafix/internal/rule/CompilerTypePrinter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ import scalafix.v1

class CompilerTypePrinter(g: ScalafixGlobal, config: ExplicitResultTypesConfig)(
implicit ctx: v1.SemanticDocument
) extends TypePrinter {
) {
import g._
private lazy val unit =
g.newCompilationUnit(ctx.input.text, ctx.input.syntax)
private val willBeImported = mutable.Map.empty[Name, ShortName]
private val isInsertedClass = mutable.Set.empty[String]

override def toPatch(
def toPatch(
pos: m.Position,
sym: v1.Symbol,
replace: m.Token,
Expand Down
60 changes: 31 additions & 29 deletions scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ final class ExplicitResultTypes(
if (config.scalacClasspath.isEmpty) {
LazyValue.now(None)
} else {
LazyValue.fromUnsafe { () =>
LazyValue.from { () =>
ScalafixGlobal.newCompiler(
config.scalacClasspath,
config.scalacOptions,
Expand Down Expand Up @@ -92,35 +92,32 @@ final class ExplicitResultTypes(
}
}

override def fix(implicit ctx: SemanticDocument): Patch = {
override def fix(implicit ctx: SemanticDocument): Patch =
try unsafeFix()
catch {
case _: CompilerException =>
shutdownCompiler()
Copy link
Collaborator Author

@mlachkar mlachkar Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler crashes sometimes because it cannot type a part of the AST. If it crashes once, it will crash again even after a retry.
Actually we do that on the file level, which means if one explicit type makes the compiler crash, we don't add any types to the entire file. Should we continue to do that ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler crashes sometimes because it cannot type a part of the AST. If it crashes once, it will crash again even after a retry.

Is that the only case? ba74db3 is not very verbose, I am not sure... In any case, if you reproduced the issue, it would be good to document the cases where the compiler crashes and how it behaves via tests.

Actually we do that on the file level, which means if one explicit type makes the compiler crash, we don't add any types to the entire file. Should we continue to do that ?

Catching the exception at a more granular seems like a good idea indeed. Maybe this wasn't done for performance reason because some files would crash the compiler over and over again?

global.restart()
try unsafeFix()
catch {
case _: CompilerException if !config.fatalWarnings =>
// Ignore compiler crashes unless `fatalWarnings = true`.
Patch.empty
}
case _: CompilerException if !config.fatalWarnings =>
Patch.empty
}
}

def unsafeFix()(implicit ctx: SemanticDocument): Patch = {
lazy val types = TypePrinter(global.value, config)
ctx.tree.collect {
case t @ Defn.Val(mods, Pat.Var(name) :: Nil, None, body)
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)

case t @ Defn.Var(mods, Pat.Var(name) :: Nil, None, Some(body))
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)

case t @ Defn.Def(mods, name, _, _, None, body)
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)
}.asPatch
global.value match {
case Some(value) =>
val types = new CompilerTypePrinter(value, config)
ctx.tree.collect {
case t @ Defn.Val(mods, Pat.Var(name) :: Nil, None, body)
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)

case t @ Defn.Var(mods, Pat.Var(name) :: Nil, None, Some(body))
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)

case t @ Defn.Def(mods, name, _, _, None, body)
if isRuleCandidate(t, name, mods, body) =>
fixDefinition(t, body, types)
}.asPatch
case None => Patch.empty
}
Comment on lines +103 to +120
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old code vs new code:

  • in old code, we will always traverse the entire Tree of the file even if we were not able to instantiate the global, which can only happen if the classpath is wrong
  • new code: we always instantiate the global, even if there no element (def, or var or val) in the Tree. But in the other hand we remove an indirection where we create a TypePrinter just to wrap the case where ScalafixGlobal is None.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with global, but c6014ce did introduce this for something else than an incorrect classpath it seems

}

// Don't explicitly annotate vals when the right-hand body is a single call
Expand Down Expand Up @@ -195,16 +192,21 @@ final class ExplicitResultTypes(
}
}

def defnType(defn: Defn, replace: Token, space: String, types: TypePrinter)(
implicit ctx: SemanticDocument
def defnType(
defn: Defn,
replace: Token,
space: String,
types: CompilerTypePrinter
)(implicit
ctx: SemanticDocument
): Option[Patch] =
for {
name <- defnName(defn)
defnSymbol <- name.symbol.asNonEmpty
patch <- types.toPatch(name.pos, defnSymbol, replace, defn, space)
} yield patch

def fixDefinition(defn: Defn, body: Term, types: TypePrinter)(implicit
def fixDefinition(defn: Defn, body: Term, types: CompilerTypePrinter)(implicit
ctx: SemanticDocument
): Patch = {
val lst = ctx.tokenList
Expand Down
28 changes: 0 additions & 28 deletions scalafix-rules/src/main/scala/scalafix/internal/rule/TypePrinter.scala
View file
Open in desktop

This file was deleted.

12 changes: 7 additions & 5 deletions scalafix-tests/unit/src/test/scala/scalafix/tests/rule/MavenFuzzSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,13 @@ class MavenFuzzSuite extends AnyFunSuite with DiffAssertions {
}

tmp.toFile().deleteOnExit()
val g = ScalafixGlobal.newCompiler(
classfiles.map(AbsolutePath(_)).toList,
Nil,
Map.empty
)
val g = ScalafixGlobal
.newCompiler(
classfiles.map(AbsolutePath(_)).toList,
Nil,
Map.empty
)
.get
val paths = getCompilingSources(g, classfiles, sources, tmp)
exec("git", "init")
exec("git", "add", ".")
Expand Down