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

[DONOTMERGE] Upgrade Disable rule to scalafix v1 #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
@@ -1,2 +1,2 @@
scalafix.internal.scaluzzi.MissingFinal
scalafix.internal.scaluzzi.DisableLegacy
scalafix.internal.scaluzzi.MissingFinal
scalafix.internal.scaluzzi.Disable
121 changes: 58 additions & 63 deletions scalafix/rules/src/main/scala/scalafix/internal/scaluzzi/Disable.scala
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package scalafix.internal.scaluzzi

import metaconfig.{Conf, Configured}

import scala.meta._
import scala.meta.transversers.Traverser
import scalafix.internal.v0.InputSynthetic
import scalafix.internal.util.SymbolOps
import scalafix.internal.v0.LegacySemanticRule
import scalafix.v0._
import scalafix.v1._

import scala.annotation.tailrec

object Disable {

Expand Down Expand Up @@ -45,91 +46,90 @@ object Disable {
}
}

final class DisableSymbolMatcher(symbols: List[DisabledSymbol])(
implicit index: SemanticdbIndex) {
final class DisableSymbolMatcher(symbols: List[DisabledSymbol]) {
def findMatch(symbol: Symbol): Option[DisabledSymbol] =
symbols.find(_.matches(symbol))

def unapply(tree: Tree): Option[(Tree, DisabledSymbol)] =
index
.symbol(tree)
.flatMap(findMatch(_).map(ds => (tree, ds)))
def unapply(tree: Tree)(implicit doc: SemanticDocument): Option[(Tree, DisabledSymbol)] =
findMatch(tree.symbol).map(ds => (tree, ds))

def unapply(symbol: Symbol): Option[(Symbol, DisabledSymbol)] =
findMatch(symbol).map(ds => (symbol, ds))
}
}

final case class Disable(index: SemanticdbIndex, config: DisableConfig)
extends SemanticRule(index, "Disable") {
case class DisableDiagnostic(symbol: Symbol, details: String, position: Position) extends Diagnostic {
override def message: String =
s"${symbol.structure} is disabled $details"

import Disable._
}

private lazy val errorCategory: LintCategory =
LintCategory.error(
"""Some constructs are unsafe to use and should be avoided""".stripMargin
)

final case class Disable(config: DisableConfig)
extends SemanticRule("Disable") {

import Disable._

override def description: String =
"Linter that reports an error on a configurable set of symbols."

override def init(config: Conf): Configured[Rule] =
config
.getOrElse("disable", "Disable")(DisableConfig.default)
.map(Disable(index, _))
override def withConfiguration(config: Configuration): Configured[Rule] = {
// should this use DisableConfig.default?
config.conf.getOrElse("disable", "Disable")(this.config).map { newConfig => Disable(newConfig) }
Copy link
Author

Choose a reason for hiding this comment

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

Not sure about this bit

}

private val safeBlocks = new DisableSymbolMatcher(config.allSafeBlocks)
private val disabledSymbolInSynthetics =
new DisableSymbolMatcher(config.ifSynthetic)

private def createLintMessage(
symbol: Symbol.Global,
symbol: Symbol,
disabled: DisabledSymbol,
pos: Position,
details: String = ""): Diagnostic = {
val message = disabled.message.getOrElse(
s"${symbol.signature.name} is disabled$details")

val id = disabled.id.getOrElse(symbol.signature.name)

errorCategory
.copy(id = id)
.at(message, pos)
// val message = disabled.message.getOrElse(
// s"${symbol.signature.name} is disabled$details")
//
// val id = disabled.id.getOrElse(symbol.signature.name)
//
// errorCategory
// .copy(id = id)
// .at(message, pos)
DisableDiagnostic(symbol, details = details, position = pos)
Copy link
Author

Choose a reason for hiding this comment

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

Needs to be cleaned up

}

private def checkTree(ctx: RuleCtx): Seq[Diagnostic] = {
private def checkTree(implicit doc: SemanticDocument): Seq[Patch] = {
def filterBlockedSymbolsInBlock(
blockedSymbols: List[DisabledSymbol],
block: Tree): List[DisabledSymbol] =
ctx.index.symbol(block) match {
case Some(symbolBlock: Symbol.Global) =>
val symbolsInMatchedBlocks =
config.unlessInside.flatMap(
u =>
if (u.safeBlocks.exists(_.matches(symbolBlock))) u.symbols
else List.empty)
val res = blockedSymbols.filterNot(symbolsInMatchedBlocks.contains)
res
case _ => blockedSymbols
}

block: Tree): List[DisabledSymbol] = {
val symbolBlock = block.symbol
val symbolsInMatchedBlocks =
config.unlessInside.flatMap(
u =>
if (u.safeBlocks.exists(_.matches(symbolBlock))) u.symbols
else List.empty)
blockedSymbols.filterNot(symbolsInMatchedBlocks.contains)
}

@tailrec
def skipTermSelect(term: Term): Boolean = term match {
case _: Term.Name => true
case Term.Select(q, _) => skipTermSelect(q)
case _ => false
}

def handleName(t: Name, blockedSymbols: List[DisabledSymbol])
: Either[Diagnostic, List[DisabledSymbol]] = {
: Either[Patch, List[DisabledSymbol]] = {
val isBlocked = new DisableSymbolMatcher(blockedSymbols)
ctx.index.symbol(t) match {
case Some(isBlocked(s: Symbol.Global, disabled)) =>
SymbolOps.normalize(s) match {
case g: Symbol.Global if g.signature.name != "<init>" =>
Left(createLintMessage(g, disabled, t.pos))
case _ => Right(blockedSymbols)
}
case _ => Right(blockedSymbols)
val s = t.symbol
isBlocked.findMatch(s).map { disabled =>
SymbolOps.normalize(s) match {
case g: Symbol if g.info.get.signature.toString() != "<init>" =>
Left(Patch.lint(DisableDiagnostic(s, "", t.pos)).atomic) // XXX this is incorrect
Copy link
Author

Choose a reason for hiding this comment

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

Fix this

case _ => Right(blockedSymbols)
}
}.getOrElse {
Right(blockedSymbols)
}
}

Expand All @@ -155,10 +155,11 @@ final case class Disable(index: SemanticdbIndex, config: DisableConfig)
Right(config.allDisabledSymbols) // reset blocked symbols in (...) => (...)
case (t: Name, blockedSymbols) =>
handleName(t, blockedSymbols)
}).result(ctx.tree)
}).result(doc.tree)
}

private def checkSynthetics(ctx: RuleCtx): Seq[Diagnostic] = {
// XXX what goes here?
private def checkSynthetics(implicit doc: SemanticDocument): Seq[Patch] = {
Copy link
Author

Choose a reason for hiding this comment

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

checkSynthetics I don't know what the v1 equivalent is

for {
synthetic <- ctx.index.synthetics
ResolvedName(
Expand All @@ -176,16 +177,10 @@ final case class Disable(index: SemanticdbIndex, config: DisableConfig)
case _ =>
"" -> pos
}
createLintMessage(symbol, disabled, caret, details)
Patch.lint(createLintMessage(symbol, disabled, caret, details)).atomic
}
}

override def check(ctx: RuleCtx): Seq[Diagnostic] = {
checkTree(ctx) ++ checkSynthetics(ctx)
override def fix(implicit doc: SemanticDocument): Patch = {
(checkTree ++ checkSynthetics).asPatch
}
}

class DisableLegacy()
extends LegacySemanticRule(
"Disable",
index => Disable(index, DisableConfig.default))
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@ package scalafix.internal.scaluzzi
import metaconfig._
import metaconfig.annotation.{Description, ExampleValue}
import metaconfig.generic.Surface
import scalafix.v0.Symbol
import scalafix.internal.util.SymbolOps
import scalafix.v1.Symbol
import scalafix.internal.config._

case class DisabledSymbol(
@ExampleValue("scala.sys.process.Process")
@Description(
"A single symbol to ban. Cannot be used together with the regex option.")
symbol: Option[Symbol.Global],
symbol: Option[Symbol],
@Description("Custom message.")
message: Option[String],
@Description("Custom id for error messages.")
Expand All @@ -33,7 +32,7 @@ case class DisabledSymbol(
def matches(symbol: Symbol): Boolean = {
this.symbol match {
case Some(s) =>
SymbolOps.isSameNormalized(symbol, s)
symbol.normalized.equals(s.normalized)
Copy link
Author

Choose a reason for hiding this comment

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

I think this is the same

case None =>
regex match {
case Some(r) => r.matches(symbol.toString)
Expand All @@ -53,10 +52,14 @@ object DisabledSymbol {
} else {
msg
}

implicit lazy val symbolV1Reader: ConfDecoder[Symbol] =
Copy link
Author

Choose a reason for hiding this comment

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

best guess at what Symbol.Global equivalent is

ConfDecoder.stringConfDecoder.map(Symbol.apply)

implicit val reader: ConfDecoder[DisabledSymbol] =
ConfDecoder.instance[DisabledSymbol] {
case c: Conf.Obj =>
(c.getOption[Symbol.Global]("symbol") |@|
(c.getOption[Symbol]("symbol") |@|
c.getOption[String]("message") |@|
c.getOption[String]("id") |@|
c.getOption[FilterMatcher]("regex"))
Expand All @@ -73,7 +76,7 @@ object DisabledSymbol {
ConfError.message("Either symbol or regex must be specified."))
}
case s: Conf.Str =>
symbolGlobalReader
symbolV1Reader
.read(s)
.map(sym => DisabledSymbol(Some(sym), None, None, None))
}
Expand Down