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

Implement NoInfer linter. #333

Merged
merged 3 commits into from
Sep 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion project/Dependencies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
3 changes: 3 additions & 0 deletions readme/Changelog05.scalatex
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 19 additions & 2 deletions readme/Rules.scalatex
Original file line number Diff line number Diff line change
Expand Up @@ -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").
1 change: 1 addition & 0 deletions readme/src/main/scala/scalafix/Readme.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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(
"""<a href="https://gitter.im/scalacenter/scalafix?utm_source=badge&amp;utm_medium=badge&amp;utm_campaign=pr-badge&amp;utm_content=badge"><img src="https://camo.githubusercontent.com/382ebf95f5b4df9275ac203229928db8c8fd5c50/68747470733a2f2f6261646765732e6769747465722e696d2f6f6c6166757270672f7363616c61666d742e737667" alt="Join the chat at https://gitter.im/scalacenter/scalafix" data-canonical-src="https://badges.gitter.im/scalacenter/scalafix.svg" style="max-width:100%;"></a>""")
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 */
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 8 additions & 21 deletions scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ object ScalafixRules {
DottyVarArgPattern
)
def semantic(index: SemanticdbIndex): List[Rule] = List(
NoInfer(index),
Sbt1(index),
ExplicitResultTypes(index),
RemoveUnusedImports(index),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does the // assert comment work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can assert which linter message is reported at which specfic lines https://github.com/scalacenter/scalafix/blob/master/scalafix-tests/input/src/main/scala/test/NoInfer.scala#L7

The test fails if

  • the linter category doesn't match,
  • a message is not reported on that line
  • a message is reported at a line without an assert

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! This is only for testkit right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is only testkit. Escape hatches for users are yet unimplemented #241

|""".stripMargin
throw new TestFailedException(
s"Uncaught linter messages! $explanation",
Expand All @@ -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 =
Expand All @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ case class MyRule2(index: SemanticdbIndex) extends SemanticRule(index) {
def rule(ctx: RuleCtx): Patch =
ctx.addGlobalImport(importer"scala.collection.immutable.Seq")
}

17 changes: 17 additions & 0 deletions scalafix-tests/input/src/main/scala/test/NoInfer.scala
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ class RemoveXmlLiterals {
}

object I {
val a = <div>{{</div>// scalafix: RemoveXmlLiterals.singleBracesEscape
val b = <div>}}</div>// scalafix: RemoveXmlLiterals.singleBracesEscape
val a = <div>{{</div>// assert: RemoveXmlLiterals.singleBracesEscape
val b = <div>}}</div>// assert: RemoveXmlLiterals.singleBracesEscape
// <div b="{{"/> >>> xml"""<div b="{{"/>"""
}

Expand Down