From 4479b5d9f432c1379e0a33a45f7e7a9b03d7cdae Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Thu, 16 Jan 2025 20:39:42 -0800 Subject: [PATCH] Tweaks per review --- .../dotty/tools/dotc/core/Definitions.scala | 2 + .../tools/dotc/transform/CheckUnused.scala | 164 +++++++++--------- tests/warn/i15503f.scala | 7 +- tests/warn/i15503kb/power.scala | 1 - tests/warn/i15503kb/square.scala | 3 +- 5 files changed, 86 insertions(+), 91 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index b89c6f01e88d..61697e6baee8 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -836,6 +836,7 @@ class Definitions { @tu lazy val QuotedExprClass: ClassSymbol = requiredClass("scala.quoted.Expr") @tu lazy val QuotesClass: ClassSymbol = requiredClass("scala.quoted.Quotes") + @tu lazy val Quotes_reflectModule: Symbol = QuotesClass.requiredClass("reflectModule") @tu lazy val Quotes_reflect: Symbol = QuotesClass.requiredValue("reflect") @tu lazy val Quotes_reflect_asTerm: Symbol = Quotes_reflect.requiredMethod("asTerm") @tu lazy val Quotes_reflect_Apply: Symbol = Quotes_reflect.requiredValue("Apply") @@ -957,6 +958,7 @@ class Definitions { def NonEmptyTupleClass(using Context): ClassSymbol = NonEmptyTupleTypeRef.symbol.asClass lazy val NonEmptyTuple_tail: Symbol = NonEmptyTupleClass.requiredMethod("tail") @tu lazy val PairClass: ClassSymbol = requiredClass("scala.*:") + @tu lazy val PairClass_unapply: Symbol = PairClass.companionModule.requiredMethod("unapply") @tu lazy val TupleXXLClass: ClassSymbol = requiredClass("scala.runtime.TupleXXL") def TupleXXLModule(using Context): Symbol = TupleXXLClass.companionModule diff --git a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala index 7d7f1c535d92..8cd2a8488b8e 100644 --- a/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala +++ b/compiler/src/dotty/tools/dotc/transform/CheckUnused.scala @@ -52,7 +52,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha override def transformIdent(tree: Ident)(using Context): tree.type = if tree.symbol.exists then - // resolve if inlined at the position of the call, or is zero extent summon + // if in an inline expansion, resolve at summonInline (synthetic pos) or in an enclosing call site val resolving = refInfos.inlined.isEmpty || tree.srcPos.isZeroExtentSynthetic @@ -111,7 +111,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha override def prepareForMatch(tree: Match)(using Context): Context = // exonerate case.pat against tree.selector (simple var pat only for now) tree.selector match - case Ident(nm) => tree.cases.foreach(k => absolveVariableBindings(List(nm), List(k.pat))) + case Ident(nm) => tree.cases.foreach(k => allowVariableBindings(List(nm), List(k.pat))) case _ => ctx override def transformMatch(tree: Match)(using Context): tree.type = @@ -244,24 +244,27 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha cases.foreach(transformAllDeep) case ByNameTypeTree(result) => transformAllDeep(result) - case _: InferredTypeTree => // do nothing - case _: Export => // nothing to do - case _ if tree.isType => - //println(s"OTHER TYPE ${tree.getClass} ${tree.show}") + //case _: InferredTypeTree => // do nothing + //case _: Export => // nothing to do + //case _ if tree.isType => case _ => - //println(s"OTHER ${tree.getClass} ${tree.show}") tree private def traverseAnnotations(sym: Symbol)(using Context): Unit = for annot <- sym.denot.annotations do transformAllDeep(annot.tree) + // if sym is not an enclosing element, record the reference + def refUsage(sym: Symbol)(using Context): Unit = + if !ctx.outersIterator.exists(cur => cur.owner eq sym) then + refInfos.refs.addOne(sym) + /** Look up a reference in enclosing contexts to determine whether it was introduced by a definition or import. * The binding of highest precedence must be correct. */ def resolveUsage(sym: Symbol, name: Name, prefix: Type)(using Context): Unit = def matchingSelector(info: ImportInfo): ImportSelector | Null = - val qtpe = info.site //info.qualifier.tpe.nn + val qtpe = info.site def loop(sels: List[ImportSelector]): ImportSelector | Null = sels match case sel :: sels if sel.isWildcard => @@ -303,33 +306,40 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha if !sym.exists then return // Names are resolved by definitions and imports, which have four precedence levels: - // 1. def from this compilation unit - // 2. specific import - // 3. wildcard import - // 4. def from another compilation unit via enclosing package - // We find the innermost, highest precedence. We have no nesting levels but assume correctness. + object PrecedenceLevels: + opaque type Precedence = Int + inline def NoPrecedence: Precedence = 5 + inline def OtherUnit: Precedence = 4 // root import or def from another compilation unit via enclosing package + inline def Wildcard: Precedence = 3 // wildcard import + inline def NamedImport: Precedence = 2 // specific import + inline def Definition: Precedence = 1 // def from this compilation unit + extension (p: Precedence) inline def weakerThan(q: Precedence) = p > q + import PrecedenceLevels.* + + // Find the innermost, highest precedence. Contexts have no nesting levels but assume correctness. // If the sym is an enclosing definition (the owner of a context), it does not count toward usages. val isLocal = sym.isLocalToBlock - var foundEnclosing = false var candidate: Context = NoContext var cachePoint: Context = NoContext // last context with Resolved cache var importer: ImportSelector | Null = null // non-null for import context - var precedence = Int.MaxValue // of current resolution; lower is higher + var precedence = NoPrecedence // of current resolution var done = false var cached = false val ctxs = ctx.outersIterator while !done && ctxs.hasNext do val cur = ctxs.next() if cur.owner eq sym then - foundEnclosing = true - done = true + return // found enclosing definition else if isLocal then - if cur.owner eq sym.owner then done = true // only checking for enclosing + if cur.owner eq sym.owner then + done = true // for local def, just checking that it is not enclosing else cached = cur.property(resolvedKey) match case Some(res) => - if precedence > 4 then cachePoint = cur // conservative, cache must be nested below the result context + // conservative, cache must be nested below the result context + if precedence == NoPrecedence then + cachePoint = cur res.saw(sym, name, prefix) case _ => false if cached then @@ -339,44 +349,40 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha val sel = matchingSelector(cur.importInfo.nn) if sel != null then if cur.importInfo.nn.isRootImport then - if precedence > 4 then - precedence = 4 + if precedence.weakerThan(OtherUnit) then + precedence = OtherUnit candidate = cur importer = sel done = true else if sel.isWildcard then - if precedence > 3 then - precedence = 3 + if precedence.weakerThan(Wildcard) then + precedence = Wildcard candidate = cur importer = sel else - if precedence > 2 then - precedence = 2 + if precedence.weakerThan(NamedImport) then + precedence = NamedImport candidate = cur importer = sel else if checkMember(cur.owner) then if sym.srcPos.sourcePos.source == ctx.source then - precedence = 1 + precedence = Definition candidate = cur importer = null done = true - else if precedence > 4 then - precedence = 4 + else if precedence.weakerThan(OtherUnit) then + precedence = OtherUnit candidate = cur end while - if !foundEnclosing then - refInfos.refs.addOne(sym) - if candidate != NoContext && candidate.isImportContext && importer != null - then refInfos.sels.put(importer, ()) - if !cached then - addCached(cachePoint) - if cachePoint ne ctx then - addCached(ctx) + // record usage and possibly an import + refInfos.refs.addOne(sym) + if candidate != NoContext && candidate.isImportContext && importer != null then + refInfos.sels.put(importer, ()) + if !cached then + addCached(cachePoint) + if cachePoint ne ctx then + addCached(ctx) end resolveUsage - // if sym is not an enclosing element, record the reference - def refUsage(sym: Symbol)(using Context): Unit = - if !ctx.outersIterator.exists(cur => cur.owner eq sym) then - refInfos.refs.addOne(sym) end CheckUnused object CheckUnused: @@ -399,7 +405,7 @@ object CheckUnused: /** Attachment holding the name of an Ident as written by the user. */ val OriginalName = Property.StickyKey[Name] - /** Suppress warning in a tree, such as a patvar absolved of unused warning by special naming convention. */ + /** Suppress warning in a tree, such as a patvar name allowed by special convention. */ val NoWarn = Property.StickyKey[Unit] /** Ignore reference. */ @@ -436,11 +442,12 @@ object CheckUnused: if (tree.symbol ne NoSymbol) && !tree.name.isWildcard then defs.addOne((tree.symbol, tree.namePos)) case _ => + //println(s"OTHER ${tree.symbol}") if tree.symbol ne NoSymbol then defs.addOne((tree.symbol, tree.srcPos)) - val inlined = Stack.empty[SrcPos] // enclosing call.srcPos of inlined code - var inliners = 0 // depth of inline def + val inlined = Stack.empty[SrcPos] // enclosing call.srcPos of inlined code (expansions) + var inliners = 0 // depth of inline def (not inlined yet) end RefInfos // Symbols already resolved in the given Context (with name and prefix of lookup) @@ -479,7 +486,7 @@ object CheckUnused: warnAt(pos)(UnusedSymbol.privateMembers) else if sym.is(Param, butNot = Given | Implicit) then val m = sym.owner - def forgiven = + def allowed = val dd = defn m.isDeprecated || m.is(Synthetic) && !m.isAnonymousFunction @@ -515,12 +522,12 @@ object CheckUnused: warnAt(pos)(UnusedSymbol.explicitParams) end checkExplicit if !infos.skip(m) - && !forgiven + && !allowed then checkExplicit() else if sym.is(Param) then // Given | Implicit val m = sym.owner - def forgiven = + def allowed = val dd = defn m.isDeprecated || m.is(Synthetic) @@ -536,7 +543,7 @@ object CheckUnused: || sym.info.isInstanceOf[RefinedType] // can't be expressed as a context bound if ctx.settings.WunusedHas.implicits && !infos.skip(m) - && !forgiven + && !allowed then if m.isPrimaryConstructor then val alias = m.owner.info.member(sym.name) @@ -710,8 +717,8 @@ object CheckUnused: // The RHS of a def is too trivial to warn about unused params, e.g. def f(x: Int) = ??? def isUnconsuming(rhs: Tree)(using Context): Boolean = rhs.symbol == defn.Predef_undefined - || rhs.tpe =:= defn.NothingType - || rhs.isInstanceOf[Literal] + || rhs.tpe =:= defn.NothingType // compiletime.error + || rhs.isInstanceOf[Literal] // 42 || rhs.tpe.match case ConstantType(_) => true case tp: TermRef => tp.underlying.classSymbol.is(Module) // Scala 2 SingleType @@ -719,56 +726,41 @@ object CheckUnused: //|| isPurePath(rhs) // a bit strong || rhs.match case Block((dd @ DefDef(anonfun, paramss, _, _)) :: Nil, Closure(Nil, Ident(nm), _)) => - isAnonymousFunctionName(anonfun) - && anonfun == nm + anonfun == nm // isAnonymousFunctionName(anonfun) && paramss.match case (ValDef(contextual, _, _) :: Nil) :: Nil => contextual.is(ContextFunctionParamName) - && isUnconsuming(dd.rhs) + && isUnconsuming(dd.rhs) // rhs was wrapped in a context function case _ => false - case Block(Nil, Literal(u)) => u.tpe =:= defn.UnitType + case Block(Nil, Literal(u)) => u.tpe =:= defn.UnitType // def f(x: X) = {} case This(_) => true case Ident(_) => rhs.symbol.is(ParamAccessor) case Typed(rhs, _) => isUnconsuming(rhs) case _ => false - def absolveVariableBindings(ok: List[Name], args: List[Tree]): Unit = - ok.zip(args).foreach: (param, arg) => - arg match - case Bind(`param`, _) => arg.withAttachment(NoWarn, ()) + def allowVariableBindings(ok: List[Name], args: List[Tree]): Unit = + ok.zip(args).foreach: + case (param, arg @ Bind(p, _)) if param == p => arg.withAttachment(NoWarn, ()) case _ => // NoWarn Binds if the name matches a "canonical" name, e.g. case element name val nowarner = new TreeTraverser: def traverse(tree: Tree)(using Context) = tree match case UnApply(fun, _, args) => - def untuple = defn.PairClass.companionModule.requiredMethod("unapply") val unapplied = tree.tpe.finalResultType.dealias.typeSymbol if unapplied.is(CaseClass) then - absolveVariableBindings(unapplied.primaryConstructor.info.firstParamNames, args) - else if fun.symbol == untuple then + allowVariableBindings(unapplied.primaryConstructor.info.firstParamNames, args) + else if fun.symbol == defn.PairClass_unapply then val ok = fun.symbol.info match case PolyType(tycon, MethodTpe(_, _, AppliedType(_, tprefs))) => tprefs.collect: case ref: TypeParamRef => termName(ref.binder.paramNames(ref.paramNum).toString.toLowerCase.nn) case _ => Nil - absolveVariableBindings(ok, args) + allowVariableBindings(ok, args) else if fun.symbol == defn.TypeTest_unapply then () // just recurse into args - /* - fun match - case Select(qual, nme.unapply) => - qual.tpe.underlying.finalResultType match - case AppliedType(tycon, args) if tycon.typeSymbol == defn.TypeTestClass => - val target = args(1).dealias.typeSymbol - if target.is(CaseClass) then - absolveVariableBindings(target.primaryConstructor.info.firstParamNames, args) - case _ => - case _ => - */ else - val Quotes_reflect: Symbol = defn.QuotesClass.requiredClass("reflectModule") - if unapplied.exists && unapplied.owner == Quotes_reflect then + if unapplied.exists && unapplied.owner == defn.Quotes_reflectModule then // cheapy search for parameter names via java reflection of Trees // in lieu of drilling into requiredClass("scala.quoted.runtime.impl.QuotesImpl") // ...member("reflect")...member(unapplied.name.toTypeName) @@ -776,9 +768,9 @@ object CheckUnused: val implName = s"dotty.tools.dotc.ast.Trees$$${unapplied.name}" try import scala.language.unsafeNulls - val clz = Class.forName(implName) + val clz = Class.forName(implName) // TODO improve to use class path or reflect val ok = clz.getConstructors.head.getParameters.map(p => termName(p.getName)).toList.init - absolveVariableBindings(ok, args) + allowVariableBindings(ok, args) catch case _: ClassNotFoundException => () args.foreach(traverse) case tree => traverseChildren(tree) @@ -830,7 +822,7 @@ object CheckUnused: case _ => false extension (imp: Import) - /** Is it the first import clause in a statement? `a.x` in `import a.x, b,{y, z}` */ + /** Is it the first import clause in a statement? `a.x` in `import a.x, b.{y, z}` */ def isPrimaryClause(using Context): Boolean = val span = imp.srcPos.span span.start != span.point // primary clause starts at `import` keyword @@ -844,15 +836,15 @@ object CheckUnused: * specifically does import an implicit. * Similarly, import of CanEqual must not warn, as it is always witness. */ - def isLoose(sel: ImportSelector)(using Context): Boolean = ctx.settings.WunusedHas.strictNoImplicitWarn && ( - sel.isWildcard - || imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit)) - || imp.expr.tpe.member(sel.name.toTypeName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit)) - ) || ( - sel.isWildcard && sel.isGiven - && imp.expr.tpe.allMembers.exists(_.symbol.isCanEqual) - || imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isCanEqual) - ) + def isLoose(sel: ImportSelector)(using Context): Boolean = + if ctx.settings.WunusedHas.strictNoImplicitWarn then + if sel.isWildcard + || imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit)) + || imp.expr.tpe.member(sel.name.toTypeName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit)) + then return true + if sel.isWildcard && sel.isGiven + then imp.expr.tpe.allMembers.exists(_.symbol.isCanEqual) + else imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isCanEqual) extension (pos: SrcPos) def isZeroExtentSynthetic: Boolean = pos.span.isSynthetic && pos.span.start == pos.span.end diff --git a/tests/warn/i15503f.scala b/tests/warn/i15503f.scala index a03d0926e658..0550f82d9398 100644 --- a/tests/warn/i15503f.scala +++ b/tests/warn/i15503f.scala @@ -32,11 +32,14 @@ object Example: case '{ None } => Some(None) case '{ ${Expr(opt)} : Some[T] } => Some(opt) case _ => None - given OptionFromExprNoisy[T](using Type[T], FromExpr[T]): FromExpr[Option[T]] with + +object ExampleWithoutWith: + import scala.quoted.* + given [T] => (Type[T], FromExpr[T]) => FromExpr[Option[T]]: def unapply(x: Expr[Option[T]])(using Quotes) = x match case '{ Option[T](${Expr(y)}) } => Some(Option(y)) case '{ None } => Some(None) - //case '{ ${Expr(opt)} : Some[T] } => Some(opt) // make Type param unused after typer + case '{ ${Expr(opt)} : Some[T] } => Some(opt) case _ => None //absolving names on matches of quote trees requires consulting non-abstract types in QuotesImpl diff --git a/tests/warn/i15503kb/power.scala b/tests/warn/i15503kb/power.scala index f4e17fc5a909..f7e5ccce6d58 100644 --- a/tests/warn/i15503kb/power.scala +++ b/tests/warn/i15503kb/power.scala @@ -1,6 +1,5 @@ object Power: - import scala.concurrent.* // warn [taps mic] import scala.math.pow as power import scala.quoted.* inline def powerMacro(x: Double, inline n: Int): Double = ${ powerCode('x, 'n) } diff --git a/tests/warn/i15503kb/square.scala b/tests/warn/i15503kb/square.scala index cf9e135dc9a0..2a5f76e9be83 100644 --- a/tests/warn/i15503kb/square.scala +++ b/tests/warn/i15503kb/square.scala @@ -1,6 +1,5 @@ -//> using options -Wunused:all +//> using options -Werror -Wunused:all object PowerUser: - import scala.concurrent.* // warn [taps mic] import Power.* def square(x: Double): Double = powerMacro(x, 2)