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
   }

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

@@ -0,0 +1,207 @@
+// scalac: -Wunused:all
+//
+class Bippy(a: Int, b: Int) {
+  private def this(c: Int) = this(c, c)           // warn /Dotty:NoWarn
+  private def boop(x: Int)            = x+a+b     // error
+    private def bippy(x: Int): Int      = bippy(x)  // error TODO: could warn
+  final private val MILLIS1           = 2000      // error no warn, /Dotty:Warn
+  final private val MILLIS2: Int      = 1000      // error
+  final private val HI_COMPANION: Int = 500       // no warn, accessed from companion
+  def hi() = Bippy.HI_INSTANCE
+}
+object Bippy {
+  def hi(x: Bippy) = x.HI_COMPANION
+  private val HI_INSTANCE: Int = 500      // no warn, accessed from instance
+  private val HEY_INSTANCE: Int = 1000    // error warn
+  private lazy val BOOL: Boolean = true   // error warn
+}
+
+class A(val msg: String)
+class B1(msg: String) extends A(msg)
+class B2(msg0: String) extends A(msg0)
+class B3(msg0: String) extends A("msg") // error /Dotty: unused explicit parameter
+
+trait Bing
+
+trait Accessors {
+  private var v1: Int = 0 // error warn
+  private var v2: Int = 0 // error warn, never set
+  private var v3: Int = 0 // warn, never got /Dotty: no warn even if not used
+  private var v4: Int = 0 // no warn
+
+  private[this] var v5 = 0 // error warn, never set
+  private[this] var v6 = 0 // warn, never got /Dotty: no warn even if not used
+  private[this] var v7 = 0 // no warn
+
+  def bippy(): Int = {
+    v3 = 3
+    v4 = 4
+    v6 = 6
+    v7 = 7
+    v2 + v4 + v5 + v7
+  }
+}
+
+class StableAccessors {
+  private var s1: Int = 0 // error warn
+  private var s2: Int = 0 // error warn, never set
+  private var s3: Int = 0 // warn, never got /Dotty: no warn even if not usued
+  private var s4: Int = 0 // no warn
+
+  private[this] var s5 = 0 // error warn, never set
+  private[this] var s6 = 0 // no warn, limitation /Dotty: Why limitation ?
+  private[this] var s7 = 0 // no warn
+
+  def bippy(): Int = {
+    s3 = 3
+    s4 = 4
+    s6 = 6
+    s7 = 7
+    s2 + s4 + s5 + s7
+  }
+}
+
+trait DefaultArgs {
+  // warn about default getters for x2 and x3
+  private def bippy(x1: Int, x2: Int = 10, x3: Int = 15): Int = x1 + x2 + x3 // no more warn warn since #17061
+
+  def boppy() = bippy(5, 100, 200)
+}
+
+
+class Outer {
+  class Inner
+}
+
+trait Locals {
+  def f0 = {
+    var x = 1 // error warn
+    var y = 2
+    y = 3
+    y + y
+  }
+  def f1 = {
+    val a = new Outer // no warn
+    val b = new Outer // error warn
+    new a.Inner
+  }
+  def f2 = {
+    var x = 100 // error warn about it being a var, var not set
+    x
+  }
+}
+
+object Types {
+  private object Dongo { def f = this } // no more warn since #17061
+  private class Bar1 // error warn
+  private class Bar2 // no warn
+  private type Alias1 = String // error warn
+  private type Alias2 = String // no warn
+  def bippo = (new Bar2).toString
+
+  def f(x: Alias2) = x.length
+
+  def l1() = {
+    object HiObject { def f = this } // no more warn since #17061
+    class Hi { // error warn
+      def f1: Hi = new Hi
+      def f2(x: Hi) = x
+    }
+    class DingDongDoobie // error warn
+    class Bippy // no warn
+    type Something = Bippy // no warn
+    type OtherThing = String // error warn
+    (new Bippy): Something
+  }
+}
+
+trait Underwarn {
+  def f(): Seq[Int]
+
+  def g() = {
+    val Seq(_, _) = f()  // no warn
+    true
+  }
+}
+
+class OtherNames {
+  private def x_=(i: Int): Unit = () // no more warn since #17061
+  private def x: Int = 42 // error Dotty triggers unused private member : To investigate
+  private def y_=(i: Int): Unit = () // // no more warn since #17061
+  private def y: Int = 42
+
+  def f = y
+}
+
+
+trait Forever {
+  def f = {
+    val t = Option((17, 42))
+    for {
+      ns <- t
+      (i, j) = ns                        // no warn
+    } yield (i + j)
+  }
+  def g = {
+    val t = Option((17, 42))
+    for {
+      ns <- t
+      (i, j) = ns                        // no warn
+    } yield 42                           // val emitted only if needed, hence nothing unused
+  }
+}
+
+trait Ignorance {
+  private val readResolve = 42      // error ignore /dotty triggers unused private member/ why should we ignore ?
+}
+
+trait CaseyKasem {
+  def f = 42 match {
+    case x if x < 25 => "no warn"
+    case y if toString.nonEmpty => "no warn" + y
+    case z => "warn"
+  }
+}
+trait CaseyAtTheBat {
+  def f = Option(42) match {
+    case Some(x) if x < 25 => "no warn"
+    case Some(y @ _) if toString.nonEmpty => "no warn"
+    case Some(z) => "warn"
+    case None => "no warn"
+  }
+}
+
+class `not even using companion privates`
+
+object `not even using companion privates` {
+  private implicit class `for your eyes only`(i: Int) {  // no more warn since #17061
+    def f = i
+  }
+}
+
+class `no warn in patmat anonfun isDefinedAt` {
+  def f(pf: PartialFunction[String, Int]) = pf("42")
+  def g = f {
+    case s => s.length        // no warn (used to warn case s => true in isDefinedAt)
+  }
+}
+
+// this is the ordinary case, as AnyRef is an alias of Object
+class `nonprivate alias is enclosing` {
+  class C
+  type C2 = C
+  private class D extends C2   // error warn
+}
+
+object `classof something` {
+  private class intrinsically
+  def f = classOf[intrinsically].toString()
+}
+
+trait `short comings` {
+  def f: Int = {
+    val x = 42 // error /Dotty only triggers in dotty
+    17
+  }
+}
+

tests/pos/i16639false-pos-on-trait.scala

@@ -0,0 +1,40 @@
+// scalac -Wunsued:all
+//Avoid warning on setter in trait Regression test : issue10154 scala
+
+trait T {
+  private var x: String = _
+
+  def y: String = {
+    if (x eq null) x = "hello, world"
+    x
+  }
+}
+
+/*
+➜  skalac -version
+Scala compiler version 2.13.10-20220920-001308-98972e5 -- Copyright 2002-2022, LAMP/EPFL and Lightbend, Inc.
+
+➜  skalac -d /tmp -Wunused -Vprint:typer t12646.scala
+t12646.scala:3: warning: parameter value x_= in variable x is never used
+  private var x: String = _
+              ^
+[[syntax trees at end of                     typer]] // t12646.scala
+package <empty> {
+  abstract trait T extends scala.AnyRef {
+    def /*T*/$init$(): Unit = {
+      ()
+    };
+    <accessor> private val x: String = _;
+    <accessor> private def x_=(x$1: String): Unit;
+    def y: String = {
+      if (T.this.x.eq(null))
+        T.this.x_=("hello, world")
+      else
+        ();
+      T.this.x
+    }
+  }
+}
+
+1 warning
+*/