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 Contains Wart for List and Option #566

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ lazy val sbtPlug: Project = Project(
val base = (Compile / sourceManaged).value
val file = base / "wartremover" / "Wart.scala"
val warts = wartClasses.value
val expectCount = 40
val expectCount = 41
assert(
warts.size == expectCount,
s"${warts.size} != ${expectCount}. please update build.sbt when add or remove wart"
Expand Down
34 changes: 34 additions & 0 deletions core/src/main/scala/wartremover/warts/Contains.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.wartremover
package warts

object Contains extends WartTraverser {
private[warts] def message: String = "Don't use List or Option `contains` method because is not typesafe"
Copy link
Contributor

Choose a reason for hiding this comment

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

actually they are type safe, just not very useful.

Copy link
Author

Choose a reason for hiding this comment

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

it is possible to implement something like List(1).contains(true) without any warning about it

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree a warning would be a good thing - my issue is with the wording, because technically it is type safe: you'll never get a ClassCastException at run time, the result is correct, and there are even legitimate use cases for this behaviour. just not very many.

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, can you suggest better wording for the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, i'm not even sure i'd want to disable these functions without some more useful replacement - which if it existed, should probably be suggested in the message

Copy link
Author

Choose a reason for hiding this comment

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

contains in Lists and Options is type unsafe it is possible to implement List(1).contains("1"). Use type-safe implementation for example from cats List.contains_

something like that?

override def apply(u: WartUniverse): u.Traverser = {
import u.universe._

val containsName = TermName("contains").encodedName

new u.Traverser {

override def traverse(tree: Tree): Unit = {

val listType = rootMirror.staticClass("scala.collection.immutable.List").asType.toTypeConstructor
val optionType = rootMirror.staticClass("scala.Option").asType.toTypeConstructor

tree match {
case t if hasWartAnnotation(u)(t) =>
// Ignore trees marked by SuppressWarnings
case Apply(TypeApply(Select(receiver, method), _), _) if
((receiver.tpe.typeConstructor <:< listType) || (receiver.tpe.typeConstructor <:< optionType))
&& (method == containsName)
=>
error(u)(tree.pos, message)
case _ =>
super.traverse(tree)
}

}

}
}
}
45 changes: 45 additions & 0 deletions core/src/test/scala/wartremover/warts/ContainsTest.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.wartremover
package test

import org.scalatest.funsuite.AnyFunSuite
import org.wartremover.warts.Contains

class ContainsTest extends AnyFunSuite with ResultAssertions {

val msg = "Don't use List or Option `contains` method because is not typesafe"

test("List.contains") {
val result = WartTestTraverser(Contains) {
List(1).contains(1)
}

assertError(result)(msg)
}

test("Option.contains") {
val result = WartTestTraverser(Contains) {
Option(1).contains("1")
}

assertError(result)(msg)
}

test("List.flatten.contains") {
def l = List(List(1, 2, 3), List(4, 5, 6, 7))

val result = WartTestTraverser(Contains) {
l.flatten.contains(1)
}

assertError(result)(msg)
}

test("Contains wart obeys SuppressWarnings") {
val result = WartTestTraverser(Contains) {
@SuppressWarnings(Array("org.wartremover.warts.Contains"))
val foo = List(1).contains(1)
}

assertEmpty(result)
}
}
9 changes: 9 additions & 0 deletions docs/_posts/2017-02-11-warts.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ val xs = List(1, true)
x.asInstanceOf[String]
```

### Contains
`contains` in Lists and Options is type unsafe it is possible to implement List(1).contains("1"). Use type-safe implementation for example from cats `List.contains_`
````scala
// Won't compile: List.contains is disabled
List(1).contains(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

but this is the useful case, isn't it? it might make sense to only disable them for un-related types...

Copy link
Author

Choose a reason for hiding this comment

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

that is generally a bad idea to use native contains , my opinion is that always something like cats.contains_ should be used to be safe

// Won't compile: Option.contains is disabled
Option(1).contains(1)
````

### DefaultArguments

Scala allows methods to have default arguments, which make it hard to use methods as functions.
Expand Down