Skip to content

Commit

Permalink
Added RepeatedIfElseBody and SwallowedException
Browse files Browse the repository at this point in the history
  • Loading branch information
t1b00 committed Aug 15, 2024
1 parent 8f98076 commit e3f4cce
Show file tree
Hide file tree
Showing 5 changed files with 180 additions and 1 deletion.
37 changes: 37 additions & 0 deletions input/src/main/scala/fix/RepeatedIfElseBody.scala
Original file line number Diff line number Diff line change
@@ -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)
}

}
59 changes: 59 additions & 0 deletions input/src/main/scala/fix/SwallowedException.scala
Original file line number Diff line number Diff line change
@@ -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;
}
}

}
4 changes: 3 additions & 1 deletion rules/src/main/resources/META-INF/services/scalafix.v1.Rule
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,6 @@ fix.FinalizerWithoutSuper
fix.LooksLikeInterpolatedString
fix.MethodReturningAny
fix.NullAssignment
fix.RepeatedCaseBody
fix.RepeatedCaseBody
fix.RepeatedIfElseBody
fix.SwallowedException
33 changes: 33 additions & 0 deletions rules/src/main/scala/fix/RepeatedIfElseBody.scala
Original file line number Diff line number Diff line change
@@ -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
}
}
48 changes: 48 additions & 0 deletions rules/src/main/scala/fix/SwallowedException.scala
Original file line number Diff line number Diff line change
@@ -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
}
}

0 comments on commit e3f4cce

Please sign in to comment.