Skip to content

Add IndexingOnANonIndexedSeq rule to Linter #1

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.

Sign up for GitHub

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

Closed
JoshRosen wants to merge 1 commit into from
Closed

Add IndexingOnANonIndexedSeq rule to Linter #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions src/main/scala/LinterPlugin.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ final class LinterPlugin(val global: Global) extends Plugin {
val SeqLikeClass: Symbol = rootMirror.getClassByName(newTermName("scala.collection.SeqLike"))
val SeqLikeContains: Symbol = SeqLikeClass.info.member(newTermName("contains"))
val SeqLikeApply: Symbol = SeqLikeClass.info.member(newTermName("apply"))
val IndexedSeqLikeClass: Symbol = rootMirror.getClassByName(newTermName("scala.collection.IndexedSeqLike"))
val MapFactoryClass = rootMirror.getClassByName(newTermName("scala.collection.generic.MapFactory"))
val TraversableFactoryClass: Symbol = rootMirror.getClassByName(newTermName("scala.collection.generic.TraversableFactory"))
val TraversableOnceClass: Symbol = rootMirror.getClassByName(newTermName("scala.collection.TraversableOnce"))
Expand Down Expand Up @@ -1944,6 +1945,12 @@ final class LinterPlugin(val global: Global) extends Plugin {

warn(tree, UseLastNotApply(identOrCol(col)))

case Apply(Select(seq, _apply), List(index))
if methodImplements(tree.symbol, SeqLikeApply)
&& !isLiteral(index)
Copy link
Copy Markdown
Owner Author

@JoshRosen JoshRosen May 20, 2017

Choose a reason for hiding this comment

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

The idea behind ignoring Seq.apply(<literal>) was to prevent this rule from masking the UseHeadNotApply rule for things like x(1).

&& !seq.tpe.baseClasses.exists(_.tpe =:= IndexedSeqLikeClass.tpe) =>
warn(tree, IndexingOnANonIndexedSeq)

case If(cond, thenp, elsep) if {//Microcosm

def warnUseHeadOrLastOptionNotIf(headOrLast: String, col: Tree): Unit = {
Expand Down
3 changes: 3 additions & 0 deletions src/main/scala/Warning.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ final object Warning {
IdenticalIfElseCondition,
IdenticalStatements,
IndexingWithNegativeNumber,
IndexingOnANonIndexedSeq,
InefficientUseOfListSize("", "", ""),
IntDivisionAssignedToFloat,
InvalidParamToRandomNextInt,
Expand Down Expand Up @@ -228,6 +229,8 @@ case object IdenticalStatements extends
Warning("You're doing the exact same thing twice or more.")
case object IndexingWithNegativeNumber extends
Warning("Using a negative index for a collection.")
case object IndexingOnANonIndexedSeq extends
Warning("Indexing on a non-IndexedSeq may cause performance problems.")
case object OptionOfOption extends
Warning("Why would you need an Option of an Option?")
case class UndesirableTypeInference(inferredType: String) extends
Expand Down
56 changes: 41 additions & 15 deletions src/test/scala/LinterPluginTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ final class LinterPluginTest extends MustThrownMatchers with ThrownStandardMatch
should(defs+"""bool match { case true => 0 case false => 1 }""")("""Pattern matching on Boolean is probably better written as an if statement.""")
should(defs+"""for (i <- 10 to 20) { if(i > 20) "" }""")("""This condition will never hold.""")
should(defs+"""for (i <- 1 to 10) { 1/(i-1) }""")("""Possible division by zero.""")
should(defs+"""{ val a = List(1,2,3); for (i <- 1 to 10) { println(a(i)) } }""")("""You will likely use a too large index.""")
should(defs+"""{ val a = IndexedSeq(1,2,3); for (i <- 1 to 10) { println(a(i)) } }""")("""You will likely use a too large index.""")
should(defs+"""for (i <- 10 to 20) { if (i.toString.length == 3) "" }""")("""This condition will never hold.""")
should(defs+"""val s = "hello"+util.Random.nextString(10)+"world"+util.Random.nextString(10)+"!"; if(s contains "world") ""; """)("""This contains always returns the same value: true""")
should(defs+"""val s = "hello"+util.Random.nextString(10)+"world"+util.Random.nextString(10)+"!"; if(s startsWith "hell") ""; """)("""This startsWith always returns the same value: true""")
Expand Down Expand Up @@ -1514,7 +1514,7 @@ final class LinterPluginTest extends MustThrownMatchers with ThrownStandardMatch
def UseLastNotApply(): Unit = {
implicit val msg = ".last instead of "
should("""val l = List(1, 2, 3); l(l.length - 1)""")
noLint("""val seq = Seq("a", "b"); seq(seq.size - 1)""")
noLint("""val seq = IndexedSeq("a", "b"); seq(seq.size - 1)""")
noLint("""val v = Vector("a", "b"); v(v.length - 1)""")
noLint("""val a = Array(4, 2); a(a.size - 1)""")
noLint("""val s = "xyz"; s(s.length - 1)""")
Expand Down Expand Up @@ -2753,68 +2753,94 @@ final class LinterPluginTest extends MustThrownMatchers with ThrownStandardMatch
implicit val msg = "negative index"

should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(-1)
""")
should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.size-a.size-1)
""")
should("""
val a = List(1,2,3,4)
val a = IndexedSeq(1,2,3,4)
val b = -a.size /* -4 */
a(a.size/2 + b + 1) /* 2 - 4 + 1 == -1 */
""")
should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.size-a.length-1)
""")
should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
val b = "a"
a(b.size-b.length-1)
""")

noLint("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.size-a.size)
""")
noLint("""
val a = List(1,2,3,4)
val a = IndexedSeq(1,2,3,4)
val b = -a.size + 1 /* -3 */
a(a.size/2 + b + 1) /* 2 - 3 + 1 == 0 */
""")
noLint("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.size-a.length)
""")
noLint("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
val b = "a"
a(b.size-b.length)
""")
}

@Test
def collections__indexOnNonIndexedSeq(): Unit = {
implicit val msg = "non-IndexedSeq"
should("""
val x = 0
val a = Seq(1,2,3)
a(x)
""")
// If it's an IndexedSeq subtype then we should not throw a warning:
noLint("""
val x = 0
val a = Array(1,2,3)
a(x)
""")
noLint("""
val x = 0
val a = IndexedSeq(1,2,3)
a(x)
""")
// Ignore constant indexes for now, since those are less likely to lead to quadratic runtime.
noLint("""
val a = Seq(1,2,3)
a(2)
""")
}

@Test
def collections__tooLargeIndex(): Unit = {
implicit val msg = "too large index"

should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.size)
""")
should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
a(a.length)
""")
should("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
val b = 5
a(b-1)
""")

noLint("""
val a = List(1,2,3)
val a = IndexedSeq(1,2,3)
val b = 5
a(b-3)
""")
Expand Down
3 changes: 2 additions & 1 deletion src/test/scala/WarningTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class WarningTest extends MustThrownMatchers {
@Test
def allIncludesAll(): Unit = {
//TODO: could do it with reflection
val knownCount = 123
val knownCount = 124
val count: Int = Warning.All.distinct.map {
case AssigningOptionToNull => 1
case AvoidOptionCollectionSize => 1
Expand All @@ -48,6 +48,7 @@ class WarningTest extends MustThrownMatchers {
case IdenticalIfElseCondition => 1
case IdenticalStatements => 1
case IndexingWithNegativeNumber => 1
case IndexingOnANonIndexedSeq => 1
case InefficientUseOfListSize(_, _, _) => 1
case IntDivisionAssignedToFloat => 1
case InvalidParamToRandomNextInt => 1
Expand Down