-
Notifications
You must be signed in to change notification settings - Fork 186
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
Upgrade to scalameta 3.7.0 with default settings. #675
Changes from 1 commit
cc9d864
7c733f0
9dcf454
3da338d
f3653e4
7e1b69b
7845d60
25b8d27
ef47a2d
5414a5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,6 @@ import java.util.concurrent.atomic.AtomicReference | |
import java.util.function.UnaryOperator | ||
import java.util.regex.Pattern | ||
import java.util.regex.PatternSyntaxException | ||
|
||
import scala.meta._ | ||
import scala.meta.inputs.Input | ||
import scala.meta.internal.inputs._ | ||
|
@@ -42,6 +41,7 @@ import scalafix.syntax._ | |
import metaconfig.Configured.Ok | ||
import metaconfig._ | ||
import metaconfig.ConfError | ||
import scalafix.internal.cli.ClasspathOps | ||
|
||
sealed abstract case class CliRunner( | ||
sourceroot: AbsolutePath, | ||
|
@@ -89,7 +89,11 @@ sealed abstract case class CliRunner( | |
val code = exitCode.get() | ||
if (config.lint.reporter.hasErrors) { | ||
ExitStatus.merge(ExitStatus.LinterError, code) | ||
} else code | ||
} else if (config.reporter.hasErrors) { | ||
ExitStatus.merge(ExitStatus.UnexpectedError, code) | ||
} else { | ||
code | ||
} | ||
} | ||
display.completedTask(msg, exit == ExitStatus.Ok) | ||
display.stop() | ||
|
@@ -315,8 +319,8 @@ object CliRunner { | |
.map(path => AbsolutePath(path)(common.workingPath)) | ||
.toList | ||
|
||
def resolveClasspath: Configured[Classpath] = | ||
classpath match { | ||
def resolveClasspath: Configured[Classpath] = { | ||
classDirectory match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker but I don't like that this name hints that the classpath is a single directory entry, which is not what I usually see for a classpath. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree this is confusing. Note that the cli still accepts --classpath as an alias for --class-directory. I renamed for consistency with sbt The public facing API is fully compatible with the previous behavior despite this change so we have wiggle room to improve on this in the future. |
||
case Some(cp) => | ||
val paths = toClasspath(cp) | ||
Ok(Classpath(paths)) | ||
|
@@ -336,6 +340,7 @@ object CliRunner { | |
.notOk | ||
} | ||
} | ||
} | ||
|
||
val resolvedSourceroot: Configured[AbsolutePath] = { | ||
val result = sourceroot | ||
|
@@ -357,9 +362,17 @@ object CliRunner { | |
val result: Configured[SemanticdbIndex] = cachedDatabase.getOrElse { | ||
(resolveClasspath |@| resolvedSourceroot).andThen { | ||
case (classpath, root) => | ||
val patched = Database.load(classpath, Sourcepath(root)) | ||
val db = | ||
EagerInMemorySemanticdbIndex(patched, Sourcepath(root), classpath) | ||
val index = Database.load(classpath, Sourcepath(root)) | ||
val deps = dependencyClasspath.map(toClasspath).getOrElse(Nil) | ||
val symbolTable = ClasspathOps.newSymbolTable( | ||
Classpath(classpath.shallow ++ deps), | ||
common.out) | ||
val db = EagerInMemorySemanticdbIndex( | ||
index, | ||
Sourcepath(root), | ||
classpath, | ||
symbolTable | ||
) | ||
if (verbose) { | ||
common.err.println( | ||
s"Loaded database with ${db.documents.length} documents.") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,41 @@ | ||
package scalafix.internal.cli | ||
|
||
import java.io.File | ||
import java.io.PrintStream | ||
import scala.meta.Classpath | ||
import scala.meta.metacp | ||
import scalafix.internal.util.LazySymbolTable | ||
import scalafix.internal.util.SymbolTable | ||
|
||
object ClasspathOps { | ||
|
||
def bootClasspath: Option[Classpath] = sys.props.collectFirst { | ||
case (k, v) if k.endsWith(".boot.class.path") => Classpath(v) | ||
} | ||
|
||
/** | ||
* Process classpath with metacp to build semanticdbs of global symbols. | ||
* | ||
* @param sclasspath Regular classpath to process. | ||
* @param out The output stream to print out error messages. | ||
*/ | ||
def toMetaClasspath(sclasspath: Classpath, out: PrintStream): Classpath = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that these methods take a printstream looks really awkward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. awkward sure, but what do you propose instead? This is not a public API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use System.out directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scalafix runs via nailgun, in IDEs with custom logging, etc, It's not OK to use System.out |
||
val withJDK = Classpath( | ||
bootClasspath.fold(sclasspath.shallow)(_.shallow ::: sclasspath.shallow)) | ||
val settings = metacp | ||
.Settings() | ||
.withClasspath(withJDK) | ||
.withScalaLibrarySynthetics(true) | ||
val reporter = metacp.Reporter().withOut(out) | ||
val mclasspath = scala.meta.cli.Metacp.process(settings, reporter).get | ||
mclasspath | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it okay to crash if metacp returns None? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've refactored the code to propagate the error and report a helpful message on failure. |
||
} | ||
|
||
def newSymbolTable(classpath: Classpath, out: PrintStream): SymbolTable = { | ||
val mclasspath = toMetaClasspath(classpath, out) | ||
new LazySymbolTable(mclasspath) | ||
} | ||
|
||
def getCurrentClasspath: String = { | ||
Thread.currentThread.getContextClassLoader match { | ||
case url: java.net.URLClassLoader => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,12 +55,20 @@ case class ScalafixOptions( | |
sourceroot: Option[String] = None, | ||
@HelpMessage( | ||
"java.io.File.pathSeparator separated list of directories or jars containing " + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a list or a single directory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It remains a list, unchanged from the previous behavior of --classpath. I've renamed it back to --classpath and added more details about --dependency-classpath. |
||
"'.semanticdb' files. The 'semanticdb' files are emitted by the " + | ||
"'.semanticdb' files. SemanticDB files are emitted by the " + | ||
"semanticdb-scalac compiler plugin and are necessary for semantic rules " + | ||
"like ExplicitResultTypes to function." | ||
) | ||
@ValueDescription("entry1.jar:entry2.jar:target/scala-2.12/classes/") | ||
classpath: Option[String] = None, | ||
@ExtraName("classpath") | ||
classDirectory: Option[String] = None, | ||
@HelpMessage( | ||
"java.io.File.pathSeparator separated list of directories or jars for dependencies. " + | ||
"Does not need to contain SemanticDB files. " + | ||
"Required for semantic rules like ExplicitResultTypes to work." | ||
) | ||
@ValueDescription("entry1.jar:entry2.jar:target/scala-2.12/classes/") | ||
dependencyClasspath: Option[String] = None, | ||
@HelpMessage( | ||
"Automatically infer --classpath starting from these directories. " + | ||
"Ignored if --classpath is provided.") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import scalafix.syntax._ | |
import scalafix.util.TokenOps | ||
import metaconfig.Conf | ||
import metaconfig.Configured | ||
import org.langmeta.internal.semanticdb.XtensionDenotationsInternal | ||
|
||
case class ExplicitResultTypes( | ||
index: SemanticdbIndex, | ||
|
@@ -74,9 +75,16 @@ case class ExplicitResultTypes( | |
def defnType(defn: Defn): Option[(Type, Patch)] = | ||
for { | ||
name <- defnName(defn) | ||
symbol <- name.symbol | ||
typ <- symbol.resultType | ||
} yield TypeSyntax.prettify(typ, ctx, config.unsafeShortenNames) | ||
denot <- name.denotation | ||
tpe <- denot.tpeInternal | ||
// Skip existential types for now since they cause problems. | ||
if !tpe.tag.isExistentialType | ||
result <- TypeSyntax.prettify( | ||
tpe, | ||
ctx, | ||
config.unsafeShortenNames, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this config passed? https://scalacenter.github.io/scalafix/docs/users/configuration shows how to add rules to the scalafix config, but does not show if it is possible to pass rule specific config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's configuration for ExplicitResultTypes documented here https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn’t able to put that together from the docs for some reason but I get it now, thanks. |
||
name.pos) | ||
} yield result | ||
import scala.meta._ | ||
def fix(defn: Defn, body: Term): Patch = { | ||
val lst = ctx.tokenList | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package scalafix.internal.util | ||
|
||
import org.langmeta.semanticdb.Symbol | ||
import scala.meta.Denotation | ||
import scalafix.SemanticdbIndex | ||
import org.langmeta.internal.semanticdb._ | ||
import scala.meta.internal.{semanticdb3 => s} | ||
|
||
/** | ||
* Combination of a SymbolTable and SemanticdbIndex. | ||
* | ||
* This utility is necessary because SymbolTable contains only global symbols and | ||
* SemanticdbIndex has local symbols. | ||
*/ | ||
class CombinedSymbolTable(index: SemanticdbIndex, table: SymbolTable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to pass both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, I've merged this into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is combined symbol table needed if shortenNames is false? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's needed both when shortenNames is true and false. |
||
private def denotationToSymbolInformation( | ||
symbol: String, | ||
denot: Denotation): s.SymbolInformation = { | ||
s.SymbolInformation( | ||
symbol = symbol, | ||
language = s.Language.SCALA, | ||
kind = s.SymbolInformation.Kind.fromValue(denot.skind.value), | ||
properties = denot.sproperties, | ||
name = denot.name, | ||
tpe = denot.tpeInternal | ||
) | ||
} | ||
def info(symbol: String): s.SymbolInformation = | ||
table.info(symbol).getOrElse { | ||
denotationToSymbolInformation( | ||
symbol, | ||
index | ||
.denotation(Symbol(symbol)) | ||
.getOrElse(throw new NoSuchElementException(symbol)) | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this formatting change intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for consistency with the changes below