From 3c3c1973cb501ea2fa2d347654ab871c34feeda9 Mon Sep 17 00:00:00 2001 From: Thibault Czarniak Date: Wed, 28 Aug 2024 16:12:37 +0200 Subject: [PATCH] Improved UnusedMethodParameter --- .../scala/fix/UnusedMethodParameter.scala | 17 ++- .../scala/fix/UnusedMethodParameter.scala | 134 +++++++++--------- 2 files changed, 84 insertions(+), 67 deletions(-) diff --git a/input/src/main/scala/fix/UnusedMethodParameter.scala b/input/src/main/scala/fix/UnusedMethodParameter.scala index 3a982aa..5efc8a4 100644 --- a/input/src/main/scala/fix/UnusedMethodParameter.scala +++ b/input/src/main/scala/fix/UnusedMethodParameter.scala @@ -13,7 +13,7 @@ object UnusedMethodParameter { class Test1 { val initstuff = "sammy" // assert: UnusedMethodParameter - val test = ??? // scalafix: ok; + val test = ??? // assert: UnusedMethodParameter def foo(a: String, b: Int, c: Int): Unit = { // assert: UnusedMethodParameter println(b) @@ -124,4 +124,19 @@ object UnusedMethodParameter { def foo(x: Int) = 42 // assert: UnusedMethodParameter def fooUnused(@unused x: Int) = 42 // scalafix: ok; } + + trait A + class MessageHandler { + val dbActor: A = ??? // assert: UnusedMethodParameter + } + class FederationHandler(dbRef: => A) extends MessageHandler { // scalafix: ok; + class B {} + + override final val dbActor: A = dbRef // assert: UnusedMethodParameter + + def olivia(a: Int) = { // assert: UnusedMethodParameter + println("test") + } + } + } diff --git a/rules/src/main/scala/fix/UnusedMethodParameter.scala b/rules/src/main/scala/fix/UnusedMethodParameter.scala index fa7cd62..5495aa8 100644 --- a/rules/src/main/scala/fix/UnusedMethodParameter.scala +++ b/rules/src/main/scala/fix/UnusedMethodParameter.scala @@ -6,6 +6,7 @@ package fix import scalafix.lint.LintSeverity import scalafix.v1._ +import scala.annotation.nowarn import scala.collection.mutable import scala.meta._ @@ -21,73 +22,45 @@ class UnusedMethodParameter extends SemanticRule("UnusedMethodParameter") { override def fix(implicit doc: SemanticDocument): Patch = { - def isNothing(sym: Symbol) = { - val symbolMatcher = SymbolMatcher.exact("scala/Nothing#") - sym.info.get.signature match { - case MethodSignature(_, _, TypeRef(_, tpe, _)) => symbolMatcher.matches(tpe) // Scala 2 - case ValueSignature(TypeRef(_, symbol, _)) => symbolMatcher.matches(symbol) // Scala 3 - case _ => false - } + def normalize(term: Stat): List[Stat] = term match { + case Term.Block(stats) => stats + case _ => List(term) } + def getFields(stats: List[Stat]): Map[String, Position] = stats.collect { + case Defn.Var.After_4_7_2(_, List(Pat.Var(name)), _, _) => (name.value, name.pos) + case Defn.Val(_, List(Pat.Var(name)), _, _) => (name.value, name.pos) + }.toMap - def analyzeStats(stats: List[Stat], ctorNotVals: Seq[String], possiblyUnusedFields: List[String], positions: mutable.HashMap[String, Position]): Patch = { - stats.collect { - // Ignore main methods and overrides - case Defn.Def.After_4_6_0(_, Term.Name("main"), Some(Member.ParamClauseGroup(_, List(Term.ParamClause(List(Term.Param(_, Term.Name("args"), Some(Type.Apply.After_4_6_0(Type.Name("Array"), Type.ArgClause(List(Type.Name("String"))))), _)), _)))), _, _) => Patch.empty - // Looking at mods directly e.g. @main def hello = () or override def hello = () - case Defn.Def.After_4_6_0(mods, _, _, _, _) if mods.exists(m => m.toString == "@main" || m.toString == "override") => Patch.empty - // Some overridden methods are not marked as overridden but this is stored in symbol information - case d @ Defn.Def.After_4_6_0(_, _, _, _, _) if d.symbol.info.exists(_.overriddenSymbols.nonEmpty) => Patch.empty - - // Ignore nothing methods - case Defn.Def.After_4_6_0(_, _, _, Some(Type.Name("Nothing")), _) => Patch.empty // Methods declared returning Nothing - case t @ Defn.Def.After_4_6_0(_, _, _, _, _) if t.symbol.info.map(_.signature).exists { - case MethodSignature(_, _, TypeRef(_, tpe, _)) => SymbolMatcher.exact("scala/Nothing#").matches(tpe) // Methods inferred to return Nothing - case _ => false - } => Patch.empty - - // Any other method - case Defn.Def.After_4_6_0(_, _, Some(Member.ParamClauseGroup(_, List(Term.ParamClause(params, _)))), _, body) => - val paramNames = params.collect { - // Collect the parameter of the functions which are not marked as unused - case Term.Param(mods, p @ Term.Name(name), _, _) if !mods.exists(_.toString == "@unused") => positions.put(name, p.pos); name - } - val usedParams = body.collect { - // Collect any parameter that is used in the body - case p @ Term.Name(name) if paramNames.contains(name) => positions.put(name, p.pos); name - } + @nowarn + def unusedParams(stats: List[Stat], params: Set[String]): Set[String] = { - // Find constructor parameters that are not used in the body - (ctorNotVals.filterNot(usedParams.contains).map { e => - Patch.lint(diag(positions(e))) - } ++ paramNames.filterNot(usedParams.contains).map { e => // Find method parameters not used in the body - Patch.lint(diag(positions(e))) - } ++ possiblyUnusedFields.filterNot(usedParams.contains).map { e => // Find fields that are not used in the body - Patch.lint(diag(positions(e))) - }).asPatch - // The three are combined into a patch - - case _ => Patch.empty - }.asPatch - } + def processArgClause(args: List[Term], acc: Set[String]): Set[String] = args.foldLeft(acc) { + case (acc, e: Stat) => unusedParams(normalize(e), acc) + case (acc, _) => acc + } - def getPossiblyUnusedFields(stats: List[Stat], positions: mutable.HashMap[String, Position]) = stats.collect { - case v @ Defn.Var.After_4_7_2(_, List(Pat.Var(name)), _, _) if !isNothing(v.symbol) => positions.put(name.value, v.pos); name.value - case v @ Defn.Val(_, List(Pat.Var(name)), _, _) if !isNothing(v.symbol) => positions.put(name.value, v.pos); name.value + if (stats.isEmpty) params + else stats.foldLeft(params) { + case (acc, Term.Name(name)) if acc.contains(name) => acc - name + case (acc, others: Stat) => others.children.foldLeft(acc) { + case (acc, e: Stat) => unusedParams(normalize(e), acc) + // Argclause is not a stat, we recurse on arguments + case (acc, Term.ArgClause(args, _)) => processArgClause(args, acc) + case (acc, _) => acc + } + case (acc, _) => acc + } } doc.tree.collect { case Defn.Trait.After_4_6_0(_, _, _, _, _) => Patch.empty case Defn.Object(_, _, Template.After_4_4_0(_, _, _, stats, _)) => - val positions = mutable.HashMap[String, Position]() - val possiblyUnusedFields = getPossiblyUnusedFields(stats, positions) - analyzeStats(stats, Nil, possiblyUnusedFields, positions) // no constructor for objects + val fields = getFields(stats) + unusedParams(stats, fields.keySet).map(e => + Patch.lint(diag(fields(e))) + ).asPatch case Defn.Class.After_4_6_0(mods, _, _, ctor, Template.After_4_4_0(_, _, _, stats, _)) if !mods.exists(m => m.toString == "abstract") => - // Easier to collect positions in a separate HashMap than to collect them in the three later collections and combine them - val positions = mutable.HashMap[String, Position]() - val possiblyUnusedFields = getPossiblyUnusedFields(stats, positions) - /* * For constructor params, some params become vals / fields of the class (and should be ignored when unused): * 1. all params in the first argument list for case classes @@ -98,15 +71,44 @@ class UnusedMethodParameter extends SemanticRule("UnusedMethodParameter") { // Else we consider everything val consideredCtorVals = if (mods.exists(_.toString == "case")) ctor.paramClauses.drop(1) else ctor.paramClauses - val ctorNotVals = consideredCtorVals + val ctorNotVals: Map[String, Position] = consideredCtorVals .flatMap(_.values) // get the values of paramclauses - .collect { case Term.Param(mods, name, _, _) if !mods.exists(m => m.toString() == "var" || m.toString() == "val" || m.toString() == "@unused") => positions.put(name.value, name.pos); name.value } // get the names of the parameters, when they are not vals / vars or unused - - if (stats.isEmpty && ctorNotVals.nonEmpty) { // If we do not have stats (i.e. no class body), we should check constructor parameters nonetheless - ctorNotVals.map { e => - Patch.lint(diag(positions(e))) - }.asPatch - } else analyzeStats(stats, ctorNotVals, possiblyUnusedFields, positions) - }.asPatch - } + .collect { + case t @ Term.Param(mods, name, _, _) if !mods.exists(m => m.toString() == "var" || m.toString() == "val" || m.toString() == "@unused") => (name.value, t.pos) + }.toMap + val fields = getFields(stats) + unusedParams(stats, fields.keySet ++ ctorNotVals.keySet).map(e => + Patch.lint(diag(fields.getOrElse(e, ctorNotVals(e)))) + ).asPatch + + case Defn.Def.After_4_6_0(_, Term.Name("main"), Some(Member.ParamClauseGroup(_, List(Term.ParamClause(List(Term.Param(_, Term.Name("args"), Some(Type.Apply.After_4_6_0(Type.Name("Array"), Type.ArgClause(List(Type.Name("String"))))), _)), _)))), _, _) => Patch.empty + // Looking at mods directly e.g. @main def hello = () or override def hello = () + case Defn.Def.After_4_6_0(mods, _, _, _, _) if mods.exists(m => m.toString == "@main" || m.toString == "override") => Patch.empty + // Some overridden methods are not marked as overridden but this is stored in symbol information + case d @ Defn.Def.After_4_6_0(_, _, _, _, _) if d.symbol.info.exists(_.overriddenSymbols.nonEmpty) => Patch.empty + + // Ignore nothing methods + case Defn.Def.After_4_6_0(_, _, _, Some(Type.Name("Nothing")), _) => Patch.empty // Methods declared returning Nothing + case t @ Defn.Def.After_4_6_0(_, _, _, _, _) if t.symbol.info.map(_.signature).exists { + case MethodSignature(_, _, TypeRef(_, tpe, _)) => SymbolMatcher.exact("scala/Nothing#").matches(tpe) // Methods inferred to return Nothing + case _ => false + } => Patch.empty + + // Any other method + case Defn.Def.After_4_6_0(_, _, paramClause, _, body) => + // Method might not have params + val params = paramClause match { + case Some(Member.ParamClauseGroup(_, List(Term.ParamClause(paramsValue, _)))) => paramsValue + case _ => Nil + } + val paramNames = params.collect { + // Collect the parameter of the functions which are not marked as unused + case Term.Param(mods, p @ Term.Name(name), _, _) if !mods.exists(_.toString == "@unused") => (name, p.pos) + }.toMap + + unusedParams(normalize(body), paramNames.keySet).map(e => + Patch.lint(diag(paramNames(e))) + ).asPatch + } + }.asPatch }