diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 1eeeff10e..5e884c1df 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -4,7 +4,7 @@ import org.scalajs.sbtplugin.ScalaJSPlugin.autoImport._ object Dependencies { - val scalametaV = "2.0.0-RC2" + val scalametaV = "2.0.0-RC3" val metaconfigV = "0.5.2" def semanticdbSbt = "0.4.0" def dotty = "0.1.1-bin-20170530-f8f52cc-NIGHTLY" diff --git a/readme/Changelog05.scalatex b/readme/Changelog05.scalatex index 1dd4ab056..b0c3eee6b 100644 --- a/readme/Changelog05.scalatex +++ b/readme/Changelog05.scalatex @@ -19,6 +19,9 @@ @li @sect.ref{Sbt1} rule to migrate usage of deprecated sbt 0.13 APIs, by @user{jvican}. + @li + @sect.ref{NoInfer} linter that reports errors when the compiler reports + types such as @code{Any} @li @sect.ref{ExplicitResultTypes} experimental @code{unsafeShortenNames} option to insert imports instead of fully qualified names. diff --git a/readme/Rules.scalatex b/readme/Rules.scalatex index c3544b2d3..0b7096852 100644 --- a/readme/Rules.scalatex +++ b/readme/Rules.scalatex @@ -253,8 +253,25 @@ // after x += (y in Compile).value - This rule currently handles many basic cases but may produce incorrect - output in more advanced cases. Please report back your experience. + @sect(NoInfer.toString) + @since("0.5.0") + + This rule reports errors when the compiler inferred one of + the following types + @ul + @NoInfer.badSymbolNames.map(li(_)) + + Example: + @hl.scala + MyCode.scala:7: error: [NoInfer.any] Inferred Any + List(1, "") + ^ + @h3{Known limitations} + + @ul + @li + Scalafix does not yet expose an way to disable rules across + regions of code, track @issue(241) for updates. @sect{Planned rules...} See @lnk("here", "https://github.com/scalacenter/scalafix/labels/rule"). diff --git a/readme/src/main/scala/scalafix/Readme.scala b/readme/src/main/scala/scalafix/Readme.scala index bce83b336..faec114af 100644 --- a/readme/src/main/scala/scalafix/Readme.scala +++ b/readme/src/main/scala/scalafix/Readme.scala @@ -24,6 +24,7 @@ object Readme { val highlight = new Highlighter {} def note = b("Note. ") def experimental = p(b("Experimental ", fa("warning"))) + def since(version: String) = p(s"Since v$version") def gitter = raw( """Join the chat at https://gitter.im/scalacenter/scalafix""") diff --git a/scalafix-core/shared/src/main/scala/org/langmeta/internal/ScalafixLangmetaHacks.scala b/scalafix-core/shared/src/main/scala/org/langmeta/internal/ScalafixLangmetaHacks.scala new file mode 100644 index 000000000..2eb7ef8f1 --- /dev/null +++ b/scalafix-core/shared/src/main/scala/org/langmeta/internal/ScalafixLangmetaHacks.scala @@ -0,0 +1,28 @@ +package org.langmeta.internal + +import scala.compat.Platform.EOL +import org.langmeta._ + +object ScalafixLangmetaHacks { + // Workaround for https://github.com/scalameta/scalameta/issues/1115 + def formatMessage(pos: Position, severity: String, message: String): String = { + if (pos != Position.None) { + val input = pos.input + val header = + s"${input.syntax}:${pos.startLine + 1}: $severity: $message" + val line = { + val start = input.lineToOffset(pos.startLine) + val notEof = start < input.chars.length + val end = if (notEof) input.lineToOffset(pos.startLine + 1) else start + val count = end - start + val eolOffset = + if (input.chars.lift(start + count - 1).contains('\n')) -1 else 0 + new String(input.chars, start, count + eolOffset) + } + var caret = " " * pos.startColumn + "^" + header + EOL + line + EOL + caret + } else { + s"$severity: $message" + } + } +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala index b7fb28dd6..dbd79cc84 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/config/PrintStreamReporter.scala @@ -6,6 +6,7 @@ import java.io.PrintStream import java.util.concurrent.atomic.AtomicInteger import scalafix.internal.util.Severity import metaconfig._ +import org.langmeta.internal.ScalafixLangmetaHacks /** A ScalafixReporter that emits messages to a PrintStream. */ case class PrintStreamReporter( @@ -40,7 +41,8 @@ case class PrintStreamReporter( val enclosing = if (includeLoggerName) s"(${ctx.enclosing.value}) " else "" outStream.println( - position.formatMessage(enclosing + severity.toString, message)) + ScalafixLangmetaHacks + .formatMessage(position, enclosing + severity.toString, message)) } /** Returns true if this reporter has seen an error */ diff --git a/scalafix-core/shared/src/main/scala/scalafix/internal/rule/NoInfer.scala b/scalafix-core/shared/src/main/scala/scalafix/internal/rule/NoInfer.scala new file mode 100644 index 000000000..12ea863ab --- /dev/null +++ b/scalafix-core/shared/src/main/scala/scalafix/internal/rule/NoInfer.scala @@ -0,0 +1,46 @@ +package scalafix.internal.rule + +import scala.meta._ +import scalafix.lint.LintCategory +import scalafix.lint.LintMessage +import scalafix.rule.RuleCtx +import scalafix.rule.SemanticRule +import scalafix.util.SemanticdbIndex +import scalafix.util.SymbolMatcher + +case class NoInfer(index: SemanticdbIndex) + extends SemanticRule(index, "NoInfer") + with Product { + private val badSymbol = SymbolMatcher.exact(NoInfer.badSymbols: _*) + val error: LintCategory = + LintCategory.error( + """The Scala compiler sometimes infers a too generic type such as Any. + |If this is intended behavior, then the type should be explicitly type + |annotated in the source.""".stripMargin + ) + override def check(ctx: RuleCtx): Seq[LintMessage] = + ctx.index.synthetics.flatMap { + case Synthetic(pos, _, names) => + names.collect { + case ResolvedName(_, badSymbol(Symbol.Global(_, signature)), _) => + val categoryId = signature.name.toLowerCase() + error + .copy(id = categoryId) + .at(s"Inferred ${signature.name}", pos) + } + } +} + +case object NoInfer { + lazy val badSymbols: List[Symbol] = List( + Symbol("_root_.java.io.Serializable#"), + Symbol("_root_.scala.Any#"), + Symbol("_root_.scala.AnyVal#"), + Symbol("_root_.scala.AnyVal#"), + Symbol("_root_.scala.Product#") + ) + + def badSymbolNames: List[String] = badSymbols.collect { + case Symbol.Global(_, signature) => signature.name + } +} diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala index 85f263eb6..f9154f20d 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintCategory.scala @@ -6,7 +6,8 @@ import scala.meta.inputs.Position /** A unique identifier for one kind of a linter message. * * @param id a string ID for this message, typically the name of the - * assigned variable. + * assigned variable. If id is empty, then the name of the + * rewrite reporting this LintCategory is used as id. * @param explanation An optional explanation for this kind of message. * @param severity The default category this message should get reported to. * Note that users can configure/override the default category. @@ -32,6 +33,10 @@ final case class LintCategory( object LintCategory { def error(id: String, explain: String): LintCategory = new LintCategory(id, explain, LintSeverity.Error) + def error(explain: String): LintCategory = + new LintCategory("", explain, LintSeverity.Error) def warning(id: String, explain: String): LintCategory = new LintCategory(id, explain, LintSeverity.Warning) + def warning(explain: String): LintCategory = + new LintCategory("", explain, LintSeverity.Warning) } diff --git a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala index 8ca019d34..4ffe773db 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/lint/LintMessage.scala @@ -24,6 +24,7 @@ final case class LintMessage( |${category.explanation} |""".stripMargin else "" - s"[${owner.value}.${category.id}] $message$explanation" + val id = if (category.id.isEmpty) "" else s".${category.id}" + s"[${owner.value}$id] $message$explanation" } } diff --git a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala index 84177beef..afdd11711 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala @@ -118,8 +118,11 @@ object Patch { private[scalafix] def lintMessages( patch: Patch, - ctx: RuleCtx): List[LintMessage] = { + ctx: RuleCtx, + checkMessages: scala.Seq[LintMessage] + ): List[LintMessage] = { val builder = List.newBuilder[LintMessage] + checkMessages.foreach(builder += _) foreach(patch) { case LintPatch(lint) => builder += lint diff --git a/scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala b/scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala index 83401583b..6984a030d 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala @@ -26,28 +26,12 @@ import metaconfig.Configured * the `check` method. Example: * {{{ * // example syntactic linter - * object NoVars extends Rule("NoVars") { - * val varDefinition = LintCategory.error("varDefinition", "Var is bad!") - * override def check(ctx: RuleCtx) = ctx.tree.collect { - * case definition @ q"$_ var $_ = $_" => varDefinition.at(definition.pos) + * object NoNulls extends Rule("NoNulls") { + * val error = LintCategory.error("Nulls are not allowed.") + * override def check(ctx: RuleCtx): List[LintMessage] = ctx.tree.collect { + * case nil @ q"null" => error.at(nil.pos) * } * } - * // example semantic linter - * case class NeverInferProduct(index: SemanticdbIndex) - * extends SemanticRule(index, "NeverInferProduct") - * with Product { - * val product = SymbolMatcher.exact(Symbol("_root_.scala.Product#")) - * val inferredProduct: LintCategory = - * LintCategory.error("inferredProduct", "Don't infer Product!") - * override def check(ctx: RuleCtx) = - * ctx.index.synthetics.flatMap { - * case Synthetic(pos, text, names) => - * names.collect { - * case ResolvedName(_, product(_), _) => - * inferredProduct.at(pos) - * } - * } - * } * }}} * * @param ruleName @@ -96,7 +80,8 @@ abstract class Rule(ruleName: RuleName) { self => final def apply(input: String): String = apply(Input.String(input)) final def apply(ctx: RuleCtx, patch: Patch): String = { val result = Patch(patch, ctx, semanticOption) - Patch.lintMessages(patch, ctx).foreach { msg => + val checkMessages = check(ctx) + Patch.lintMessages(patch, ctx, checkMessages).foreach { msg => // Set the lint message owner. This allows us to distinguish // LintCategory with the same id from different rules. ctx.printLintMessage(msg, name) @@ -145,6 +130,8 @@ object Rule { .flipSeq(rules.map(_.init(config))) .map(x => new CompositeRule(x.toList)) } + override def check(ctx: RuleCtx): Seq[LintMessage] = + rules.flatMap(_.check(ctx)) override def fix(ctx: RuleCtx): Patch = Patch.empty ++ rules.map(_.fix(ctx)) override def semanticOption: Option[SemanticdbIndex] = diff --git a/scalafix-core/shared/src/main/scala/scalafix/rule/ScalafixRules.scala b/scalafix-core/shared/src/main/scala/scalafix/rule/ScalafixRules.scala index 51bfe0c2e..82bcbb3ab 100644 --- a/scalafix-core/shared/src/main/scala/scalafix/rule/ScalafixRules.scala +++ b/scalafix-core/shared/src/main/scala/scalafix/rule/ScalafixRules.scala @@ -14,6 +14,7 @@ object ScalafixRules { DottyVarArgPattern ) def semantic(index: SemanticdbIndex): List[Rule] = List( + NoInfer(index), Sbt1(index), ExplicitResultTypes(index), RemoveUnusedImports(index), diff --git a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala index 01a5bd7d5..37b8621fe 100644 --- a/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala +++ b/scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala @@ -9,11 +9,11 @@ import org.scalameta.logger import org.scalatest.BeforeAndAfterAll import org.scalatest.FunSuite import org.scalatest.exceptions.TestFailedException -import scala.meta.internal.inputs.XtensionPositionFormatMessage import scala.util.matching.Regex +import org.langmeta.internal.ScalafixLangmetaHacks object SemanticRuleSuite { - val LintAssertion: Regex = " scalafix: (.*)".r + val LintAssertion: Regex = " assert: (.*)".r def stripTestkitComments(input: String): String = stripTestkitComments(input.tokenize.get) def stripTestkitComments(tokens: Tokens): String = { @@ -69,20 +69,21 @@ abstract class SemanticRuleSuite( tokens: Tokens): Unit = { val lintMessages = lints.to[mutable.Set] def assertLint(position: Position, key: String): Unit = { - val matchingMessage = lintMessages.find { m => - // NOTE(olafur) I have no idea why -1 is necessary. - m.position.startLine == (position.startLine - 1) && + val matchingMessage = lintMessages.filter { m => + assert(m.position.input == position.input) + m.position.startLine == position.startLine && m.category.key(rule.name) == key } - matchingMessage match { - case Some(x) => - lintMessages -= x - case None => - throw new TestFailedException( - position - .formatMessage("error", s"Message '$key' was not reported here!"), - 0 - ) + if (matchingMessage.isEmpty) { + throw new TestFailedException( + ScalafixLangmetaHacks.formatMessage( + position, + "error", + s"Message '$key' was not reported here!"), + 0 + ) + } else { + lintMessages --= matchingMessage } } tokens.foreach { @@ -95,7 +96,7 @@ abstract class SemanticRuleSuite( val key = lintMessages.head.category.key(rule.name) val explanation = s"""|To fix this problem, suffix the culprit lines with - | // scalafix: $key + | // assert: $key |""".stripMargin throw new TestFailedException( s"Uncaught linter messages! $explanation", @@ -113,11 +114,12 @@ abstract class SemanticRuleSuite( val patch = rule.fix(ctx) val obtainedWithComment = Patch.apply(patch, ctx, rule.semanticOption) val tokens = obtainedWithComment.tokenize.get + val checkMessages = rule.check(ctx) assertLintMessagesAreReported( rule, ctx, - Patch.lintMessages(patch, ctx), - tokens) + Patch.lintMessages(patch, ctx, checkMessages), + ctx.tokens) val obtained = SemanticRuleSuite.stripTestkitComments(tokens) val candidateOutputFiles = expectedOutputSourceroot.flatMap { root => val scalaSpecificFilename = @@ -126,17 +128,20 @@ abstract class SemanticRuleSuite( diffTest.filename.toString().replaceFirst("scala", path)))) root.resolve(diffTest.filename) :: scalaSpecificFilename } - val candidateBytes = candidateOutputFiles + val expected = candidateOutputFiles .collectFirst { case f if f.isFile => f.readAllBytes } + .map(new String(_)) .getOrElse { - val tried = candidateOutputFiles.mkString("\n") - sys.error( - s"""Missing expected output file for test ${diffTest.filename}. Tried: - |$tried""".stripMargin) + // TODO(olafur) come up with more principled check to determine if + // rule is linter or rewrite. + if (checkMessages.nonEmpty) obtained // linter + else { + val tried = candidateOutputFiles.mkString("\n") + sys.error( + s"""Missing expected output file for test ${diffTest.filename}. Tried: + |$tried""".stripMargin) + } } - val expected = new String( - candidateBytes - ) assertNoDiff(obtained, expected) } } diff --git a/scalafix-tests/input/src/main/resources/rewrites/MyRule.scala b/scalafix-tests/input/src/main/resources/rewrites/MyRule.scala index f1e9e49d3..1ac8344f5 100644 --- a/scalafix-tests/input/src/main/resources/rewrites/MyRule.scala +++ b/scalafix-tests/input/src/main/resources/rewrites/MyRule.scala @@ -20,3 +20,4 @@ case class MyRule2(index: SemanticdbIndex) extends SemanticRule(index) { def rule(ctx: RuleCtx): Patch = ctx.addGlobalImport(importer"scala.collection.immutable.Seq") } + diff --git a/scalafix-tests/input/src/main/scala/test/NoInfer.scala b/scalafix-tests/input/src/main/scala/test/NoInfer.scala new file mode 100644 index 000000000..dfcd17f1d --- /dev/null +++ b/scalafix-tests/input/src/main/scala/test/NoInfer.scala @@ -0,0 +1,17 @@ +/* +rule = NoInfer +*/ +package test + +case object NoInfer { + val x = List(1, "")// assert: NoInfer.any + x.map(x => x -> x)// assert: NoInfer.any + List[Any](1, "") // OK, not reported message + List[Any](1, "").map(identity[Any])/*(canBuildFrom[Any])*/// assert: NoInfer.any + (null: Any) + null match { + case _: Any => + } + case class A() + List(NoInfer, A())// assert: NoInfer.product +} diff --git a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala index 78fd3b439..a30539c6a 100644 --- a/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala +++ b/scalafix-tests/input/src/main/scala/test/RemoveXmlLiterals.scala @@ -45,8 +45,8 @@ class RemoveXmlLiterals { } object I { - val a =
{{
// scalafix: RemoveXmlLiterals.singleBracesEscape - val b =
}}
// scalafix: RemoveXmlLiterals.singleBracesEscape + val a =
{{
// assert: RemoveXmlLiterals.singleBracesEscape + val b =
}}
// assert: RemoveXmlLiterals.singleBracesEscape //
>>> xml"""
""" }