Improve -Wunused: locals, privates with unset vars warning #16639 (#17160)

This PR is related to my Bachelor Semester Project, supervised by
@anatoliykmetyuk.

The latter consist in improving and implementing more Scala 3 linter
options (see #15503), with #16639 as a starting issue fixed in this PR.

- During the traversal in CheckUnused.scala (Miniphase & local
TreeTraverser), when reaching an `Assign` case, symbols are collected as
set, and then used to filter used locals and privates variable at
reporting time.
- Adapt test suit, and Add more test accordingly.
- Note that for a same variable the unused warning always has priority
and shadows the unset warning.

That feature follows the Scala 2 `-Ywarn-unused:<args>` behavior.

Author: szymon-rd

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

@@ -27,6 +27,7 @@ import dotty.tools.dotc.core.Symbols.Symbol
 import dotty.tools.dotc.core.StdNames.nme
 import scala.math.Ordering
 
+
 /**
  * A compiler phase that checks for unused imports or definitions
  *
@@ -146,6 +147,13 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
     if !tree.isInstanceOf[tpd.InferredTypeTree] then typeTraverser(unusedDataApply).traverse(tree.tpe)
     ctx
 
+  override def prepareForAssign(tree: tpd.Assign)(using Context): Context =
+    unusedDataApply{ ud =>
+      val sym = tree.lhs.symbol
+      if sym.exists then
+        ud.registerSetVar(sym)
+    }
+
   // ========== MiniPhase Transform ==========
 
   override def transformBlock(tree: tpd.Block)(using Context): tpd.Tree =
@@ -172,6 +180,7 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
     unusedDataApply(_.removeIgnoredUsage(tree.symbol))
     tree
 
+
   // ---------- MiniPhase HELPERS -----------
 
   private def pushInBlockTemplatePackageDef(tree: tpd.Block | tpd.Template | tpd.PackageDef)(using Context): Context =
@@ -215,11 +224,11 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
         case sel: Select =>
           prepareForSelect(sel)
           traverseChildren(tree)(using newCtx)
-        case _: (tpd.Block | tpd.Template | tpd.PackageDef) =>
+        case tree: (tpd.Block | tpd.Template | tpd.PackageDef) =>
           //! DIFFERS FROM MINIPHASE
-          unusedDataApply { ud =>
-            ud.inNewScope(ScopeType.fromTree(tree))(traverseChildren(tree)(using newCtx))
-          }
+          pushInBlockTemplatePackageDef(tree)
+          traverseChildren(tree)(using newCtx)
+          popOutBlockTemplatePackageDef()
         case t:tpd.ValDef =>
           prepareForValDef(t)
           traverseChildren(tree)(using newCtx)
@@ -235,6 +244,9 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
         case t: tpd.Bind =>
           prepareForBind(t)
           traverseChildren(tree)(using newCtx)
+        case t:tpd.Assign =>
+          prepareForAssign(t)
+          traverseChildren(tree)
         case _: tpd.InferredTypeTree =>
         case t@tpd.TypeTree() =>
           //! DIFFERS FROM MINIPHASE
@@ -278,6 +290,10 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
           report.warning(s"unused private member", t)
         case UnusedSymbol(t, _, WarnTypes.PatVars) =>
           report.warning(s"unused pattern variable", t)
+        case UnusedSymbol(t, _, WarnTypes.UnsetLocals) =>
+          report.warning(s"unset local variable", t)
+        case UnusedSymbol(t, _, WarnTypes.UnsetPrivates) =>
+          report.warning(s"unset private variable", t)
     }
 
 end CheckUnused
@@ -297,6 +313,8 @@ object CheckUnused:
     case ImplicitParams
     case PrivateMembers
     case PatVars
+    case UnsetLocals
+    case UnsetPrivates
 
   /**
    * The key used to retrieve the "unused entity" analysis metadata,
@@ -343,12 +361,8 @@ object CheckUnused:
     private val implicitParamInScope = MutSet[tpd.MemberDef]()
     private val patVarsInScope = MutSet[tpd.Bind]()
 
-    /* Unused collection collected at the end */
-    private val unusedLocalDef = MutSet[tpd.MemberDef]()
-    private val unusedPrivateDef = MutSet[tpd.MemberDef]()
-    private val unusedExplicitParams = MutSet[tpd.MemberDef]()
-    private val unusedImplicitParams = MutSet[tpd.MemberDef]()
-    private val unusedPatVars = MutSet[tpd.Bind]()
+    /** All variables sets*/
+    private val setVars = MutSet[Symbol]()
 
     /** All used symbols */
     private val usedDef = MutSet[Symbol]()
@@ -360,15 +374,6 @@ object CheckUnused:
 
     private val paramsToSkip = MutSet[Symbol]()
 
-    /**
-     * Push a new Scope of the given type, executes the given Unit and
-     * pop it back to the original type.
-     */
-    def inNewScope(newScope: ScopeType)(execInNewScope: => Unit)(using Context): Unit =
-      val prev = currScopeType
-      pushScope(newScope)
-      execInNewScope
-      popScope()
 
     def finishAggregation(using Context)(): Unit =
       val unusedInThisStage = this.getUnused
@@ -443,6 +448,9 @@ object CheckUnused:
       impInScope.push(MutSet())
       usedInScope.push(MutSet())
 
+    def registerSetVar(sym: Symbol): Unit =
+      setVars += sym
+
     /**
      * leave the current scope and do :
      *
@@ -501,15 +509,19 @@ object CheckUnused:
           unusedImport.map(d => UnusedSymbol(d.srcPos, d.name, WarnTypes.Imports)).toList
         else
           Nil
-      val sortedLocalDefs =
+      // Partition to extract unset local variables from usedLocalDefs
+      val (usedLocalDefs, unusedLocalDefs) =
         if ctx.settings.WunusedHas.locals then
-          localDefInScope
-            .filterNot(d => d.symbol.usedDefContains)
-            .filterNot(d => usedInPosition.exists { case (pos, name) => d.span.contains(pos.span) && name == d.symbol.name})
-            .filterNot(d => containsSyntheticSuffix(d.symbol))
-            .map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.LocalDefs)).toList
+          localDefInScope.partition(d => d.symbol.usedDefContains)
         else
-          Nil
+          (Nil, Nil)
+      val sortedLocalDefs =
+        unusedLocalDefs
+          .filterNot(d => usedInPosition.exists { case (pos, name) => d.span.contains(pos.span) && name == d.symbol.name})
+          .filterNot(d => containsSyntheticSuffix(d.symbol))
+          .map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.LocalDefs)).toList
+      val unsetLocalDefs = usedLocalDefs.filter(isUnsetVarDef).map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.UnsetLocals)).toList
+
       val sortedExplicitParams =
         if ctx.settings.WunusedHas.explicits then
           explicitParamInScope
@@ -527,14 +539,14 @@ object CheckUnused:
             .map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.ImplicitParams)).toList
         else
           Nil
-      val sortedPrivateDefs =
+      // Partition to extract unset private variables from usedPrivates
+      val (usedPrivates, unusedPrivates) =
         if ctx.settings.WunusedHas.privates then
-          privateDefInScope
-            .filterNot(d => d.symbol.usedDefContains)
-            .filterNot(d => containsSyntheticSuffix(d.symbol))
-            .map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.PrivateMembers)).toList
+          privateDefInScope.partition(d => d.symbol.usedDefContains)
         else
-          Nil
+          (Nil, Nil)
+      val sortedPrivateDefs = unusedPrivates.filterNot(d => containsSyntheticSuffix(d.symbol)).map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.PrivateMembers)).toList
+      val unsetPrivateDefs = usedPrivates.filter(isUnsetVarDef).map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.UnsetPrivates)).toList
       val sortedPatVars =
         if ctx.settings.WunusedHas.patvars then
           patVarsInScope
@@ -544,7 +556,9 @@ object CheckUnused:
             .map(d => UnusedSymbol(d.namePos, d.name, WarnTypes.PatVars)).toList
         else
           Nil
-      val warnings = List(sortedImp, sortedLocalDefs, sortedExplicitParams, sortedImplicitParams, sortedPrivateDefs, sortedPatVars).flatten.sortBy { s =>
+      val warnings =
+        List(sortedImp, sortedLocalDefs, sortedExplicitParams, sortedImplicitParams,
+                  sortedPrivateDefs, sortedPatVars, unsetLocalDefs, unsetPrivateDefs).flatten.sortBy { s =>
         val pos = s.pos.sourcePos
         (pos.line, pos.column)
       }
@@ -731,10 +745,13 @@ object CheckUnused:
         !isSyntheticMainParam(sym) &&
         !sym.shouldNotReportParamOwner
 
-
       private def shouldReportPrivateDef(using Context): Boolean =
         currScopeType.top == ScopeType.Template && !memDef.symbol.isConstructor && memDef.symbol.is(Private, butNot = SelfName | Synthetic | CaseAccessor)
 
+      private def isUnsetVarDef(using Context): Boolean =
+        val sym = memDef.symbol
+        sym.is(Mutable) && !setVars(sym)
+
     extension (imp: tpd.Import)
       /** Enum generate an import for its cases (but outside them), which should be ignored */
       def isGeneratedByEnum(using Context): Boolean =

tests/neg-custom-args/fatal-warnings/i15503b.scala

@@ -2,49 +2,91 @@
 
 val a = 1 // OK
 
+var cs = 3 // OK
+
 val b = // OK
+  var e3 = 2 // error
   val e1 = 1 // error
   def e2 = 2 // error
   1
 
 val c = // OK
-  val e1 = 1 // OK
+  var e1 = 1 // error not set
+  def e2 = e1 // OK
+  val e3 = e2 // OK
+    e3
+
+val g = // OK
+  var e1 = 1 // OK
   def e2 = e1 // OK
-    e2
+  val e3 = e2 // OK
+  e1 = e3 // OK
+    e3
 
 def d = 1 // OK
 
 def e = // OK
   val e1 = 1 // error
   def e2 = 2 // error
+  var e3 = 4 // error
   1
 
 def f = // OK
   val f1 = 1 // OK
-  def f2 = f1 // OK
+  var f2 = f1 // error not set
+  def f3 = f2 // OK
+  f3
+
+def h = // OK
+  val f1 = 1 // OK
+  var f2 = f1 // OK
+  def f3 = f2 // OK
+  f2 = f3 // OK
   f2
 
 class Foo {
+  val a = 1 // OK
+
+  var cs = 3 // OK
+
   val b = // OK
+    var e3 = 2 // error
     val e1 = 1 // error
     def e2 = 2 // error
     1
 
   val c = // OK
-    val e1 = 1 // OK
+    var e1 = 1 // error not set
+    def e2 = e1 // OK
+    val e3 = e2 // OK
+    e3
+
+  val g = // OK
+    var e1 = 1 // OK
     def e2 = e1 // OK
-      e2
+    val e3 = e2 // OK
+    e1 = e3 // OK
+    e3
 
   def d = 1 // OK
 
   def e = // OK
     val e1 = 1 // error
     def e2 = 2 // error
+    var e3 = 4 // error
     1
 
   def f = // OK
     val f1 = 1 // OK
-    def f2 = f1 // OK
+    var f2 = f1 // error not set
+    def f3 = f2 // OK
+    f3
+
+  def h = // OK
+    val f1 = 1 // OK
+    var f2 = f1 // OK
+    def f3 = f2 // OK
+    f2 = f3 // OK
     f2
 }
 
@@ -68,7 +110,7 @@ package foo.scala2.tests:
       new a.Inner
     }
     def f2 = {
-      var x = 100
+      var x = 100 // error not set
       x
     }
   }
@@ -89,7 +131,7 @@ package foo.scala2.tests:
   }
 
 package test.foo.twisted.i16682:
-  def myPackage = 
+  def myPackage =
     object IntExtractor: // OK
       def unapply(s: String): Option[Int] = s.toIntOption
 

tests/neg-custom-args/fatal-warnings/i15503c.scala

@@ -12,20 +12,37 @@ class A:
     private[this] val f = e // OK
     private val g = f // OK
 
+    private[A] var h = 1 // OK
+    private[this] var i = h // error not set
+    private var j = i // error not set
+
+    private[this] var k = 1 // OK
+    private var l = 2 // OK
+    private val m = // error
+      k = l
+      l = k
+      l
+
     private def fac(x: Int): Int = // error
       if x == 0 then 1 else x * fac(x - 1)
 
     val x = 1 // OK
     def y = 2 // OK
     def z = g // OK
+    var w = 2 // OK
 
 package foo.test.contructors:
   case class A private (x:Int) // OK
   class B private (val x: Int) // OK
   class C private (private val x: Int) // error
   class D private (private val x: Int): // OK
     def y = x
-
+  class E private (private var x: Int): // error not set
+    def y = x
+  class F private (private var x: Int): // OK
+    def y =
+      x = 3
+      x
 
 package test.foo.i16682:
   object myPackage:

tests/neg-custom-args/fatal-warnings/i15503i.scala

@@ -142,8 +142,8 @@ package foo.test.possibleclasses.withvar:
     private var y: Int // OK
   )(
     s: Int, // OK
-    var t: Int, // OK
-    private var z: Int // OK
+    var t: Int, // OK global scope can be set somewhere else
+    private var z: Int // error not set
   ) {
     def a = k + y + s + t + z
   }
@@ -159,11 +159,11 @@ package foo.test.possibleclasses.withvar:
 
   class AllUsed(
     k: Int, // OK
-    private var y: Int // OK
+    private var y: Int // error not set
   )(
     s: Int, // OK
-    var t: Int, // OK
-    private var z: Int // OK
+    var t: Int, // OK global scope can be set somewhere else
+    private var z: Int // error not set
   ) {
     def a = k + y + s + t + z
   }