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

Fix #266: Extended NoInfer adding extraSymbols field #355

Closed
wants to merge 1 commit into from
Closed
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package scalafix.internal.config

import metaconfig._
import org.langmeta._

case class NoInferConfig(
Copy link
Contributor

Choose a reason for hiding this comment

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

God there is an incredible amount of ceremony involved around adding configuration, we should try to come up with something easier 🤔

extraSymbols: List[Symbol] = Nil
) {
implicit val reader: ConfDecoder[NoInferConfig] =
ConfDecoder.instanceF[NoInferConfig] { c =>
c.getOrElse("extraSymbols")(extraSymbols).map(xs => NoInferConfig(xs))
}
implicit val symbolReader: ConfDecoder[Symbol] =
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a Symbol.Global reader in scalafix.internal.config which we should use instead, it handles a few corner cases.

ConfDecoder.instanceF[Symbol] { c =>
c.as[String].map(Symbol.apply)
}
}

object NoInferConfig {
val default = NoInferConfig()
implicit val reader: ConfDecoder[NoInferConfig] = default.reader
}
Original file line number Diff line number Diff line change
@@ -1,25 +1,39 @@
package scalafix.internal.rule

import metaconfig.{Conf, Configured}

import scala.meta._
import scalafix.internal.config.NoInferConfig
import scalafix.lint.LintCategory
import scalafix.lint.LintMessage
import scalafix.rule.RuleCtx
import scalafix.rule.SemanticRule
import scalafix.rule.{Rule, RuleCtx, SemanticRule}
import scalafix.util.SemanticdbIndex
import scalafix.util.SymbolMatcher

case class NoInfer(index: SemanticdbIndex)
case class NoInfer(index: SemanticdbIndex,
config: NoInferConfig = NoInferConfig.default)
extends SemanticRule(index, "NoInfer")
with Product {
private val badSymbol = SymbolMatcher.exact(NoInfer.badSymbols: _*)
private val badSymbol = SymbolMatcher.exact(NoInfer.badSymbols ++ config.extraSymbols: _*)

def this(index: SemanticdbIndex) =
this(index, NoInferConfig.default)

override def init(config: Conf): Configured[Rule] =
config
.getOrElse("noInfer")(NoInferConfig.default)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to think a good convention is to make the configuration key match exactly with the rule name (instead of lowercasing first character like I've done until now). What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 to this

Copy link
Author

Choose a reason for hiding this comment

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

I agree. It would feel more intuitive without the lower case.

.map(c => NoInfer(index, c))

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 {

override def check(ctx: RuleCtx): Seq[LintMessage] = {

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Author

@bmatthews68 bmatthews68 Sep 18, 2017

Choose a reason for hiding this comment

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

No. I didn't undo enough when backtracking a change. I will clean it up.

ctx.index.synthetics.flatMap {
case Synthetic(pos, _, names) =>
names.collect {
case ResolvedName(_, badSymbol(Symbol.Global(_, signature)), _) =>
Expand All @@ -29,6 +43,7 @@ case class NoInfer(index: SemanticdbIndex)
.at(s"Inferred ${signature.name}", pos)
}
}
}
}

case object NoInfer {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
rules = [
ProcedureSyntax
NoAutoTupling
]
]

NoInfer.extraSymbols = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Author

Choose a reason for hiding this comment

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

No. I will take it out.

"_root_.scala.Predef.any2stringadd."
]
5 changes: 5 additions & 0 deletions scalafix-tests/input/src/main/scala/test/NoInfer.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
/*
rule = NoInfer
noInfer.extraSymbols = [
"_root_.test.NoInfer.B#"
]
*/
package test

Expand All @@ -14,4 +17,6 @@ case object NoInfer {
}
case class A()
List(NoInfer, A())// assert: NoInfer.product
case class B()
List(B())// assert: NoInfer.b
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

}