OrphanImplicits #635

vovapolu · 2018-02-25T18:38:51Z

Depends on #634. Fixes #526.

olafurpg · 2018-03-06T08:32:29Z

Can you please rebase this PR and cleanup the WIP commits?

olafurpg

Left a couple quick comments to clarify the expected functionality. Will need another round to review the implementation details

olafurpg · 2018-03-12T16:51:34Z

website/src/main/tut/docs/rules/OrphanImplicits.md

+  }
+
+  object Bar {
+    implicit val foo: Foo = ??? // error


Why is this an error? Foo is not of shape F[G]

It's not a violation of

This rule bans definitions of implicits F[G] unless they are on the companion of the F or the G.

there is a move to avoid implicits that are not of shape F[_] in scalaz, so I'd be happy with this as an error!

(but it's not an orphan problem... so this should not error under this rule)

But is this an "orphan implicit error" or something else? I'll leave the final decision to you, just wanted to point out that the tests were not in sync with the documentation description.

This is not an orphan implicit

@fommil what about F[A, B] form? Is it should be ignored or not?

F[A, B] can look like F[_] if one of the types is fixed. If you can test if this implicit is defined on F, A or B that would be good.

Is this example up-to-date?

olafurpg · 2018-03-12T16:54:41Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+    extends Rule("OrphanImplicits") {
+
+  override def description: String =
+    "Linter bans definitions of implicits F[G] unless they are on the companion of the F or the G"


Linter that reports an error on implicit instances of shape F[G] or F that do not belong in the companion objects of F or G

this is definitely about things of shape F[G], not about implicits of F on an F.

In fact I'd like a rule to ban implicits for anything except typeclasses 😄

(ReaderT is a far superior alternative to implicit configuration objects)

The docs had an example of implicit val f: Foo = null so I assumed that was intended behavior too. Drop the "or F" part then

olafurpg · 2018-03-12T16:56:18Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+      }
+    }
+    new ContextTraverser[LintMessage, List[Symbol.Global]](List.empty)({
+      case (Defn.Object(_, name, _), objs) =>


what about implicit instances outside of objects? For example traits/classes/term.block/...

the trait rule be fairly complex to implement. It could be enough to say that it's only allowed on a trait if it's in the same compilation unit as the F or G. To combat implicit ordering, you end up with crazy scaffolding like this https://github.com/scalaz/scalaz/blob/series/7.3.x/core/src/main/scala/scalaz/EitherT.scala#L297-L373

olafurpg · 2018-03-12T16:57:46Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+    new ContextTraverser[LintMessage, List[Symbol.Global]](List.empty)({
+      case (Defn.Object(_, name, _), objs) =>
+        index.symbol(name) match {
+          case Some(s @ Symbol.Global(_, _)) => Right(s :: objs)


Does this report an error?

trait Foo[T] object Foo { object Bar { implicit val x: Foo[Int] = null } }

It should be an error IMO because the implicit is not a member of the Foo companion object.

fommil · 2018-03-16T17:18:32Z

in hindsight, I think this is very much related to #661 ... 661 is about what is allowed to be implicit and this is about where the implicits can be defined.

olafurpg

Left some comments on both documentation and implementation.

olafurpg · 2018-03-19T09:59:14Z

scalafix-core/shared/src/main/scala/scalafix/internal/config/DisableConfig.scala

-import metaconfig.generic.Surface
+
+case class DisabledSymbol(
+    @Description("Symbol to ban.")


A single symbol to ban. Cannot be used together with the regex option.

Can we give examples here?

scala.Predef.any2stringadd

scala.sys.process.Process

olafurpg · 2018-03-19T10:28:54Z

scalafix-core/shared/src/main/scala/scalafix/internal/config/DisableConfig.scala

+    @Description("Custom id for error messages.")
+    id: Option[String],
+    @Description(
+      "Regex to ban instead of one symbol. " +


Regex to ban symbols matching a given include/exclude patterns.
Cannot be used together with the symbol option.
Include and exclude filters can be either a list of regex strings or a single regex string.

olafurpg · 2018-03-19T10:32:07Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+
+  override def description: String =
+    "Linter that reports an error on implicit instances of shape F[G] " +
+      "that do not belong in the companion objects of F or G."


that do not belong to the companion object

olafurpg · 2018-03-19T10:33:47Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+      "that do not belong in the companion objects of F or G."
+
+  private lazy val errorCategory: LintCategory =
+    LintCategory.error("Orphan implicits should be avoided")


I think this description can be "" because you provide a more detailed message below.

olafurpg · 2018-03-19T10:34:26Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+          errorCategory
+            .at(
+              s"""Orphan implicits are not allowed.
+                 |You should put this definition to one of the following objects:


This definition is only allowed in one of the following objects:

olafurpg · 2018-03-19T10:37:22Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+    }
+
+    ctx.tree.collect {
+      case Defn.Object(_, name, templ) =>


case Defn.Object(_, name @ index.Symbol(obj), templ) =>

olafurpg · 2018-03-19T10:40:46Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+        index.symbol(name) match {
+          case Some(obj @ Symbol.Global(_, _)) =>
+            templ.stats.collect {
+              case t @ Defn.Val(mods, _, Some(tpe), _)


This won't catch implicits with inferred types

object Foo { implicit val x = List(1) }

The robust way to implement this is to query the type of a definition is to use the .resultType extension method as is done here

scalafix/scalafix-core/shared/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

Lines 75 to 78 in e46a040

for {

name <- defnName(defn)

symbol <- name.symbol

typ <- symbol.resultType

.resultTypes will soon be refactored to use Denotation.tpe: semanticdb3.Type, but I can take care of that refactoring once that happens.

olafurpg · 2018-03-19T10:41:42Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+        pos: Position): Option[LintMessage] = {
+
+      val tpeSymbols = tpe match {
+        case Type.Apply(t, Seq(arg)) =>


This won't handle cases with multiple type arguments

object Foo { implicit val x: Map[String, Int] = Map.empty }

Yeah, it won't :) As I understand @fommil wants only F[G] form or some complicated detection of fixed type-vars.

in hindsight, #661 is a prerequisite for doing orphan implicits properly... it bans everything except typeclasses. And then with what's left we can seach on all the type parameters to see if it's defined in a good place or not.

olafurpg · 2018-03-19T10:43:14Z

website/src/main/tut/docs/rules/OrphanImplicits.md

+  }
+
+  object Bar {
+    implicit val foo: Foo = ??? // error


Is this example up-to-date?

olafurpg · 2018-03-26T13:59:39Z

in hindsight, #661 is a prerequisite for doing orphan implicits properly... it bans everything except typeclasses. And then with what's left we can seach on all the type parameters to see if it's defined in a good place or not.

Should the two features be under the same rule? If yes, what should it be named and what are the configuration options?

olafurpg · 2018-03-26T14:04:41Z

scalafix-core/shared/src/main/scala/scalafix/internal/config/DisableConfig.scala

+        symbolGlobalReader
+          .read(s)
+          .map(sym => DisabledSymbol(Some(sym), None, None, None))
+      case _ => Configured.NotOk(ConfError.message("Wrong config format"))


.instance allows you to pass a partial function and metaconfig produces a slightly more helpful error message

olafurpg · 2018-03-26T14:09:14Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+        obj: Symbol.Global): Option[LintMessage] = {
+      if (mods.exists(_.is[Mod.Implicit])) {
+        tpe match {
+          case Some(t) => handleImplicit(t, obj, d.pos)


This would report false positives on situations like

trait Writer[T] object Writer { type IntWriter = Writer[Int] implicit val x: IntWriter = ... }

I would not rely on the syntactic structure of the type in the source.

Well, symbol.resultType doesn't work in this case too :)

Ahh, you're right! The new semanticdb Type does handle that case however, you can leave it unchanged for now 😉

olafurpg · 2018-03-26T14:11:53Z

scalafix-tests/input/src/main/scala/test/DisableRegex.scala

+  import scala.collection.mutable   // ok
+
+  @SuppressWarnings(Array("Disable.ListBuffer"))
+  @SuppressWarnings(Array("Disable.<init>"))


I think we should not report duplicate errors on new X if X is disabled. I think it would be better to skip <init> because there is no way to target a constructor without referencing the explicit type.

Not sure what to do if the user wants to disable a specific constructor overload.

In this case I'm disabling regex "scala.collection.mutable.*". I'm not sure there's a good solution to ignore <init> in regexes. I'd say regex initially is kind of hack, everyone who uses it should expect it's not a silver bullet.

a good usecase is to allow creating File, but not doing anything with it. I'm fine with this meaning that overloads are treated equally. I custom syntax in the future might be nice, but it's a second order feature for sure.

olafurpg · 2018-03-26T14:13:12Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/OrphanImplicits.scala

+    }
+
+    ctx.tree.collect {
+      case Defn.Object(_, name @ index.Symbol(sym), templ) =>


case Defn.Object(_, name @ index.Symbol(sym: Symbol.Global), templ) =>

olafurpg · 2018-03-26T14:14:06Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/ContextTraverser.scala

+  * At every tree node, either builds a new Context or returns a new Value to accumulate.
+  * To collect all accumulated values, use result(Tree).
+  */
+class ContextTraverser[Value, Context](initContext: Context)(


Is this still used by OrphanImplicits?

vovapolu · 2018-03-26T21:12:22Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/Disable.scala

-            Left(
-              createLintMessage(s, disabled, t.pos)
-            )
+            SymbolOps.normalize(s) match {


Is it better than the old version? It doesn't affect tests. I added this because of your suggestion to disable normalized symbols.

olafurpg · 2018-04-04T07:58:09Z

Superseded by #669

fommil changed the title ~~OprhanImplicits~~ OrphanImplicits Feb 25, 2018

vovapolu force-pushed the implicit-rules branch 2 times, most recently from 6cbfaaa to 3521b22 Compare March 12, 2018 16:54

olafurpg suggested changes Mar 12, 2018

View reviewed changes

vovapolu force-pushed the implicit-rules branch from 6efe39d to e4b2428 Compare March 17, 2018 19:48

olafurpg suggested changes Mar 19, 2018

View reviewed changes

olafurpg mentioned this pull request Mar 19, 2018

Regex and hierarchy disabling #634

Closed

vovapolu added 16 commits March 26, 2018 13:59

regexes and hierachy handling in Disable

d64262f

regex and hierarchy banning

e166870

tests and fixes, hierarchy doesn't work properly

4d793fe

Rebased on master

22c68e5

Various fixes and FilterMatcher instead of single Regex

7510531

Removed banOverrides

92ae2c6

Formatting

0c3f2d1

Minor docs fixes in DisableConfig

29f0cb3

Minor formatting and scalafix:ok fix in DisableRegex

b7028d8

Orphan implicits

f0b2623

OprhapImplicits now only bans F[G] and not the other implicit forms

f8204ec

Minor pattern-match fix in OrphanImplicits rule

309bd56

Doc fixes and inferred types handling

a539f75

Revert Disable.md

3771bb2

Removed debug log and regex test fix

04a0bc3

Regex test fix

cb0f471

vovapolu force-pushed the implicit-rules branch from ba920b7 to cb0f471 Compare March 26, 2018 11:23

olafurpg suggested changes Mar 26, 2018

View reviewed changes

vovapolu added 2 commits March 27, 2018 00:07

Cleanning up

97c963e

Fixed DisableRegex test

0382d78

vovapolu commented Mar 26, 2018

View reviewed changes

vovapolu mentioned this pull request Apr 1, 2018

Add support to disable symbol via regex #669

Merged

olafurpg closed this Apr 4, 2018

	for {
	name <- defnName(defn)
	symbol <- name.symbol
	typ <- symbol.resultType

OrphanImplicits #635

OrphanImplicits #635

Conversation

vovapolu commented Feb 25, 2018

olafurpg commented Mar 6, 2018

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vovapolu Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fommil commented Mar 16, 2018

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vovapolu Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Apr 4, 2018

vovapolu Mar 16, 2018 •

edited

Loading

vovapolu Mar 26, 2018 •

edited

Loading