Skip to content

Commit

Permalink
Added VariableShadowing
Browse files Browse the repository at this point in the history
  • Loading branch information
t1b00 committed Aug 26, 2024
1 parent 9c7382a commit 1886a00
Show file tree
Hide file tree
Showing 2 changed files with 274 additions and 0 deletions.
153 changes: 153 additions & 0 deletions input/src/main/scala/fix/VariableShadowing.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
/*
rule = VariableShadowing
*/
package fix

object VariableShadowing {

def foo1 = {
val a = 1
def boo = {
val a = 2 // assert: VariableShadowing
println(a)
}
}

class Test2 {
val a = 1
def foo = {
val a = 2 // assert: VariableShadowing
println(a)
}
}

class Test3 {
val a = 1
def foo(b: Int) = b match {
case a => println(a) // assert: VariableShadowing
}
}

class Test4 {
val a = 1
def foo(b : Int) = b match {
case f =>
def boo() = {
val a = 2 // assert: VariableShadowing
println(a)
}
}
}

class Test5 {
def foo(a : Int) = {
val a = 1 // assert: VariableShadowing
println(a)
}
}

class Test6 {
def foo(a : Int) = {
println(a)
def boo = {
val a = 2 // assert: VariableShadowing
println(a)
}
}
}

class Test7 {
def foo = {
val a = 1 // scalafix: ok;
println(a)
}
def boo = {
val a = 2 // scalafix: ok;
println(a)
}
}

class Test8 {
val something = 4
if (1 > 0) {
val something = 4 // assert: VariableShadowing
println(something+1)
} else {
val something = 2 // assert: VariableShadowing
println(something+2)
}
}

class Test9 {
if (1 > 0) {
val something = 4 // scalafix: ok;
println(something+1)
} else {
val something = 2 // scalafix: ok;
println(something+2)
}
}

class Test11 {
val possibility: Option[Int] = Some(3)
possibility match {
case Some(x) =>
val y = x + 1 // scalafix: ok;
println(y)
case None =>
val y = 0 // scalafix: ok;
println(y)
}
}

class Test12 {
val possibility: Option[Int] = Some(3)
possibility match {
case Some(x) =>
val y = x + 1 // scalafix: ok;
println(y)
case None => println("None")
}
}

final case class A(value: String) // scalafix: ok;
final case class B(value: String) // scalafix: ok;
final case class C(value: Int) // scalafix: ok;

for (i <- 1 to 10) println(i.toString) // scalafix: ok;
for (i <- 1 to 10) println(i.toString) // scalafix: ok;


for {
c <- "Hello, world!" // scalafix: ok;
if c != ','
} println(c)

object Test1 {

def f(x: Int): String = x.toString
def g(y: String): Int = y.toInt

val a = Seq(1, 2, 3) // scalafix: ok;
System.out.println(
a // scalafix: ok;
.map(s => f(s))
.map(s => g(s))
)
}

class TestVariableShadowing {
private def testCallbackFunction1(shadowedArg: Long): Boolean = shadowedArg > 10
private def testCallbackFunction2(arg: Long): Boolean = arg < 10

private def testCaller(renamedArg: Long, func: Long => Boolean): Boolean = func(renamedArg)

def test(shadowedArg: AnyVal): Boolean =
shadowedArg match {
case l1: Long => testCaller(l1, testCallbackFunction1) // scalafix: ok;
case l2: Double => testCaller(l2.toLong, testCallbackFunction2)
case l3 => false
}
}

}
121 changes: 121 additions & 0 deletions rules/src/main/scala/fix/VariableShadowing.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/*
rule = VariableShadowing
*/
package fix

import scalafix.lint.LintSeverity
import scalafix.v1._

import scala.annotation.nowarn
import scala.collection.mutable
import scala.meta._

class VariableShadowing extends SemanticRule("VariableShadowing") {

private def diag(pos: Position) = Diagnostic(
"",
"Checks for multiple uses of the variable name in nested scopes.",
pos,
"Variable shadowing is very useful, but can easily lead to nasty bugs in your code. Shadowed variables can be potentially confusing to other maintainers when the same name is adopted to have a new meaning in a nested scope.",
LintSeverity.Warning
)
override def fix(implicit doc: SemanticDocument): Patch = {

def collect(block: List[Stat], vars: mutable.HashSet[String]): List[Patch] = {
// Extracts the stats from blocks and otherwise converts them so that we can do a foreach on them
def normalize(term: Stat) = term match {
case Term.Block(stats) => stats
case _ => List(term)
}

@nowarn // this is because in Scala 2, t.children at line 47 is a List[Tree], but in Scala 3 it is a List[Stat], so we need to suppress the warning for the unreachable case case _ in Scala 3
def collectInner(block: List[Stat], vars: mutable.HashSet[String], flagged: mutable.HashSet[Position]): Unit = {


def handleCases(c: List[Case], vars: mutable.HashSet[String]): Unit = {
c.foreach {
// Collect variables in cases e.g. case a => ...
case Case(Pat.Var(name), _, body) => updateVars(name.value, name.pos); collectInner(normalize(body), vars.clone(), flagged)
case _ => ()
}
}

def updateVars(name: String, pos: Position): Unit = {
if(vars.contains(name)) flagged += pos
else vars += name
}

block.foreach {
// Corresponds to var a = ...
case Defn.Val(_, List(Pat.Var(name)), _, _) => updateVars(name.value, name.pos)
case Defn.Var.After_4_7_2(_, List(Pat.Var(name)), _, _) => updateVars(name.value, name.pos)
// Examine paramclauses e.g. def foo(a: Int) or class bar(b: Int)
case Term.ParamClause(params, _) => params.foreach(p => updateVars(p.name.value, p.name.pos))

// Blocks {} and templates
case Term.Block(stats) => collectInner(stats, vars.clone(), flagged)
case Template.After_4_4_0(_, _, _, stats, _) => collectInner(stats, vars.clone(), flagged)

case Defn.Def.After_4_6_0(_, _, paramClauseGroup, _, Term.Block(stats)) =>
// Collect parameters in method e.g. def(a : Int) = ...
if(paramClauseGroup.isDefined) paramClauseGroup.get.paramClauses.foreach(p => p.values.foreach(param => updateVars(param.name.value, param.name.pos)))
collectInner(stats, vars.clone(), flagged)

// Examine for loop body recursively. Condition cannot be a block so no need to be examined
case Term.For(_, body) => collectInner(normalize(body), vars.clone(), flagged)
// Examine the body of the if condition and the body of the else condition recursively
case Term.If.After_4_4_0(_, thenBody, elseBody, _) => collectInner(normalize(thenBody), vars.clone(), flagged); collectInner(normalize(elseBody), vars.clone(), flagged)
// Examine the body of the match cases recursively
case Term.Match.After_4_4_5(_, cases, _) => handleCases(cases, vars)
// Examine body, catches and finally condition of the try block recursively
case Term.Try(body, catches, finallyp) =>
collectInner(normalize(body), vars.clone(), flagged)
handleCases(catches, vars)
if (finallyp.isDefined) collectInner(normalize(finallyp.get), vars.clone(), flagged)
// Examine the condition and body of the while loop recursively, condition can have block / assignment inside
case Term.While(condition, body) => collectInner(normalize(condition), vars.clone(), flagged); collectInner(normalize(body), vars.clone(), flagged)

// Examine partial functions and functions (e.g. l.collect { ... } or l.foreach(e => ...)
case Term.PartialFunction(cases) => cases.foreach(c => collectInner(normalize(c.body), vars.clone(), flagged))
case Term.Function.After_4_6_0(_, body) => collectInner(normalize(body), vars.clone(), flagged)

// Examine argclauses e.g. l.foreach(e => ...), e => ... is the argclause
case Term.ArgClause(args, _) => args.foreach(a => collectInner(normalize(a), vars.clone(), flagged))


// Try to handle everything else
case t: Stat =>
t.children.foreach { // t.children is Tree, not compatible with normalize
case c: Stat => collectInner(normalize(c), vars.clone(), flagged)
case Term.ArgClause(args, _) => args.foreach(a => collectInner(normalize(a), vars.clone(), flagged)) // For some reason, ArgClause is not considered as a Stat even though it inherits from it
case _ => ()
}

case _ => ()
}
}
val flagged = mutable.HashSet.empty[Position]
collectInner(block, vars, flagged)
flagged.map(p => Patch.lint(diag(p))).toList
}

def collectWithConstructor(params: Seq[Term.ParamClause], stats: List[Stat]): List[Patch] = {
val vars = mutable.HashSet.empty[String]
params.foreach(c => c.values.foreach(p => vars += p.name.value))
collect(stats, vars)
}

// We first look at the templates (start of a definition) i.e. start of a scope (variables don't go outside)
// We also look at blocks as context themselves, because a variable could be declared in a block and if it is
// declared again inside of it, it should be flagged
doc.tree.collect {
// We don't handle template directly to be able to collect the variables in the constructor
case Defn.Class.After_4_6_0(_, _, _, Ctor.Primary.After_4_6_0(_, _, params), Template.After_4_4_0(_, _, _, stats, _)) => collectWithConstructor(params, stats)
case Defn.Trait.After_4_6_0(_, _, _, Ctor.Primary.After_4_6_0(_, _, params), Template.After_4_4_0(_, _, _, stats, _)) => collectWithConstructor(params, stats)
case Defn.Enum.After_4_6_0(_, _, _, Ctor.Primary.After_4_6_0(_, _, params), Template.After_4_4_0(_, _, _, stats, _)) => collectWithConstructor(params, stats)
case Defn.Object(_, _, Template.After_4_4_0(_, _, _, stats, _)) => collect(stats, mutable.HashSet.empty[String])
// Inspect blocks as contexts themselves
case Term.Block(stats) => collect(stats, mutable.HashSet.empty[String])
}.flatten.asPatch
}
}

0 comments on commit 1886a00

Please sign in to comment.