-
Notifications
You must be signed in to change notification settings - Fork 113
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" | ||
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) | ||
} | ||
|
||
} | ||
|
||
} | ||
} | ||
} |
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) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 itThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 catsList.contains_
something like that?