Add Disable linter to disallow usage of a set of symbols #353
Conversation
There was a problem hiding this comment.
Thank you for this contribution @barambani ! This looks great, only a minor comment on name of the rule and configurability.
| import scalafix.lint.LintCategory | ||
| import scalafix.syntax._ | ||
|
|
||
| case class AsInstanceOfDisabled(index: SemanticdbIndex) |
There was a problem hiding this comment.
What do you think about renaming this rewrite to Disable and make the disabled symbols configurable as is done with NoInfer in #355 ? Each rule does a full traversal on the tree so we should avoid a custom rule per disabled symbol.
There was a problem hiding this comment.
Hehe, I remember that @barambani wanted to do it this way, and then I convinced him to make it specific to asInstanceOf and if there's interest we can generalize it afterwards. 👍 I agree that if general, better!
There was a problem hiding this comment.
Fixed, thanks to both for the great feedback. Please, give it a look and let me know your thoughts.
| ) | ||
|
|
||
| override def check(ctx: RuleCtx): Seq[LintMessage] = | ||
| ctx.tree.collect { |
There was a problem hiding this comment.
ctx.index.names.collect { ... } will do the same with the benefit of avoiding the tricky Term.Apply... match. SymbolMatcher.normalized can also be used as a replacement of s.symbol.contains with the benefit of being able to support multiple symbols.
There was a problem hiding this comment.
Changed, thanks for the help.
|
|
||
| case object Disable { | ||
| lazy val disabledSymbol: List[Symbol] = | ||
| Symbol("_root_.scala.Any#asInstanceOf()Ljava/lang/Object;.") :: Nil |
There was a problem hiding this comment.
I would probably use the normalized symbol here, and change the SymbolMatcher accordingly. It's a little nicer to read and more consistent with other usages of Symbol across rules
There was a problem hiding this comment.
On a second thought though, now that you made the rule more general it's kind of strange that asInstanceOf is the default. It feels arbitrary. Maybe this rule could be noop if not configured?
There was a problem hiding this comment.
Makes sense, I removed the default from the rule and added a config entry in the test. Thanks.
There was a problem hiding this comment.
LGTM 👍 Thank you @barambani ! I will follow up with a PR to rename the rule to DisableSymbol following a discussion with @gabro
|
Nice work btw! I suspect this will become a popular rul! |
Ported from Wartremover, completed as part of the Scala Center Dublin spree