From e3f4cce9e5cdbff0ee0363da97bc551c7514d65e Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Thu, 15 Aug 2024 12:22:55 +0200 Subject: [PATCH] Added RepeatedIfElseBody and SwallowedException --- .../main/scala/fix/RepeatedIfElseBody.scala | 37 ++++++++++++ .../main/scala/fix/SwallowedException.scala | 59 +++++++++++++++++++ .../META-INF/services/scalafix.v1.Rule | 4 +- .../main/scala/fix/RepeatedIfElseBody.scala | 33 +++++++++++ .../main/scala/fix/SwallowedException.scala | 48 +++++++++++++++ 5 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 input/src/main/scala/fix/RepeatedIfElseBody.scala create mode 100644 input/src/main/scala/fix/SwallowedException.scala create mode 100644 rules/src/main/scala/fix/RepeatedIfElseBody.scala create mode 100644 rules/src/main/scala/fix/SwallowedException.scala diff --git a/input/src/main/scala/fix/RepeatedIfElseBody.scala b/input/src/main/scala/fix/RepeatedIfElseBody.scala new file mode 100644 index 0000000..e149821 --- /dev/null +++ b/input/src/main/scala/fix/RepeatedIfElseBody.scala @@ -0,0 +1,37 @@ +/* +rule = RepeatedIfElseBody + */ +package fix + +object RepeatedIfElseBody { + + def test(): Unit = { + val a = "input" + println(if (a.length > 3) { // assert: RepeatedIfElseBody + "long string" + } else { + "long string" + }) + + if (1 > 0) { // assert: RepeatedIfElseBody + val len = a.length + println(s"Length is $len") + } else { + val len = a.length + println("I won't say") + } + + println(if (a.length > 3) { // scalafix: ok; + "long string" + } else { + "short string" + }) + + if (a.length > 3) println(42) // assert: RepeatedIfElseBody + else println(42) + + if (a.length > 3) println("olivia") // scalafix: ok; + else println(42) + } + +} diff --git a/input/src/main/scala/fix/SwallowedException.scala b/input/src/main/scala/fix/SwallowedException.scala new file mode 100644 index 0000000..94a863f --- /dev/null +++ b/input/src/main/scala/fix/SwallowedException.scala @@ -0,0 +1,59 @@ +/* +rule = SwallowedException + */ +package fix + +object SwallowedException { + + def test(): Unit = { + try {} catch { + case e: Exception => // assert: SwallowedException + } + + try {} catch { + case e: Exception => // assert: SwallowedException + case e: Throwable => // assert: SwallowedException + } + + try {} catch { + case e: Exception => println("a") // scalafix: ok; + case e: Throwable => // assert: SwallowedException + } + + try { + println(Integer.valueOf("asdf")) + } catch { + case nfe: NumberFormatException => // assert: SwallowedException + throw new IllegalArgumentException("not a number") + } + + def error(e: Exception, str: String): Unit = println("Log ERROR " + str + " " + e) + try { + println(Integer.valueOf("asdf")) + } catch { + case nfe2: NumberFormatException => // assert: SwallowedException + error(nfe2, "Invalid format") + throw new IllegalStateException("it's not a number") + } + + try {} catch { + case e: RuntimeException => println("a") // scalafix: ok; + case f: Exception => println("b") // scalafix: ok; + case x: Throwable => println("c") // scalafix: ok; + } + + try { println() } + catch { case ignored: Exception => } // scalafix: ok; + + try { println() } + catch { case ignore: Exception => } // scalafix: ok; + + try { + println(Integer.valueOf("asdf")) + } catch { + case nfe: NumberFormatException => + throw new IllegalArgumentException("not a number", nfe) // 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 0882904..1a55ef5 100644 --- a/rules/src/main/resources/META-INF/services/scalafix.v1.Rule +++ b/rules/src/main/resources/META-INF/services/scalafix.v1.Rule @@ -38,4 +38,6 @@ fix.FinalizerWithoutSuper fix.LooksLikeInterpolatedString fix.MethodReturningAny fix.NullAssignment -fix.RepeatedCaseBody \ No newline at end of file +fix.RepeatedCaseBody +fix.RepeatedIfElseBody +fix.SwallowedException \ No newline at end of file diff --git a/rules/src/main/scala/fix/RepeatedIfElseBody.scala b/rules/src/main/scala/fix/RepeatedIfElseBody.scala new file mode 100644 index 0000000..afe9679 --- /dev/null +++ b/rules/src/main/scala/fix/RepeatedIfElseBody.scala @@ -0,0 +1,33 @@ +/* +rule = RepeatedIfElseBody + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.collection.mutable +import scala.meta._ + +class RepeatedIfElseBody extends SemanticRule("RepeatedIfElseBody") { + + private def diag(pos: Position) = Diagnostic( + "", + "Checks for the main branch and the else branch of an if being the same.", + pos, + "The if statement could be refactored if both branches are the same or start with the same instruction.", + LintSeverity.Warning + ) + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case t @ Term.If.After_4_4_0(_, Term.Block(ifStats), Term.Block(elseStats), _) => + if (ifStats.toString() == elseStats.toString()) Patch.lint(diag(t.pos)) // Blocks are the same + else (ifStats, elseStats) match { + case (headIf :: _, headElse :: _) if headIf.toString() == headElse.toString() => Patch.lint(diag(t.pos)) // First statement is the same + case _ => Patch.empty + } + case t @ Term.If.After_4_4_0(_, ifStats, elseStats, _) if ifStats.toString() == elseStats.toString() => Patch.lint(diag(t.pos)) // If and else are the same, but not blocks e.g. if (a) 1 else 1 + }.asPatch + } +} diff --git a/rules/src/main/scala/fix/SwallowedException.scala b/rules/src/main/scala/fix/SwallowedException.scala new file mode 100644 index 0000000..75fb2c2 --- /dev/null +++ b/rules/src/main/scala/fix/SwallowedException.scala @@ -0,0 +1,48 @@ +/* +rule = SwallowedException + */ +package fix + +import scalafix.lint.LintSeverity +import scalafix.v1._ + +import scala.meta._ + +class SwallowedException extends SemanticRule("SwallowedException") { + + private def diag(pos: Position) = Diagnostic( + "", + "Finds catch blocks that don't handle caught exceptions.", + pos, + "If you use a try/catch block to deal with an exception, you should handle all of the caught exceptions or name it ignore(d). If you're throwing another exception in the result, you should include the original exception as the cause.", + LintSeverity.Warning + ) + + private def containsCorrectRethrow(stats: List[Stat], name: String): Boolean = { + // This first finds the throw statement, then checks if the values in the new thrown exception contain the original exception + stats.exists { + // For some reason in Scalameta, throws can have multiple argclauses (e.g. throw new Exception("a")("b") i.e. exception currying) + // In practice, we choose to ignore this case as this is really uncommon. If that becomes an issue, a fix is quite easy to implement. + case Term.Throw(Term.New(Init.After_4_6_0(_, _, List(Term.ArgClause(values, _))))) => // Find throw statement + values.exists { + case Term.Name(otherName) if otherName == name => true // Check if the exception is rethrown + case _ => false + } + case _ => false + } + } + + override def fix(implicit doc: SemanticDocument): Patch = { + doc.tree.collect { + case Term.Try(_, catches, _) => + catches.collect { + // Pat.Typed(Pat.Var(Term.Name(e)), Excep) roughly corresponds to case e: Excep + case Case(Pat.Typed(Pat.Var(Term.Name("ignore" | "ignored")), _), _, _) => Patch.empty // Ignore cases where the exception is named ignore(d) + case c @ Case(_, _, Term.Block(Nil) | Lit.Unit()) => Patch.lint(diag(c.pos)) // Empty catch block + case c @ Case(Pat.Typed(Pat.Var(Term.Name(name)), _), _, Term.Block(stats)) if !containsCorrectRethrow(stats, name) => Patch.lint(diag(c.pos)) // If the catch block contains a throw that doesn't include the caught exception + case c @ Case(Pat.Typed(Pat.Var(Term.Name(name)), _), _, thr @ Term.Throw(_)) if !containsCorrectRethrow(List(thr), name) => Patch.lint(diag(c.pos)) // Same, but with direct throw statement instead of block + case _ => Patch.empty + } + }.flatten.asPatch + } +}