From 01ad6837f7cb53512fc09d8bf35e128f423d3dfc Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sat, 6 Dec 2025 11:09:44 -0800 Subject: [PATCH 1/2] Simplify handling of Assign in unused check --- .../tools/dotc/transform/CheckUnused.scala | 38 +++++++++---------- tests/warn/i24280.scala | 18 +++++++++ 2 files changed, 35 insertions(+), 21 deletions(-) create mode 100644 tests/warn/i24280.scala diff --git a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala index b48afd24a121..80a7cd3256a4 100644 --- a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala +++ b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala @@ -51,7 +51,6 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha tree override def transformIdent(tree: Ident)(using Context): tree.type = - refInfos.isAssignment = tree.hasAttachment(AssignmentTarget) if tree.symbol.exists then // if in an inline expansion, resolve at summonInline (synthetic pos) or in an enclosing call site val resolving = @@ -67,12 +66,10 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha resolveUsage(tree.symbol, tree.name, tree.typeOpt.importPrefix.skipPackageObject, imports = resolving) else if tree.hasType then resolveUsage(tree.tpe.classSymbol, tree.name, tree.tpe.importPrefix.skipPackageObject) - refInfos.isAssignment = false tree // import x.y; y may be rewritten x.y, also import x.z as y override def transformSelect(tree: Select)(using Context): tree.type = - refInfos.isAssignment = tree.hasAttachment(AssignmentTarget) val name = tree.removeAttachment(OriginalName).getOrElse(nme.NO_NAME) inline def isImportable = tree.qualifier.srcPos.isSynthetic && tree.qualifier.tpe.match @@ -96,7 +93,6 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha resolveUsage(sym, name, tree.qualifier.tpe) else if !ignoreTree(tree) then refUsage(sym) - refInfos.isAssignment = false tree override def transformLiteral(tree: Literal)(using Context): tree.type = @@ -132,10 +128,11 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha tree override def prepareForAssign(tree: Assign)(using Context): Context = - tree.lhs.putAttachment(AssignmentTarget, ()) // don't take LHS reference as a read + if tree.lhs.symbol.exists then + refInfos.setAssignmentTarget(tree.lhs.symbol) ctx override def transformAssign(tree: Assign)(using Context): tree.type = - tree.lhs.removeAttachment(AssignmentTarget) + refInfos.resetAssignmentTarget() tree override def prepareForMatch(tree: Match)(using Context): Context = @@ -310,13 +307,15 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha for annot <- sym.denot.annotations do transformAllDeep(annot.tree) - /** If sym is not an enclosing element with respect to the give context, record the reference + /** If sym is not an enclosing element with respect to the given context, record the reference * * Also check that every enclosing element is not a synthetic member * of the sym's case class companion module. + * + * The LHS of a current Assign is never recorded as a reference (that is, a usage). */ def refUsage(sym: Symbol)(using Context): Unit = - if !refInfos.hasRef(sym) then + if !refInfos.hasRef(sym) && !refInfos.isAssignmentTarget(sym) then val isCase = sym.is(Case) && sym.isClass if !ctx.outersIterator.exists: outer => val owner = outer.owner @@ -505,9 +504,6 @@ object CheckUnused: /** Ignore reference. */ val Ignore = Property.StickyKey[Unit] - /** Tree is LHS of Assign. */ - val AssignmentTarget = Property.StickyKey[Unit] - /** Tree is an inlined parameter. */ val InlinedParameter = Property.StickyKey[Unit] @@ -559,18 +555,18 @@ object CheckUnused: var inliners = 0 // depth of inline def (not inlined yet) - // instead of refs.addOne, use refUsage -> addRef to distinguish a read from a write to var - var isAssignment = false + private var assignmentTarget: Symbol = NoSymbol + def isAssignmentTarget(sym: Symbol): Boolean = sym eq assignmentTarget + def resetAssignmentTarget(): Unit = + assignmentTarget = NoSymbol + def setAssignmentTarget(sym: Symbol): Unit = + assignmentTarget = sym + asss.addOne(sym) + // @pre !isAssignmentTarget(sym), see refUsage def addRef(sym: Symbol): Unit = - if isAssignment then - asss.addOne(sym) - else - refs.addOne(sym) + refs.addOne(sym) def hasRef(sym: Symbol): Boolean = - if isAssignment then - asss(sym) - else - refs(sym) + refs(sym) // currently compiletime.testing is completely erased, so ignore the unit var isNullified = false diff --git a/tests/warn/i24280.scala b/tests/warn/i24280.scala new file mode 100644 index 000000000000..ecc3b750d52e --- /dev/null +++ b/tests/warn/i24280.scala @@ -0,0 +1,18 @@ +//> using options -Wunused:all + +class Foo { + def foo(): Any = { + var i = 0 // warn mutated but not read + val f = () => i += 1 + f + } + def bar(): Any = { + var i = 0 // warn + val g = () => i = i + 1 + g + } + object Select: + private var i = 0 // warn + class Select: + def test = Select.i += 1 +} From c38137413e913f9067ff16d18b079a47cacff77c Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sat, 6 Dec 2025 14:29:57 -0800 Subject: [PATCH 2/2] Make assign check a context check At Assign, context receives the tree so that variable lookup detects assignment by target symbol and by position. --- .../tools/dotc/transform/CheckUnused.scala | 72 +++++++++---------- tests/warn/i23704.check | 4 ++ tests/warn/i23704.scala | 2 +- tests/warn/i24280.scala | 14 ++++ 4 files changed, 54 insertions(+), 38 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala index 80a7cd3256a4..eb308b032348 100644 --- a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala +++ b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala @@ -53,19 +53,19 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha override def transformIdent(tree: Ident)(using Context): tree.type = if tree.symbol.exists then // if in an inline expansion, resolve at summonInline (synthetic pos) or in an enclosing call site - val resolving = + val resolvingImports = tree.srcPos.isUserCode(using if tree.hasAttachment(InlinedParameter) then ctx.outer else ctx) || tree.srcPos.isZeroExtentSynthetic // take as summonInline if !ignoreTree(tree) then def loopOverPrefixes(prefix: Type, depth: Int): Unit = if depth < 10 && prefix.exists && !prefix.classSymbol.isEffectiveRoot then - resolveUsage(prefix.classSymbol, nme.NO_NAME, NoPrefix, imports = resolving) + resolveUsage(prefix.classSymbol, nme.NO_NAME, NoPrefix, tree.srcPos, resolvingImports) loopOverPrefixes(prefix.normalizedPrefix, depth + 1) if tree.srcPos.isZeroExtentSynthetic then loopOverPrefixes(tree.typeOpt.normalizedPrefix, depth = 0) - resolveUsage(tree.symbol, tree.name, tree.typeOpt.importPrefix.skipPackageObject, imports = resolving) + resolveUsage(tree.symbol, tree.name, tree.typeOpt.importPrefix.skipPackageObject, tree.srcPos, resolvingImports) else if tree.hasType then - resolveUsage(tree.tpe.classSymbol, tree.name, tree.tpe.importPrefix.skipPackageObject) + resolveUsage(tree.tpe.classSymbol, tree.name, tree.tpe.importPrefix.skipPackageObject, tree.srcPos) tree // import x.y; y may be rewritten x.y, also import x.z as y @@ -86,13 +86,13 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha else if tycon.typeSymbol == defn.TypeableType then args(0) // T in Typeable[T] else return tree val target = res.dealias.typeSymbol - resolveUsage(target, target.name, res.importPrefix.skipPackageObject) // case _: T => + resolveUsage(target, target.name, res.importPrefix.skipPackageObject, tree.srcPos) // case _: T => case _ => else if isImportable || name.exists(_ != sym.name) then if !ignoreTree(tree) then - resolveUsage(sym, name, tree.qualifier.tpe) + resolveUsage(sym, name, tree.qualifier.tpe, tree.srcPos) else if !ignoreTree(tree) then - refUsage(sym) + refUsage(sym, tree.srcPos) tree override def transformLiteral(tree: Literal)(using Context): tree.type = @@ -117,23 +117,21 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha tree match case Apply(Select(left, nme.Equals | nme.NotEquals), right :: Nil) => val caneq = defn.CanEqualClass.typeRef.appliedTo(left.tpe.widen :: right.tpe.widen :: Nil) - resolveScoped(caneq) + resolveScoped(caneq, tree.srcPos) case tree => - refUsage(tree.tpe.typeSymbol) + refUsage(tree.tpe.typeSymbol, tree.srcPos) tree override def transformTypeApply(tree: TypeApply)(using Context): tree.type = if tree.symbol.exists && tree.symbol.isConstructor then - refUsage(tree.symbol.owner) // redundant with use of resultType in transformSelect of fun + refUsage(tree.symbol.owner, tree.srcPos) // redundant with use of resultType in transformSelect of fun tree override def prepareForAssign(tree: Assign)(using Context): Context = if tree.lhs.symbol.exists then - refInfos.setAssignmentTarget(tree.lhs.symbol) - ctx - override def transformAssign(tree: Assign)(using Context): tree.type = - refInfos.resetAssignmentTarget() - tree + refInfos.addAssignmentTarget(tree.lhs.symbol) + ctx.fresh.setTree(tree) + else ctx override def prepareForMatch(tree: Match)(using Context): Context = // allow case.pat against tree.selector (simple var pat only for now) @@ -144,13 +142,14 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha override def transformMatch(tree: Match)(using Context): tree.type = if tree.isInstanceOf[InlineMatch] && tree.selector.isEmpty then val sf = defn.Compiletime_summonFrom - resolveUsage(sf, sf.name, NoPrefix) + resolveUsage(sf, sf.name, NoPrefix, tree.srcPos) tree override def transformTypeTree(tree: TypeTree)(using Context): tree.type = tree.tpe match case AnnotatedType(_, annot) => transformAllDeep(annot.tree) - case tpt if !tree.isInferred && tpt.typeSymbol.exists => resolveUsage(tpt.typeSymbol, tpt.typeSymbol.name, NoPrefix) + case tpt if !tree.isInferred && tpt.typeSymbol.exists => + resolveUsage(tpt.typeSymbol, tpt.typeSymbol.name, NoPrefix, tree.srcPos) case _ => tree @@ -191,12 +190,14 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha traverseAnnotations(tree.symbol) if tree.name.startsWith("derived$") && tree.hasType then def loop(t: Tree): Unit = t match - case Ident(name) => resolveUsage(t.tpe.typeSymbol, name, t.tpe.underlyingPrefix.skipPackageObject) - case Select(t, _) => loop(t) - case _ => + case Ident(name) => + resolveUsage(t.tpe.typeSymbol, name, t.tpe.underlyingPrefix.skipPackageObject, tree.srcPos) + case Select(t, _) => + loop(t) + case _ => tree.getAttachment(OriginalTypeClass).foreach(loop) if tree.symbol.isAllOf(DeferredGivenFlags) then - resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix) + resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix, tree.srcPos) tree override def prepareForDefDef(tree: DefDef)(using Context): Context = @@ -216,7 +217,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha if tree.symbol.is(Inline) then refInfos.inliners -= 1 if tree.symbol.isAllOf(DeferredGivenFlags) then - resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix) + resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix, tree.srcPos) tree override def transformTypeDef(tree: TypeDef)(using Context): tree.type = @@ -282,7 +283,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha def resolve(tpe: Type): Unit = val sym = tpe.typeSymbol if sym.exists then - resolveUsage(sym, sym.name, NoPrefix) + resolveUsage(sym, sym.name, NoPrefix, tree.srcPos) resolve(lo) resolve(hi) case _ => @@ -314,8 +315,8 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha * * The LHS of a current Assign is never recorded as a reference (that is, a usage). */ - def refUsage(sym: Symbol)(using Context): Unit = - if !refInfos.hasRef(sym) && !refInfos.isAssignmentTarget(sym) then + def refUsage(sym: Symbol, pos: SrcPos)(using Context): Unit = + if !refInfos.hasRef(sym) then val isCase = sym.is(Case) && sym.isClass if !ctx.outersIterator.exists: outer => val owner = outer.owner @@ -324,6 +325,9 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha && owner.exists && owner.is(Synthetic) && owner.owner.eq(sym.companionModule.moduleClass) + || outer.tree.match + case Assign(lhs, _) => lhs.symbol.eq(sym) && outer.tree.srcPos.sourcePos.contains(pos.sourcePos) + case _ => false then refInfos.addRef(sym) @@ -338,7 +342,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha * The `imports` flag is whether an identifier can mark an import as used: the flag is false * for inlined code, except for `summonInline` (and related constructs) which are resolved at inlining. */ - def resolveUsage(sym0: Symbol, name: Name, prefix: Type, imports: Boolean = true)(using Context): Unit = + def resolveUsage(sym0: Symbol, name: Name, prefix: Type, pos: SrcPos, imports: Boolean = true)(using Context): Unit = import PrecedenceLevels.* val sym = sym0.userSymbol @@ -441,7 +445,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha end while // record usage and possibly an import if !enclosed then - refUsage(sym) + refUsage(sym, pos) if imports && candidate != NoContext && candidate.isImportContext && importer != null then refInfos.sels.put(importer, ()) end resolveUsage @@ -449,7 +453,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha /** Simulate implicit search for contextual implicits in lexical scope and mark any definitions or imports as used. * Avoid cached ctx.implicits because it needs the precise import context that introduces the given. */ - def resolveScoped(tp: Type)(using Context): Unit = + def resolveScoped(tp: Type, pos: SrcPos)(using Context): Unit = var done = false val ctxs = ctx.outersIterator while !done && ctxs.hasNext do @@ -461,7 +465,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha else Nil implicitRefs.find(ref => ref.underlyingRef.widen <:< tp) match case Some(found: TermRef) => - refUsage(found.denot.symbol) + refUsage(found.denot.symbol, pos) if cur.isImportContext then cur.importInfo.nn.selectors.find(sel => sel.isGiven || sel.rename == found.name) match case Some(sel) => @@ -469,7 +473,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha case _ => return case Some(found: RenamedImplicitRef) if cur.isImportContext => - refUsage(found.underlyingRef.denot.symbol) + refUsage(found.underlyingRef.denot.symbol, pos) cur.importInfo.nn.selectors.find(sel => sel.rename == found.implicitName) match case Some(sel) => refInfos.sels.put(sel, ()) @@ -555,14 +559,8 @@ object CheckUnused: var inliners = 0 // depth of inline def (not inlined yet) - private var assignmentTarget: Symbol = NoSymbol - def isAssignmentTarget(sym: Symbol): Boolean = sym eq assignmentTarget - def resetAssignmentTarget(): Unit = - assignmentTarget = NoSymbol - def setAssignmentTarget(sym: Symbol): Unit = - assignmentTarget = sym + def addAssignmentTarget(sym: Symbol): Unit = asss.addOne(sym) - // @pre !isAssignmentTarget(sym), see refUsage def addRef(sym: Symbol): Unit = refs.addOne(sym) def hasRef(sym: Symbol): Boolean = diff --git a/tests/warn/i23704.check b/tests/warn/i23704.check index 71a69aefef11..76c7c82f6071 100644 --- a/tests/warn/i23704.check +++ b/tests/warn/i23704.check @@ -6,6 +6,10 @@ 16 | private var myvar: Int = 0 // warn for the same case with simpler syntax | ^^^^^ | private variable was mutated but not read +-- [E198] Unused Symbol Warning: tests/warn/i23704.scala:22:14 --------------------------------------------------------- +22 | private var myvar: Int = 0 // warn (because read is in RHS of assignment; see incr) + | ^^^^^ + | private variable was mutated but not read -- [E198] Unused Symbol Warning: tests/warn/i23704.scala:26:8 ---------------------------------------------------------- 26 | var localvar = 0 // warn local variable was mutated but not read | ^^^^^^^^ diff --git a/tests/warn/i23704.scala b/tests/warn/i23704.scala index 9cfaae3278c1..6a6e2d36b703 100644 --- a/tests/warn/i23704.scala +++ b/tests/warn/i23704.scala @@ -19,7 +19,7 @@ class C: 27 class D: - private var myvar: Int = 0 // nowarn (although read is RHS of assignment) + private var myvar: Int = 0 // warn (because read is in RHS of assignment; see incr) def incr(): Unit = myvar = myvar + 1 def local(): Unit = diff --git a/tests/warn/i24280.scala b/tests/warn/i24280.scala index ecc3b750d52e..f82f6b610081 100644 --- a/tests/warn/i24280.scala +++ b/tests/warn/i24280.scala @@ -15,4 +15,18 @@ class Foo { private var i = 0 // warn class Select: def test = Select.i += 1 + + def nested(): Any = + var i = 0 // warn, read of i is in RHS of assign to i + i = + var j = 0 // nowarn, j is assigned to and read + j = i + 1 + j + + def escaped(): Any = + var i = 0 // warn, read of i is in RHS of assign to i, but should nowarn because assigned to j + var j = 0 // nowarn, j is assigned to and read + i = + j = i + 1 + j }