From 9bf26604dfd66e42b0288b52ba9e96e5f41f716c Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:13:14 +0200 Subject: [PATCH 01/12] Added isList, isSet... and isVarArgs methods to Util --- rules/src/main/scala/fix/Util.scala | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/rules/src/main/scala/fix/Util.scala b/rules/src/main/scala/fix/Util.scala index 34f1fa8..2436866 100644 --- a/rules/src/main/scala/fix/Util.scala +++ b/rules/src/main/scala/fix/Util.scala @@ -29,6 +29,15 @@ object Util { } } + def isVarArgs(term: Stat)(implicit doc: SemanticDocument): Boolean = { + term.symbol.info.exists(_.signature match { + case ValueSignature(RepeatedType(_)) => true + case _ => false + } + ) + } + + /** Compare the type of a term with the passed symbols * @param term * The term to compare @@ -220,4 +229,11 @@ object Util { } } + def isSet(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/collection/immutable/Set", "scala/collection/mutable/Set", "scala/Predef.Set") + def isMap(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/collection/immutable/Map", "scala/collection/mutable/Map", "scala/Predef.Map") + def isSeq(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/collection/immutable/Seq", "scala/collection/mutable/Seq", "scala/package.Seq") + def isList(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/collection/immutable/List", "scala/package.List") + def isVector(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/collection/immutable/Vector", "scala/package.Vector") + def isArray(term: Term)(implicit doc: SemanticDocument): Boolean = Util.matchType(term, "scala/Array") + } From 483c79987a4c050877320a7defdaddf696d19e74 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:13:39 +0200 Subject: [PATCH 02/12] Added ExistsSimplifiableToContains --- .../fix/ExistsSimplifiableToContains.scala | 43 ++++++++++++ .../META-INF/services/scalafix.v1.Rule | 3 +- .../fix/ExistsSimplifiableToContains.scala | 68 +++++++++++++++++++ 3 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 input/src/main/scala/fix/ExistsSimplifiableToContains.scala create mode 100644 rules/src/main/scala/fix/ExistsSimplifiableToContains.scala diff --git a/input/src/main/scala/fix/ExistsSimplifiableToContains.scala b/input/src/main/scala/fix/ExistsSimplifiableToContains.scala new file mode 100644 index 0000000..b082991 --- /dev/null +++ b/input/src/main/scala/fix/ExistsSimplifiableToContains.scala @@ -0,0 +1,43 @@ +/* +rule = ExistsSimplifiableToContains + */ +package fix + +object ExistsSimplifiableToContains { + val exists1 = List(1, 2, 3).exists(x => x == 2) // assert: ExistsSimplifiableToContains + val exists12 = List(1, 2, 3).exists(x => 2 == x) // assert: ExistsSimplifiableToContains + + val list = List("sam", "spade") + val exists2 = list.exists(_ == "spoof") // assert: ExistsSimplifiableToContains + val exists22 = list.exists("spoof" == _) // assert: ExistsSimplifiableToContains + + val exists3 = (1 to 3).exists(_ == 2) // assert: ExistsSimplifiableToContains + val exists32 = (1 to 3).exists(2 == _) // assert: ExistsSimplifiableToContains + val exists33 = (1 until 3).exists(_ == 2) // assert: ExistsSimplifiableToContains + val exists34 = (1 until 3).exists(2 == _) // assert: ExistsSimplifiableToContains + + def isItA(strings: String*): Boolean = { + strings.exists { element => // assert: ExistsSimplifiableToContains + element.toLowerCase == "a" + } + } + + val l: Iterable[String] = List[String]("a", "b", "c") + print(l.exists(_ == "a")) // scalafix: ok; + + def atLeastOneIsAllLowercase(strings: String*): Boolean = { + strings.exists { element => // scalafix: ok; + element == element.toLowerCase + } + } + + def containsNoA(strings: String*): Boolean = { + strings.exists { element => // scalafix: ok; + element.replaceAll("a", "").size == element.size + } + } + + val map = Map("answer" -> "42") + val isCorrect = map.exists(_._2 == "42") // scalafix: ok; + +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index b155300..942a5be 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -67,4 +67,5 @@ fix.UnsafeTraversableMethods fix.UnusedMethodParameter fix.VarCouldBeVal fix.VariableShadowing -fix.WhileTrue \ No newline at end of file +fix.WhileTrue +fix.ExistsSimplifiableToContains \ No newline at end of file diff --git a/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala b/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala new file mode 100644 index 0000000..9e4aa82 --- /dev/null +++ b/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala @@ -0,0 +1,68 @@ +/* +rule = ExistsSimplifiableToContains + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class ExistsSimplifiableToContains extends SemanticRule("ExistsSimplifiableToContains") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks if `exists()` can be simplified to `contains()`", + pos, + "`exists(x => x == y)` can be replaced with `contains(y)`.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + + // Ranges are respectively for until and to, we also handle varargs + def isContainsTraversable(term: Term) = Util.isArray(term) || Util.isSeq(term) || Util.isList(term) || Util.isSet(term) || Util.isMap(term) || Util.isVector(term) || Util.matchType(term, "scala/collection/immutable/Range", "scala/collection/immutable/Range.Inclusive") || Util.isVarArgs(term) + def isUsedOnce(other: Tree, vari: String) = !other.collect { case Term.Name(n) => n == vari }.contains(true) + + def ruleNotAnonymous(var1: String, lhs: Term, rhs: Term.ArgClause, t: Term): Patch = { + (lhs, rhs) match { + case (Term.Name(var2), other) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) + case (other, Term.ArgClause(List(Term.Name(var2)), _)) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) + // Ignore calls to _.1 or ._2 for maps + case (Term.Select(Term.Name(_), Term.Name("_1" | "_2")), _) => Patch.empty + case (Term.Select(Term.Name(var2), _), other) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) + case (other, Term.ArgClause(List(Term.Select(Term.Name(var2), _)), _)) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) + case _ => Patch.empty + } + } + + def ruleAnonymous(lhs: Term, rhs: Term.ArgClause, t: Term): Patch = { + (lhs, rhs) match { + // Same as above + case (Term.Select(Term.Placeholder(), Term.Name("_1" | "_2")), _) => Patch.empty + case (Term.Placeholder() | Term.Select(Term.Placeholder(), _), _) => Patch.lint(diag(t.pos)) + case (_, Term.ArgClause(List(Term.Placeholder() | Term.Select(Term.Placeholder(), _)), _)) => Patch.lint(diag(t.pos)) + case _ => Patch.empty + } + } + + doc.tree.collect { + // Corresponds to l.exists(x => x == y), l.exists(x => y == x), l.exists(x => x != y), l.exists(x => y != x) + case t @ Term.Apply.After_4_6_0(Term.Select(qual, Term.Name("exists")), Term.ArgClause(List(fun), _)) if isContainsTraversable(qual) => + // Extract block in case of .exists {} instead of .exists() + val extractedFun = fun match { + case Term.Block(List(stats)) => stats + case _ => fun + } + // Avoids code repetition + extractedFun match { + case Term.Function.After_4_6_0(Term.ParamClause(List(Term.Param(_, Term.Name(var1), _, _)), _), Term.ApplyInfix.After_4_6_0(lhs, Term.Name("==" | "!="), _, rhs)) => ruleNotAnonymous(var1, lhs, rhs, t) + case Term.AnonymousFunction(Term.ApplyInfix.After_4_6_0(lhs, Term.Name("==" | "!="), _, rhs)) => ruleAnonymous(lhs, rhs, t) + case _ => Patch.empty + } + // We check that we are doing a comparison of the type x == y or y == x with x the variable, i.e. at least one of (lhs, rhs) needs to be the parameter + + }.asPatch + } + +} From 8ac9299efe75584528417dcbe515da1f89d15b5b Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:13:58 +0200 Subject: [PATCH 03/12] Refactored MapGetAndGetOrElse and UnnecessaryConversion --- rules/src/main/scala/fix/MapGetAndGetOrElse.scala | 2 +- .../src/main/scala/fix/UnnecessaryConversion.scala | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/rules/src/main/scala/fix/MapGetAndGetOrElse.scala b/rules/src/main/scala/fix/MapGetAndGetOrElse.scala index 4a3bda3..0b69936 100644 --- a/rules/src/main/scala/fix/MapGetAndGetOrElse.scala +++ b/rules/src/main/scala/fix/MapGetAndGetOrElse.scala @@ -23,7 +23,7 @@ class MapGetAndGetOrElse extends SemanticRule("MapGetAndGetOrElse") { // Corresponds to map.get(key).getOrElse(value) case Term.Apply.After_4_6_0(Term.Select(Term.Apply.After_4_6_0(Term.Select(qual, Term.Name("get")), _), Term.Name("getOrElse")), _) // Maps can either be immutable, mutable or simply a Map(k,v) from Predef that we use as Map(k,v).get(key).getOrElse(value) - if Util.matchType(qual, "scala/collection/immutable/Map", "scala/collection/mutable/Map", "scala/Predef.Map") => Patch.lint(diag(qual.pos)) + if Util.isMap(qual) => Patch.lint(diag(qual.pos)) case _ => Patch.empty }.asPatch } diff --git a/rules/src/main/scala/fix/UnnecessaryConversion.scala b/rules/src/main/scala/fix/UnnecessaryConversion.scala index c652cfe..f0bfff9 100644 --- a/rules/src/main/scala/fix/UnnecessaryConversion.scala +++ b/rules/src/main/scala/fix/UnnecessaryConversion.scala @@ -45,14 +45,14 @@ class UnnecessaryConversion extends SemanticRule("UnnecessaryConversion") { case t @ Term.Select(name @ Term.Name(_), Term.Name("toDouble")) if Util.matchType(name, "scala/Double") => Patch.lint(diag(t.pos)) // Predef.Set corresponds to Set(_) in Scala, same for Map - case t @ Term.Select(qual, Term.Name("toSet")) if Util.matchType(qual, "scala/collection/immutable/Set", "scala/collection/mutable/Set", "scala/Predef.Set") => Patch.lint(diag(t.pos)) - case t @ Term.Select(qual, Term.Name("toMap")) if Util.matchType(qual, "scala/collection/immutable/Map", "scala/collection/mutable/Map", "scala/Predef.Map") => Patch.lint(diag(t.pos)) + case t @ Term.Select(qual, Term.Name("toSet")) if Util.isSet(qual) => Patch.lint(diag(t.pos)) + case t @ Term.Select(qual, Term.Name("toMap")) if Util.isMap(qual) => Patch.lint(diag(t.pos)) // package.List corresponds to List(_) in Scala, same for Seq and Vector - case t @ Term.Select(qual, Term.Name("toList")) if Util.matchType(qual, "scala/collection/immutable/List", "scala/package.List") => Patch.lint(diag(t.pos)) - case t @ Term.Select(qual, Term.Name("toSeq")) if Util.matchType(qual, "scala/collection/immutable/Seq", "scala/collection/mutable/Seq", "scala/package.Seq") => Patch.lint(diag(t.pos)) - case t @ Term.Select(qual, Term.Name("toVector")) if Util.matchType(qual, "scala/collection/immutable/Vector", "scala/package.Vector") => Patch.lint(diag(t.pos)) - case t @ Term.Select(qual, Term.Name("toArray")) if Util.matchType(qual, "scala/Array", "scala/Array.empty") => Patch.lint(diag(t.pos)) - case _ => Patch.empty + case t @ Term.Select(qual, Term.Name("toList")) if Util.isList(qual) => Patch.lint(diag(t.pos)) + case t @ Term.Select(qual, Term.Name("toSeq")) if Util.isSeq(qual) => Patch.lint(diag(t.pos)) + case t @ Term.Select(qual, Term.Name("toVector")) if Util.isVector(qual) => Patch.lint(diag(t.pos)) + case t @ Term.Select(qual, Term.Name("toArray")) if Util.isArray(qual) => Patch.lint(diag(t.pos)) + case _ => Patch.empty }.asPatch } } From 7a67758c6ad93bea635f7a5d6ee2f64375b61fe1 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:14:02 +0200 Subject: [PATCH 04/12] Scalafmt Util --- rules/src/main/scala/fix/Util.scala | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/rules/src/main/scala/fix/Util.scala b/rules/src/main/scala/fix/Util.scala index 2436866..ffa5b0e 100644 --- a/rules/src/main/scala/fix/Util.scala +++ b/rules/src/main/scala/fix/Util.scala @@ -32,12 +32,10 @@ object Util { def isVarArgs(term: Stat)(implicit doc: SemanticDocument): Boolean = { term.symbol.info.exists(_.signature match { case ValueSignature(RepeatedType(_)) => true - case _ => false - } - ) + case _ => false + }) } - /** Compare the type of a term with the passed symbols * @param term * The term to compare From 336ee416af149c1e6ed93f073398c0786bbd9d70 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:17:17 +0200 Subject: [PATCH 05/12] Improved ExistsSimplifiableToContains documentation --- .../scala/fix/ExistsSimplifiableToContains.scala | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala b/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala index 9e4aa82..67940b5 100644 --- a/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala +++ b/rules/src/main/scala/fix/ExistsSimplifiableToContains.scala @@ -20,15 +20,18 @@ class ExistsSimplifiableToContains extends SemanticRule("ExistsSimplifiableToCon override def fix(implicit doc: SemanticDocument): Patch = { - // Ranges are respectively for until and to, we also handle varargs + // Array, seq, list, set, map, vector, range, varargs implement the contains method + // Ranges are respectively for until and to def isContainsTraversable(term: Term) = Util.isArray(term) || Util.isSeq(term) || Util.isList(term) || Util.isSet(term) || Util.isMap(term) || Util.isVector(term) || Util.matchType(term, "scala/collection/immutable/Range", "scala/collection/immutable/Range.Inclusive") || Util.isVarArgs(term) def isUsedOnce(other: Tree, vari: String) = !other.collect { case Term.Name(n) => n == vari }.contains(true) + // Handle normal function (with variable) def ruleNotAnonymous(var1: String, lhs: Term, rhs: Term.ArgClause, t: Term): Patch = { + // We check if one of the two sides is the variable, and if it is used only once i.e. not in the other side, as otherwise contains is not sufficient (lhs, rhs) match { case (Term.Name(var2), other) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) case (other, Term.ArgClause(List(Term.Name(var2)), _)) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) - // Ignore calls to _.1 or ._2 for maps + // Ignore calls to _.1 or ._2 for maps as contains would not be sufficient case (Term.Select(Term.Name(_), Term.Name("_1" | "_2")), _) => Patch.empty case (Term.Select(Term.Name(var2), _), other) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) case (other, Term.ArgClause(List(Term.Select(Term.Name(var2), _)), _)) if var1 == var2 && isUsedOnce(other, var2) => Patch.lint(diag(t.pos)) @@ -36,9 +39,10 @@ class ExistsSimplifiableToContains extends SemanticRule("ExistsSimplifiableToCon } } + // Handle anonymous function (with placeholders) def ruleAnonymous(lhs: Term, rhs: Term.ArgClause, t: Term): Patch = { (lhs, rhs) match { - // Same as above + // Here the placeholder is necessarily used one, no need for the check, we simply check for _.1 or _.2 for maps case (Term.Select(Term.Placeholder(), Term.Name("_1" | "_2")), _) => Patch.empty case (Term.Placeholder() | Term.Select(Term.Placeholder(), _), _) => Patch.lint(diag(t.pos)) case (_, Term.ArgClause(List(Term.Placeholder() | Term.Select(Term.Placeholder(), _)), _)) => Patch.lint(diag(t.pos)) @@ -60,8 +64,6 @@ class ExistsSimplifiableToContains extends SemanticRule("ExistsSimplifiableToCon case Term.AnonymousFunction(Term.ApplyInfix.After_4_6_0(lhs, Term.Name("==" | "!="), _, rhs)) => ruleAnonymous(lhs, rhs, t) case _ => Patch.empty } - // We check that we are doing a comparison of the type x == y or y == x with x the variable, i.e. at least one of (lhs, rhs) needs to be the parameter - }.asPatch } From b4e26bcabe568977fa2620a82a836fdc7357d4bb Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 15:58:47 +0200 Subject: [PATCH 06/12] Added FilterDotHead, FilterDotHeadOption, FilterDotIsEmpty and FilterOptionAndGet --- input/src/main/scala/fix/FilterDotHead.scala | 12 ++++++ .../main/scala/fix/FilterDotHeadOption.scala | 12 ++++++ .../src/main/scala/fix/FilterDotIsEmpty.scala | 12 ++++++ .../main/scala/fix/FilterOptionAndGet.scala | 13 ++++++ .../META-INF/services/scalafix.v1.Rule | 6 ++- rules/src/main/scala/fix/FilterDotHead.scala | 27 +++++++++++++ .../main/scala/fix/FilterDotHeadOption.scala | 27 +++++++++++++ .../src/main/scala/fix/FilterDotIsEmpty.scala | 27 +++++++++++++ .../main/scala/fix/FilterOptionAndGet.scala | 40 +++++++++++++++++++ 9 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 input/src/main/scala/fix/FilterDotHead.scala create mode 100644 input/src/main/scala/fix/FilterDotHeadOption.scala create mode 100644 input/src/main/scala/fix/FilterDotIsEmpty.scala create mode 100644 input/src/main/scala/fix/FilterOptionAndGet.scala create mode 100644 rules/src/main/scala/fix/FilterDotHead.scala create mode 100644 rules/src/main/scala/fix/FilterDotHeadOption.scala create mode 100644 rules/src/main/scala/fix/FilterDotIsEmpty.scala create mode 100644 rules/src/main/scala/fix/FilterOptionAndGet.scala diff --git a/input/src/main/scala/fix/FilterDotHead.scala b/input/src/main/scala/fix/FilterDotHead.scala new file mode 100644 index 0000000..74d5eee --- /dev/null +++ b/input/src/main/scala/fix/FilterDotHead.scala @@ -0,0 +1,12 @@ +/* +rule = FilterDotHead + */ +package fix + +object FilterDotHead { + def test(): Unit = { + List(1, 2, 3).filter(_ < 0).head // assert: FilterDotHead + List(1, 2, 3).map(e => e).head // scalafix: ok; + } + +} diff --git a/input/src/main/scala/fix/FilterDotHeadOption.scala b/input/src/main/scala/fix/FilterDotHeadOption.scala new file mode 100644 index 0000000..702f556 --- /dev/null +++ b/input/src/main/scala/fix/FilterDotHeadOption.scala @@ -0,0 +1,12 @@ +/* +rule = FilterDotHeadOption + */ +package fix + +object FilterDotHeadOption { + def test(): Unit = { + List(1, 2, 3).filter(_ < 0).headOption // assert: FilterDotHeadOption + List(1, 2, 3).map(e => e).headOption // scalafix: ok; + } + +} diff --git a/input/src/main/scala/fix/FilterDotIsEmpty.scala b/input/src/main/scala/fix/FilterDotIsEmpty.scala new file mode 100644 index 0000000..fd4e367 --- /dev/null +++ b/input/src/main/scala/fix/FilterDotIsEmpty.scala @@ -0,0 +1,12 @@ +/* +rule = FilterDotIsEmpty + */ +package fix + +object FilterDotIsEmpty { + def test(): Unit = { + List(1, 2, 3).filter(_ < 0).isEmpty // assert: FilterDotIsEmpty + List(1, 2, 3).map(e => e).isEmpty // scalafix: ok; + } + +} diff --git a/input/src/main/scala/fix/FilterOptionAndGet.scala b/input/src/main/scala/fix/FilterOptionAndGet.scala new file mode 100644 index 0000000..8f917c8 --- /dev/null +++ b/input/src/main/scala/fix/FilterOptionAndGet.scala @@ -0,0 +1,13 @@ +/* +rule = FilterOptionAndGet + */ +package fix + +object FilterOptionAndGet { + def test(): Unit = { + val a = List(None, Some("sam")).filter(_.isDefined).map(_.get) // assert: FilterOptionAndGet + val b = List(None, Some("sam")).filter(_.isDefined).map(_.getOrElse("default")) // scalafix: ok; + + } + +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index 942a5be..8ce4c66 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -68,4 +68,8 @@ fix.UnusedMethodParameter fix.VarCouldBeVal fix.VariableShadowing fix.WhileTrue -fix.ExistsSimplifiableToContains \ No newline at end of file +fix.ExistsSimplifiableToContains +fix.FilterDotHead +fix.FilterDotHeadOption +fix.FilterDotIsEmpty +fix.FilterOptionAndGet \ No newline at end of file diff --git a/rules/src/main/scala/fix/FilterDotHead.scala b/rules/src/main/scala/fix/FilterDotHead.scala new file mode 100644 index 0000000..b78c739 --- /dev/null +++ b/rules/src/main/scala/fix/FilterDotHead.scala @@ -0,0 +1,27 @@ +/* +rule = FilterDotHead + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FilterDotHead extends SemanticRule("FilterDotHead") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for use of filter().head.", + pos, + "`filter().head` can throw an exception if the collection is empty - it can be replaced with `find() match {...}`.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("filter")), _), Term.Name("head")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} diff --git a/rules/src/main/scala/fix/FilterDotHeadOption.scala b/rules/src/main/scala/fix/FilterDotHeadOption.scala new file mode 100644 index 0000000..6625577 --- /dev/null +++ b/rules/src/main/scala/fix/FilterDotHeadOption.scala @@ -0,0 +1,27 @@ +/* +rule = FilterDotHeadOption + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FilterDotHeadOption extends SemanticRule("FilterDotHeadOption") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for use of filter().headOption.", + pos, + "`filter()` scans the entire collection, which is unnecessary if you only want to get the first element that satisfies the predicate - `filter().headOption` can be replaced with `find()` to potentially avoid scanning the entire collection.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("filter")), _), Term.Name("headOption")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} diff --git a/rules/src/main/scala/fix/FilterDotIsEmpty.scala b/rules/src/main/scala/fix/FilterDotIsEmpty.scala new file mode 100644 index 0000000..8454874 --- /dev/null +++ b/rules/src/main/scala/fix/FilterDotIsEmpty.scala @@ -0,0 +1,27 @@ +/* +rule = FilterDotIsEmpty + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FilterDotIsEmpty extends SemanticRule("FilterDotIsEmpty") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for use of filter().isEmpty.", + pos, + "`filter()` scans the entire collection, which can potentially be avoided if the element exists in the collection - `filter().isEmpty` can be replaced with `!exists()`.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("filter")), _), Term.Name("isEmpty")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} diff --git a/rules/src/main/scala/fix/FilterOptionAndGet.scala b/rules/src/main/scala/fix/FilterOptionAndGet.scala new file mode 100644 index 0000000..f8c837f --- /dev/null +++ b/rules/src/main/scala/fix/FilterOptionAndGet.scala @@ -0,0 +1,40 @@ +/* +rule = FilterOptionAndGet + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FilterOptionAndGet extends SemanticRule("FilterOptionAndGet") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks whether the expression can be rewritten using flatten.", + pos, + "`filter(_.isDefined).map(_.get)` can be replaced with `flatten`.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + + def extractFun(term: Term): Stat = term match { + case Term.Block(List(fun)) => fun + case t => t + } + + def isFunction(term: Stat, functionName: String): Boolean = term match { + case Term.Function.After_4_6_0(Term.ParamClause(List(Term.Param(_, Term.Name(var1), _, _)), _), Term.Select(Term.Name(var2), Term.Name(funName))) if funName == functionName && var1 == var2 => true + case Term.AnonymousFunction(Term.Select(_, Term.Name(funName))) if funName == functionName => true + case _ => false + } + + doc.tree.collect { + case t @ Term.Apply.After_4_6_0(Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("filter")), Term.ArgClause(List(isDefinedFun), _)), Term.Name("map")), Term.ArgClause(List(mapFun), _)) + if isFunction(extractFun(isDefinedFun), "isDefined") && isFunction(extractFun(mapFun), "get") => Patch.lint(diag(t.pos)) + }.asPatch + } + +} From ef1e9ce65b5a951d5f6f8b790ca11386ebc7ee50 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 16:18:18 +0200 Subject: [PATCH 07/12] Implemented FinalModifierOnCaseClass --- .../scala/fix/FinalModifierOnCaseClass.scala | 17 ++++++++++++ .../META-INF/services/scalafix.v1.Rule | 3 ++- .../scala/fix/FinalModifierOnCaseClass.scala | 27 +++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 input/src/main/scala/fix/FinalModifierOnCaseClass.scala create mode 100644 rules/src/main/scala/fix/FinalModifierOnCaseClass.scala diff --git a/input/src/main/scala/fix/FinalModifierOnCaseClass.scala b/input/src/main/scala/fix/FinalModifierOnCaseClass.scala new file mode 100644 index 0000000..eeb0854 --- /dev/null +++ b/input/src/main/scala/fix/FinalModifierOnCaseClass.scala @@ -0,0 +1,17 @@ +/* +rule = FinalModifierOnCaseClass + */ +package fix + +object FinalModifierOnCaseClass { + case class Person(name: String) // assert: FinalModifierOnCaseClass + abstract case class Person2(name: String) // scalafix: ok; + final case class Person3(name: String) // scalafix: ok; + + // Comes from an issue on Scapegoat GitHub + sealed abstract case class Nat(toInt: Int) // scalafix: ok; + object Nat { + def fromInt(n: Int): Option[Nat] = + if (n >= 0) Some(new Nat(n) {}) else None + } +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index 8ce4c66..b9c5597 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -72,4 +72,5 @@ fix.ExistsSimplifiableToContains fix.FilterDotHead fix.FilterDotHeadOption fix.FilterDotIsEmpty -fix.FilterOptionAndGet \ No newline at end of file +fix.FilterOptionAndGet +fix.FinalModifierOnCaseClass \ No newline at end of file diff --git a/rules/src/main/scala/fix/FinalModifierOnCaseClass.scala b/rules/src/main/scala/fix/FinalModifierOnCaseClass.scala new file mode 100644 index 0000000..5e7d664 --- /dev/null +++ b/rules/src/main/scala/fix/FinalModifierOnCaseClass.scala @@ -0,0 +1,27 @@ +/* +rule = FinalModifierOnCaseClass + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FinalModifierOnCaseClass extends SemanticRule("FinalModifierOnCaseClass") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for case classes without a final modifier.", + pos, + "Using case classes without final modifier can lead to surprising breakage.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Defn.Class.After_4_6_0(mods, _, _, _, _) if mods.exists(_.is[Mod.Case]) && !mods.exists(_.is[Mod.Abstract]) && !mods.exists(_.is[Mod.Final]) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} From fa3625a53936185478648dc91c6a62baa00312cc Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 16:33:04 +0200 Subject: [PATCH 08/12] Added FindAndNotEqualsNoneReplaceWithExists and FindDotIsDefined --- ...indAndNotEqualsNoneReplaceWithExists.scala | 13 +++++++++ .../src/main/scala/fix/FindDotIsDefined.scala | 12 ++++++++ .../META-INF/services/scalafix.v1.Rule | 4 ++- ...indAndNotEqualsNoneReplaceWithExists.scala | 28 +++++++++++++++++++ .../src/main/scala/fix/FindDotIsDefined.scala | 27 ++++++++++++++++++ 5 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 input/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala create mode 100644 input/src/main/scala/fix/FindDotIsDefined.scala create mode 100644 rules/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala create mode 100644 rules/src/main/scala/fix/FindDotIsDefined.scala diff --git a/input/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala b/input/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala new file mode 100644 index 0000000..bc58bc2 --- /dev/null +++ b/input/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala @@ -0,0 +1,13 @@ +/* +rule = FindAndNotEqualsNoneReplaceWithExists + */ +package fix + +object FindAndNotEqualsNoneReplaceWithExists { + def test(): Unit = { + List(1, 2, 3).find(_ > 0) != None // assert: FindAndNotEqualsNoneReplaceWithExists + List(1, 2, 3).find(_ > 0) == None // assert: FindAndNotEqualsNoneReplaceWithExists + List(1, 2, 3).find(_ > 0) // scalafix: ok; + } + +} diff --git a/input/src/main/scala/fix/FindDotIsDefined.scala b/input/src/main/scala/fix/FindDotIsDefined.scala new file mode 100644 index 0000000..1abd901 --- /dev/null +++ b/input/src/main/scala/fix/FindDotIsDefined.scala @@ -0,0 +1,12 @@ +/* +rule = FindDotIsDefined + */ +package fix + +object FindDotIsDefined { + def test(): Unit = { + val a = List(1, 2, 3).find(_ > 4).isDefined // assert: FindDotIsDefined + val b = List(1, 2, 3).find(_ > 4) // scalafix: ok; + } + +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index b9c5597..8706647 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -73,4 +73,6 @@ fix.FilterDotHead fix.FilterDotHeadOption fix.FilterDotIsEmpty fix.FilterOptionAndGet -fix.FinalModifierOnCaseClass \ No newline at end of file +fix.FinalModifierOnCaseClass +fix.FindAndNotEqualsNoneReplaceWithExists +fix.FindDotIsDefined \ No newline at end of file diff --git a/rules/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala b/rules/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala new file mode 100644 index 0000000..2643dfc --- /dev/null +++ b/rules/src/main/scala/fix/FindAndNotEqualsNoneReplaceWithExists.scala @@ -0,0 +1,28 @@ +/* +rule = FindAndNotEqualsNoneReplaceWithExists + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +// Note: differs from Scapegoat rules since we also flag .find(...) == None which can simply be replaced by !.exists(...) which is more concise +class FindAndNotEqualsNoneReplaceWithExists extends SemanticRule("FindAndNotEqualsNoneReplaceWithExists") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks whether `find()` can be replaced with exists().", + pos, + "`find() != None` (resp find == None) can be replaced with `exists()` (resp !exists()), which is more concise.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.ApplyInfix.After_4_6_0(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("find")), _), Term.Name("==" | "!="), _, Term.ArgClause(List(Term.Name("None")), _)) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} diff --git a/rules/src/main/scala/fix/FindDotIsDefined.scala b/rules/src/main/scala/fix/FindDotIsDefined.scala new file mode 100644 index 0000000..cb264b3 --- /dev/null +++ b/rules/src/main/scala/fix/FindDotIsDefined.scala @@ -0,0 +1,27 @@ +/* +rule = FindDotIsDefined + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FindDotIsDefined extends SemanticRule("FindDotIsDefined") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks whether `find()` can be replaced with `exists()`.", + pos, + "`find().isDefined` can be replaced with `exists()`, which is more concise.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("find")), _), Term.Name("isDefined")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} From 67e669d91aac4c693b9360705067a09385386b31 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Mon, 16 Sep 2024 17:01:38 +0200 Subject: [PATCH 09/12] Added InvalidRegexTest, IsInstanceOf and added suppress warnings tests in AsInstanceOf --- input/src/main/scala/fix/AsInstanceOf.scala | 18 +++++++- .../src/main/scala/fix/InvalidRegexTest.scala | 9 ++++ input/src/main/scala/fix/IsInstanceOf.scala | 44 +++++++++++++++++++ .../META-INF/services/scalafix.v1.Rule | 4 +- .../src/main/scala/fix/InvalidRegexTest.scala | 39 ++++++++++++++++ rules/src/main/scala/fix/IsInstanceOf.scala | 28 ++++++++++++ 6 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 input/src/main/scala/fix/InvalidRegexTest.scala create mode 100644 input/src/main/scala/fix/IsInstanceOf.scala create mode 100644 rules/src/main/scala/fix/InvalidRegexTest.scala create mode 100644 rules/src/main/scala/fix/IsInstanceOf.scala diff --git a/input/src/main/scala/fix/AsInstanceOf.scala b/input/src/main/scala/fix/AsInstanceOf.scala index ba6eee3..3f103d6 100644 --- a/input/src/main/scala/fix/AsInstanceOf.scala +++ b/input/src/main/scala/fix/AsInstanceOf.scala @@ -4,8 +4,7 @@ rule = AsInstanceOf package fix object AsInstanceOf { - // In the original Scapegoat test code, there are tests with SuppressWarning annotations. - // We chose to not include these as the same could be done for every rule. If a user wants to suppress a rule, they can simply remove it from the configuration. + def test(): Unit = { val s: Any = "sammy" println(s.asInstanceOf[String]) // assert: AsInstanceOf @@ -18,6 +17,21 @@ object AsInstanceOf { case _ => println("no match :(") // scalafix: ok; } + @SuppressWarnings(Array("all")) + def hello: Unit = { + val s: Any = "sammy" + println(s.asInstanceOf[String]) // scalafix: ok; + } + + @SuppressWarnings(Array("AsInstanceOf")) + def hello2: Unit = { + val s: Any = "sammy" + println(s.asInstanceOf[String]) // scalafix: ok; + } + + @SuppressWarnings(Array("AsInstanceOf")) + val mf = manifest[Class[_]] // scalafix: ok; + sealed trait MyGADT[T] final case class VariantInt(value: Int) extends MyGADT[Int] final case class VariantString(value: String) extends MyGADT[String] diff --git a/input/src/main/scala/fix/InvalidRegexTest.scala b/input/src/main/scala/fix/InvalidRegexTest.scala new file mode 100644 index 0000000..a68d736 --- /dev/null +++ b/input/src/main/scala/fix/InvalidRegexTest.scala @@ -0,0 +1,9 @@ +/* +rule = InvalidRegexTest + */ +package fix + +object InvalidRegexTest { + val regex = "?".r // assert: InvalidRegexTest + val regex2 = "^[A-Z][A-Za-z0-9]*$".r // scalafix: ok; +} diff --git a/input/src/main/scala/fix/IsInstanceOf.scala b/input/src/main/scala/fix/IsInstanceOf.scala new file mode 100644 index 0000000..f0d1188 --- /dev/null +++ b/input/src/main/scala/fix/IsInstanceOf.scala @@ -0,0 +1,44 @@ +/* +rule = IsInstanceOf + */ +package fix + +object IsInstanceOf { + def test(): Unit = { + val s: Any = "sammy" + println(s.isInstanceOf[String]) // assert: IsInstanceOf + + case class MappingCharFilter(name: String, mappings: (String, String)*) // scalafix: ok; + + val pf: PartialFunction[Any, Unit] = { + case s: String => println(s) // scalafix: ok; + case i: Int if i == 4 => println(i) // scalafix: ok; + case _ => println("no match :(") // scalafix: ok; + } + + @SuppressWarnings(Array("all")) + def hello: Unit = { + val s: Any = "sammy" + println(s.isInstanceOf[String]) // scalafix: ok; + } + + @SuppressWarnings(Array("IsInstanceOf")) + def hello2: Unit = { + val s: Any = "sammy" + println(s.isInstanceOf[String]) // scalafix: ok; + } + + + sealed trait MyGADT[T] + final case class VariantInt(value: Int) extends MyGADT[Int] + final case class VariantString(value: String) extends MyGADT[String] + + def doStuff[T](gadt: MyGADT[T]): T = { // scalafix: ok; + gadt match { + case VariantInt(value) => value + case VariantString(value) => value + } + } + } + +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index 8706647..7248106 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -75,4 +75,6 @@ fix.FilterDotIsEmpty fix.FilterOptionAndGet fix.FinalModifierOnCaseClass fix.FindAndNotEqualsNoneReplaceWithExists -fix.FindDotIsDefined \ No newline at end of file +fix.FindDotIsDefined +fix.InvalidRegexTest +fix.IsInstanceOf \ No newline at end of file diff --git a/rules/src/main/scala/fix/InvalidRegexTest.scala b/rules/src/main/scala/fix/InvalidRegexTest.scala new file mode 100644 index 0000000..646ae32 --- /dev/null +++ b/rules/src/main/scala/fix/InvalidRegexTest.scala @@ -0,0 +1,39 @@ +/* +rule = InvalidRegexTest + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import java.util.regex.PatternSyntaxException +import scala.annotation.nowarn +import scala.meta._ + +class InvalidRegexTest extends SemanticRule("InvalidRegexTest") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for invalid regex literals.", + pos, + "Invalid regex literals can fail at compile time with a PatternSyntaxException. This could be caused by e.g. dangling meta characters, or unclosed escape characters, etc.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + + // Put in a function to avoid returning in pattern match, causing warn + def tryCompile(regex: String): Boolean = { + try regex.r + catch { + case _: PatternSyntaxException => return false + } + true + } + + doc.tree.collect { + case t @ Term.Select(Lit.String(regex), Term.Name("r")) if !tryCompile(regex) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} diff --git a/rules/src/main/scala/fix/IsInstanceOf.scala b/rules/src/main/scala/fix/IsInstanceOf.scala new file mode 100644 index 0000000..4b0dee6 --- /dev/null +++ b/rules/src/main/scala/fix/IsInstanceOf.scala @@ -0,0 +1,28 @@ +/* +rule = IsInstanceOf + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class IsInstanceOf extends SemanticRule("IsInstanceOf") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for use of isInstanceOf.", + pos, + "Use of isInstanceOf is considered a bad practice - consider using pattern matching instead.", + LintSeverity.Warning + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + // Corresponds to a.asInstanceOf(...) or a.asInstanceOf[...] + case t @ Term.Select(_, Term.Name("isInstanceOf")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} From 5058a9d8792ef589f051a2fb254ff7dce280afa3 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Sat, 23 Nov 2024 10:12:00 +0200 Subject: [PATCH 10/12] Added FilterDotSize --- input/src/main/scala/fix/FilterDotSize.scala | 12 +++++++++ .../META-INF/services/scalafix.v1.Rule | 23 ++++++++-------- rules/src/main/scala/fix/FilterDotSize.scala | 27 +++++++++++++++++++ 3 files changed, 51 insertions(+), 11 deletions(-) create mode 100644 input/src/main/scala/fix/FilterDotSize.scala create mode 100644 rules/src/main/scala/fix/FilterDotSize.scala diff --git a/input/src/main/scala/fix/FilterDotSize.scala b/input/src/main/scala/fix/FilterDotSize.scala new file mode 100644 index 0000000..8baa24f --- /dev/null +++ b/input/src/main/scala/fix/FilterDotSize.scala @@ -0,0 +1,12 @@ +/* +rule = FilterDotSize + */ +package fix + +object FilterDotSize { + def test(): Unit = { + List(1, 2, 3).filter(_ < 0).size // assert: FilterDotSize + List(1, 2, 3).map(e => e).size // scalafix: ok; + } + +} diff --git a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule index 7248106..771834a 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -39,12 +39,23 @@ fix.EmptyMethod fix.EmptySynchronizedBlock fix.EmptyTryBlock fix.EmptyWhileBlock +fix.ExistsSimplifiableToContains +fix.FilterDotHead +fix.FilterDotHeadOption +fix.FilterDotIsEmpty +fix.FilterDotSize +fix.FilterOptionAndGet +fix.FinalModifierOnCaseClass fix.FinalizerWithoutSuper +fix.FindAndNotEqualsNoneReplaceWithExists +fix.FindDotIsDefined fix.IllegalFormatString fix.ImpossibleOptionSizeCondition fix.IncorrectNumberOfArgsToFormat fix.IncorrectlyNamedExceptions fix.InterpolationToString +fix.InvalidRegexTest +fix.IsInstanceOf fix.LonelySealedTrait fix.LooksLikeInterpolatedString fix.MapGetAndGetOrElse @@ -67,14 +78,4 @@ fix.UnsafeTraversableMethods fix.UnusedMethodParameter fix.VarCouldBeVal fix.VariableShadowing -fix.WhileTrue -fix.ExistsSimplifiableToContains -fix.FilterDotHead -fix.FilterDotHeadOption -fix.FilterDotIsEmpty -fix.FilterOptionAndGet -fix.FinalModifierOnCaseClass -fix.FindAndNotEqualsNoneReplaceWithExists -fix.FindDotIsDefined -fix.InvalidRegexTest -fix.IsInstanceOf \ No newline at end of file +fix.WhileTrue \ No newline at end of file diff --git a/rules/src/main/scala/fix/FilterDotSize.scala b/rules/src/main/scala/fix/FilterDotSize.scala new file mode 100644 index 0000000..d450659 --- /dev/null +++ b/rules/src/main/scala/fix/FilterDotSize.scala @@ -0,0 +1,27 @@ +/* +rule = FilterDotSize + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class FilterDotSize extends SemanticRule("FilterDotSize") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for use of filter().size.", + pos, + "`filter().size` can be replaced with `count()`, which is more concise.", + LintSeverity.Info + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.Select(Term.Apply.After_4_6_0(Term.Select(_, Term.Name("filter")), _), Term.Name("size")) => Patch.lint(diag(t.pos)) + }.asPatch + } + +} From 3d438215a7d2b4e8205a6ecd299c952550e12e1d Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Sat, 23 Nov 2024 10:12:13 +0200 Subject: [PATCH 11/12] Scalaformatted --- input/src/main/scala/fix/IsInstanceOf.scala | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/input/src/main/scala/fix/IsInstanceOf.scala b/input/src/main/scala/fix/IsInstanceOf.scala index f0d1188..e5e02bf 100644 --- a/input/src/main/scala/fix/IsInstanceOf.scala +++ b/input/src/main/scala/fix/IsInstanceOf.scala @@ -11,9 +11,9 @@ object IsInstanceOf { case class MappingCharFilter(name: String, mappings: (String, String)*) // scalafix: ok; val pf: PartialFunction[Any, Unit] = { - case s: String => println(s) // scalafix: ok; + case s: String => println(s) // scalafix: ok; case i: Int if i == 4 => println(i) // scalafix: ok; - case _ => println("no match :(") // scalafix: ok; + case _ => println("no match :(") // scalafix: ok; } @SuppressWarnings(Array("all")) @@ -28,14 +28,13 @@ object IsInstanceOf { println(s.isInstanceOf[String]) // scalafix: ok; } - sealed trait MyGADT[T] final case class VariantInt(value: Int) extends MyGADT[Int] final case class VariantString(value: String) extends MyGADT[String] def doStuff[T](gadt: MyGADT[T]): T = { // scalafix: ok; gadt match { - case VariantInt(value) => value + case VariantInt(value) => value case VariantString(value) => value } } From ef40eeb915b2e4d58a363acf9306c2745368242f Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Sat, 23 Nov 2024 10:12:53 +0200 Subject: [PATCH 12/12] Changed license in build.sbt, added rules and updated to 1.1.4 --- build.sbt | 4 +- readme.md | 170 ++++++++++++++++++++++++++++++------------------------ 2 files changed, 98 insertions(+), 76 deletions(-) diff --git a/build.sbt b/build.sbt index 0f04f29..53337d6 100644 --- a/build.sbt +++ b/build.sbt @@ -8,12 +8,12 @@ inThisBuild( organizationName := "dedis", organizationHomepage := Some(url("https://dedis.epfl.ch")), homepage := Some(url("https://github.com/dedis/scapegoat-scalafix")), - licenses := List("AGPL 3.0" -> url("https://www.gnu.org/licenses/agpl-3.0.en.html")), + licenses := List("GPL 3.0" -> url("https://www.gnu.org/licenses/gpl-3.0.en.html")), developers := List(Developer("t1b00", "Thibault Czarniak", "thibault.czarniak@epfl.ch", url("https://www.linkedin.com/in/thcz/"))), semanticdbEnabled := true, semanticdbVersion := scalafixSemanticdb.revision, scmInfo := Some(ScmInfo(url("https://github.com/dedis/scapegoat-scalafix"), "scm:git@github.com:dedis/scapegoat-scalafix.git")), - version := "1.1.3", + version := "1.1.4", versionScheme := Some("pvp") ) ) diff --git a/readme.md b/readme.md index 04f2429..ab17117 100644 --- a/readme.md +++ b/readme.md @@ -13,7 +13,7 @@ ## Description This project is a Scalafix implementation of the Scapegoat linter for Scala 3. It contains a set of rules that can be run on Scala code to detect potential issues and bad practices. The rules are based on the Scapegoat linter for Scala 2, but have been adapted to work with Scalafix and Scala 3. -For now, **this project has 70 rules** but more are being worked on. +For now, **this project has 81 rules** but more are being worked on. You can track the progress [here](https://docs.google.com/spreadsheets/d/1XovJJg3EInQFFL1-tpqGxpP2O4RFzCOF7zDalQIeB7E/edit?usp=sharing). ## Installation @@ -26,7 +26,7 @@ addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.12.1") Then, to obtain this rule set, simply add the following line to your `build.sbt` file: ``` -ThisBuild / scalafixDependencies += "io.github.dedis" %% "scapegoat-scalafix" % "1.1.3" +ThisBuild / scalafixDependencies += "io.github.dedis" %% "scapegoat-scalafix" % "1.1.4" ``` **The rules are compatible with Scala 2.13 and Scala 3 (tested for Scala 3.3.1).** @@ -47,78 +47,89 @@ inThisBuild( This is necessary to enable the SemanticDB, which is required for the rules to work. Only add these lines if SemanticDB is not already enabled in the `build.sbt`. ## Rule list -| Name |Brief Description |Default Level| -|------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------|-------------| -| ArrayEquals |Checks for comparison of arrays using `==` which will always return false |Info | -| ArraysInFormat |Checks for arrays passed to String.format |Error | -| ArraysToString |Checks for explicit toString calls on arrays |Warning | -| AsInstanceOf |Checks for use of `asInstanceOf` |Warning | -| AvoidSizeEqualsZero |Traversable.size can be slow for some data structure, prefer .isEmpty |Warning | -| AvoidSizeNotEqualsZero |Traversable.size can be slow for some data structure, prefer .nonEmpty |Warning | -| AvoidToMinusOne |Checks for loops that use `x to n-1` instead of `x until n` |Info | -| BigDecimalDoubleConstructor |Checks for use of `BigDecimal(double)` which can be unsafe |Warning | -| BigDecimalScaleWithoutRoundingMode |`setScale()` on a `BigDecimal` without setting the rounding mode can throw an exception |Warning | -| BooleanParameter |Checks for functions that have a Boolean parameter |Info | -| BoundedByFinalType |Looks for types with upper bounds of a final type |Warning | -| BrokenOddness |Checks for a % 2 == 1 for oddness because this fails on negative numbers |Warning | -| CatchException |Checks for try blocks that catch Exception |Warning | -| CatchExceptionImmediatelyRethrown |Checks for try-catch blocks that immediately rethrow caught exceptions. |Warning | -| CatchFatal |Checks for try blocks that catch fatal exceptions: VirtualMachineError, ThreadDeath, InterruptedException, LinkageError, ControlThrowable|Warning | -| CatchNpe |Checks for try blocks that catch null pointer exceptions |Error | -| CatchThrowable |Checks for try blocks that catch Throwable |Warning | -| ClassNames |Ensures class names adhere to the style guidelines |Info | -| CollectionIndexOnNonIndexedSeq |Checks for indexing on a Seq which is not an IndexedSeq |Warning | -| CollectionNamingConfusion |Checks for variables that are confusingly named |Info | -| CollectionNegativeIndex |Checks for negative access on a sequence eg `list.get(-1)` |Warning | -| CollectionPromotionToAny |Checks for collection operations that promote the collection to `Any` |Warning | -| ComparingFloatingPointTypes |Checks for equality checks on floating point types |Error | -| ComparisonToEmptyList |Checks for code like `a == List()` or `a == Nil` |Info | -| ComparisonToEmptySet |Checks for code like `a == Set()` or `a == Set.empty` |Info | -| ComparisonWithSelf |Checks for equality checks with itself |Warning | -| ConstantIf |Checks for code where the if condition compiles to a constant |Warning | -| DivideByOne |Checks for divide by one, which always returns the original value |Warning | -| DoubleNegation |Checks for code like `!(!b)` |Info | -| DuplicateImport |Checks for import statements that import the same selector |Info | -| DuplicateMapKey |Checks for duplicate key names in Map literals |Warning | -| DuplicateSetValue |Checks for duplicate values in set literals |Warning | -| EitherGet |Checks for use of .get on Left or Right |Error | -| EmptyCaseClass |Checks for case classes like `case class Faceman()` |Info | -| EmptyFor |Checks for empty `for` loops |Warning | -| EmptyIfBlock |Checks for empty `if` blocks |Warning | -| EmptyInterpolatedString |Looks for interpolated strings that have no arguments |Error | -| EmptyMethod |Looks for empty methods |Warning | -| EmptySynchronizedBlock |Looks for empty synchronized blocks |Warning | -| EmptyTryBlock |Looks for empty try blocks |Warning | -| EmptyWhileBlock |Looks for empty while loops |Warning | -| FinalizerWithoutSuper |Checks for overridden finalizers that do not call super |Warning | -| IllegalFormatString |Looks for invalid format strings |Error | -| ImpossibleOptionSizeCondition |Checks for code like `option.size > 2` which can never be true |Error | -| IncorrectNumberOfArgsToFormat |Checks for wrong number of arguments to `String.format` |Error | -| IncorrectlyNamedExceptions |Checks for exceptions that are not called *Exception and vice versa |Error | -| InterpolationToString |Checks for string interpolations that have .toString in their arguments |Warning | -| LonelySealedTrait |Checks for sealed traits which have no implementation |Error | -| LooksLikeInterpolatedString |Finds strings that look like they should be interpolated but are not |Warning | -| MapGetAndGetOrElse |`Map.get(key).getOrElse(value)` can be replaced with `Map.getOrElse(key, value)` |Error | -| MethodReturningAny |Checks for defs that are defined or inferred to return `Any` |Warning | -| NanComparison |Checks for `x == Double.NaN` which will always fail |Error | -| NullAssignment |Checks for use of `null` in assignments |Warning | -| NullParameter |Checks for use of `null` in method invocation |Warning | -| OptionGet |Checks for `Option.get` |Error | -| OptionSize |Checks for `Option.size` |Error | -| RepeatedCaseBody |Checks for case statements which have the same body |Warning | -| RepeatedIfElseBody |Checks for the main branch and the else branch of an `if` being the same |Warning | -| StripMarginOnRegex |Checks for .stripMargin on regex strings that contain a pipe |Error | -| SwallowedException |Finds catch blocks that don't handle caught exceptions |Warning | -| TryGet |Checks for use of `Try.get` |Error | -| UnnecessaryConversion |Checks for unnecessary `toInt` on instances of Int or `toString` on Strings, etc. |Warning | -| UnreachableCatch |Checks for catch clauses that cannot be reached |Warning | -| UnsafeContains |Checks for `List.contains(value)` for invalid types |Error | -| UnsafeStringContains |Checks for `String.contains(value)` for invalid types |Error | -| UnsafeTraversableMethods |Check unsafe traversable method usages (head, tail, init, last, reduce, reduceLeft, reduceRight, max, maxBy, min, minBy) |Error | -| UnusedMethodParameter |Checks for unused method parameters |Warning | -| VarCouldBeVal |Checks for `var`s that could be declared as `val`s |Warning | -| VariableShadowing |Checks for multiple uses of the variable name in nested scopes |Warning | -| WhileTrue |Checks for code that uses a `while(true)` or `do { } while(true)` block. |Warning | +| Name | Brief Description | Default Level | +|---------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------|---------------| +| ArrayEquals | Checks for comparison of arrays using `==` which will always return false | Info | +| ArraysInFormat | Checks for arrays passed to String.format | Error | +| ArraysToString | Checks for explicit toString calls on arrays | Warning | +| AsInstanceOf | Checks for use of `asInstanceOf` | Warning | +| AvoidSizeEqualsZero | Traversable.size can be slow for some data structure, prefer .isEmpty | Warning | +| AvoidSizeNotEqualsZero | Traversable.size can be slow for some data structure, prefer .nonEmpty | Warning | +| AvoidToMinusOne | Checks for loops that use `x to n-1` instead of `x until n` | Info | +| BigDecimalDoubleConstructor | Checks for use of `BigDecimal(double)` which can be unsafe | Warning | +| BigDecimalScaleWithoutRoundingMode | `setScale()` on a `BigDecimal` without setting the rounding mode can throw an exception | Warning | +| BooleanParameter | Checks for functions that have a Boolean parameter | Info | +| BoundedByFinalType | Looks for types with upper bounds of a final type | Warning | +| BrokenOddness | Checks for a % 2 == 1 for oddness because this fails on negative numbers | Warning | +| CatchException | Checks for try blocks that catch Exception | Warning | +| CatchExceptionImmediatelyRethrown | Checks for try-catch blocks that immediately rethrow caught exceptions. | Warning | +| CatchFatal | Checks for try blocks that catch fatal exceptions: VirtualMachineError, ThreadDeath, InterruptedException, LinkageError, ControlThrowable | Warning | +| CatchNpe | Checks for try blocks that catch null pointer exceptions | Error | +| CatchThrowable | Checks for try blocks that catch Throwable | Warning | +| ClassNames | Ensures class names adhere to the style guidelines | Info | +| CollectionIndexOnNonIndexedSeq | Checks for indexing on a Seq which is not an IndexedSeq | Warning | +| CollectionNamingConfusion | Checks for variables that are confusingly named | Info | +| CollectionNegativeIndex | Checks for negative access on a sequence eg `list.get(-1)` | Warning | +| CollectionPromotionToAny | Checks for collection operations that promote the collection to `Any` | Warning | +| ComparingFloatingPointTypes | Checks for equality checks on floating point types | Error | +| ComparisonToEmptyList | Checks for code like `a == List()` or `a == Nil` | Info | +| ComparisonToEmptySet | Checks for code like `a == Set()` or `a == Set.empty` | Info | +| ComparisonWithSelf | Checks for equality checks with itself | Warning | +| ConstantIf | Checks for code where the if condition compiles to a constant | Warning | +| DivideByOne | Checks for divide by one, which always returns the original value | Warning | +| DoubleNegation | Checks for code like `!(!b)` | Info | +| DuplicateImport | Checks for import statements that import the same selector | Info | +| DuplicateMapKey | Checks for duplicate key names in Map literals | Warning | +| DuplicateSetValue | Checks for duplicate values in set literals | Warning | +| EitherGet | Checks for use of .get on Left or Right | Error | +| EmptyCaseClass | Checks for case classes like `case class Faceman()` | Info | +| EmptyFor | Checks for empty `for` loops | Warning | +| EmptyIfBlock | Checks for empty `if` blocks | Warning | +| EmptyInterpolatedString | Looks for interpolated strings that have no arguments | Error | +| EmptyMethod | Looks for empty methods | Warning | +| EmptySynchronizedBlock | Looks for empty synchronized blocks | Warning | +| EmptyTryBlock | Looks for empty try blocks | Warning | +| EmptyWhileBlock | Looks for empty while loops | Warning | +| ExistsSimplifiableToContains | `exists(x => x == b)` replaceable with `contains(b)` | Info | +| FilterDotHead | `.filter(x => ).head` can be replaced with `find(x => ) match { .. } ` | Info | +| FilterDotHeadOption | `.filter(x =>).headOption` can be replaced with `find(x => )` | Info | +| FilterDotIsEmpty | `.filter(x => ).isEmpty` can be replaced with `!exists(x => )` | Info | +| FilterDotSize | `.filter(x => ).size` can be replaced more concisely with with `count(x => )` | Info | +| FilterOptionAndGet | `.filter(_.isDefined).map(_.get)` can be replaced with `flatten` | Info | +| FinalModifierOnCaseClass | Using Case classes without `final` modifier can lead to surprising breakage | Info | +| FinalizerWithoutSuper | Checks for overridden finalizers that do not call super | Warning | +| FindAndNotEqualsNoneReplaceWithExists | `.find(x => ) != None` can be replaced with `exists(x => )` | Info | +| FindDotIsDefined | `find(x => ).isDefined` can be replaced with `exist(x => )` | Info | +| IllegalFormatString | Looks for invalid format strings | Error | +| ImpossibleOptionSizeCondition | Checks for code like `option.size > 2` which can never be true | Error | +| IncorrectNumberOfArgsToFormat | Checks for wrong number of arguments to `String.format` | Error | +| IncorrectlyNamedExceptions | Checks for exceptions that are not called *Exception and vice versa | Error | +| InvalidRegex | Checks for invalid regex literals | Info | +| IsInstanceOf | Checks for use of `isInstanceOf` | Warning | +| InterpolationToString | Checks for string interpolations that have .toString in their arguments | Warning | +| LonelySealedTrait | Checks for sealed traits which have no implementation | Error | +| LooksLikeInterpolatedString | Finds strings that look like they should be interpolated but are not | Warning | +| MapGetAndGetOrElse | `Map.get(key).getOrElse(value)` can be replaced with `Map.getOrElse(key, value)` | Error | +| MethodReturningAny | Checks for defs that are defined or inferred to return `Any` | Warning | +| NanComparison | Checks for `x == Double.NaN` which will always fail | Error | +| NullAssignment | Checks for use of `null` in assignments | Warning | +| NullParameter | Checks for use of `null` in method invocation | Warning | +| OptionGet | Checks for `Option.get` | Error | +| OptionSize | Checks for `Option.size` | Error | +| RepeatedCaseBody | Checks for case statements which have the same body | Warning | +| RepeatedIfElseBody | Checks for the main branch and the else branch of an `if` being the same | Warning | +| StripMarginOnRegex | Checks for .stripMargin on regex strings that contain a pipe | Error | +| SwallowedException | Finds catch blocks that don't handle caught exceptions | Warning | +| TryGet | Checks for use of `Try.get` | Error | +| UnnecessaryConversion | Checks for unnecessary `toInt` on instances of Int or `toString` on Strings, etc. | Warning | +| UnreachableCatch | Checks for catch clauses that cannot be reached | Warning | +| UnsafeContains | Checks for `List.contains(value)` for invalid types | Error | +| UnsafeStringContains | Checks for `String.contains(value)` for invalid types | Error | +| UnsafeTraversableMethods | Check unsafe traversable method usages (head, tail, init, last, reduce, reduceLeft, reduceRight, max, maxBy, min, minBy) | Error | +| UnusedMethodParameter | Checks for unused method parameters | Warning | +| VarCouldBeVal | Checks for `var`s that could be declared as `val`s | Warning | +| VariableShadowing | Checks for multiple uses of the variable name in nested scopes | Warning | +| WhileTrue | Checks for code that uses a `while(true)` or `do { } while(true)` block. | Warning | ## Usage @@ -172,12 +183,23 @@ rules = [ EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileBlock, + ExistsSimplifiableToContains, + FilterDotHead, + FilterDotHeadOption, + FilterDotIsEmpty, + FilterDotSize, + FilterOptionAndGet, + FinalModifierOnCaseClass, FinalizerWithoutSuper, + FindAndNotEqualsNoneReplaceWithExists, + FindDotIsDefined, IllegalFormatString, ImpossibleOptionSizeCondition, IncorrectNumberOfArgsToFormat, IncorrectlyNamedExceptions, InterpolationToString, + InvalidRegexTest, + IsInstanceOf, LonelySealedTrait, LooksLikeInterpolatedString, MapGetAndGetOrElse,