Skip to content

Commit

Permalink
In selector check, prefix of reference must match import qualifier (#…
Browse files Browse the repository at this point in the history
…20894)

This PR changes the `CheckUnused` phase to rely on the `MiniPhase` API
(instead of custom traversal). That improves fidelity to `Context`
(instead of approximate scoping).

The phase should work seamlessly with subsequent linting phases
(currently, `CheckShadowed`).

It is a goal of the PR to eliminate false reports. It is also a goal not
to regress previous work on efficiency.

A remaining limitation of the current approach is that contexts don't
provide a nesting level. Practically, this means that for a wildcard
import nested below a higher precedence named import, the wildcard is
deemed "unused". (A more general tool for "managing" or "formatting"
imports could do more to pick a preferred scope.)

This PR adds `-Wunused:patvars`, as forward-ported from Scala 2: it
relies on attachments for some details about desugaring, but otherwise
uses positions (where only the original patvar has a non-synthetic
position).

As in Scala 2, it does not warn about patvars with the "canonical" name
of a case class element (this is complicated by type tests and the
quotes API); other exclusions are to be ported, such as "name derived
from the match selector".

Support is added for `-Wconf:origin=full.path.selector`, as in Scala 2.
That allows, for example:
```
-Wconf:origin=scala.util.chaining.given:s
```
to exclude certain blessed imports from warnings, or to work around
false positives (should they arise).

Support is added to `-rewrite` unused imports. There are no options to
"format"; instead, textual deletions preserve existing formatting,
except that blank lines are removed and braces removed when there is
only one selector.

Notable fixes are to support `compiletime` and `inline`; there are more
fixes to pursue in this area.

Fixes #19657
Fixes #20520
Fixes #19998
Fixes #18313
Fixes #17371
Fixes #18708
Fixes #21917
Fixes #21420
Fixes #20951
Fixes #19252
Fixes #18289
Fixes #17667
Fixes #17252
Fixes #21807
Fixes #17753
Fixes #17318
Fixes #18564
Fixes #22376
Fixes #21525
  • Loading branch information
sjrd authored Jan 28, 2025
2 parents 89c20f8 + 152a8cd commit 9cb97ec
Show file tree
Hide file tree
Showing 94 changed files with 2,638 additions and 1,248 deletions.
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {

methSymbol = dd.symbol
jMethodName = methSymbol.javaSimpleName
returnType = asmMethodType(dd.symbol).returnType
returnType = asmMethodType(methSymbol).returnType
isMethSymStaticCtor = methSymbol.isStaticConstructor

resetMethodBookkeeping(dd)
Expand Down Expand Up @@ -915,7 +915,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {
for (p <- params) { emitLocalVarScope(p.symbol, veryFirstProgramPoint, onePastLastProgramPoint, force = true) }
}

if (isMethSymStaticCtor) { appendToStaticCtor(dd) }
if (isMethSymStaticCtor) { appendToStaticCtor() }
} // end of emitNormalMethodBody()

lineNumber(rhs)
Expand All @@ -936,7 +936,7 @@ trait BCodeSkelBuilder extends BCodeHelpers {
*
* TODO document, explain interplay with `fabricateStaticInitAndroid()`
*/
private def appendToStaticCtor(dd: DefDef): Unit = {
private def appendToStaticCtor(): Unit = {

def insertBefore(
location: asm.tree.AbstractInsnNode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import java.util.zip.{CRC32, Deflater, ZipEntry, ZipOutputStream}

import dotty.tools.dotc.core.Contexts.*
import dotty.tools.dotc.core.Decorators.em
import dotty.tools.dotc.util.chaining.*
import dotty.tools.io.{AbstractFile, PlainFile, VirtualFile}
import dotty.tools.io.PlainFile.toPlainFile
import BTypes.InternalName
import scala.util.chaining.*
import dotty.tools.io.JarArchive

import scala.language.unsafeNulls
Expand Down
6 changes: 2 additions & 4 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import typer.{TyperPhase, RefChecks}
import parsing.Parser
import Phases.Phase
import transform.*
import dotty.tools.backend
import backend.jvm.{CollectSuperCalls, GenBCode}
import localopt.StringInterpolatorOpt

Expand All @@ -34,8 +33,7 @@ class Compiler {
protected def frontendPhases: List[List[Phase]] =
List(new Parser) :: // Compiler frontend: scanner, parser
List(new TyperPhase) :: // Compiler frontend: namer, typer
List(new CheckUnused.PostTyper) :: // Check for unused elements
List(new CheckShadowing) :: // Check shadowing elements
List(CheckUnused.PostTyper(), CheckShadowing()) :: // Check for unused, shadowed elements
List(new YCheckPositions) :: // YCheck positions
List(new sbt.ExtractDependencies) :: // Sends information on classes' dependencies to sbt via callbacks
List(new semanticdb.ExtractSemanticDB.ExtractSemanticInfo) :: // Extract info into .semanticdb files
Expand All @@ -51,10 +49,10 @@ class Compiler {
List(new sbt.ExtractAPI) :: // Sends a representation of the API of classes to sbt via callbacks
List(new Inlining) :: // Inline and execute macros
List(new PostInlining) :: // Add mirror support for inlined code
List(new CheckUnused.PostInlining) :: // Check for unused elements
List(new Staging) :: // Check staging levels and heal staged types
List(new Splicing) :: // Replace level 1 splices with holes
List(new PickleQuotes) :: // Turn quoted trees into explicit run-time data structures
List(new CheckUnused.PostInlining) :: // Check for unused elements
Nil

/** Phases dealing with the transformation from pickled trees to backend trees */
Expand Down
14 changes: 12 additions & 2 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ object desugar {
*/
val UntupledParam: Property.Key[Unit] = Property.StickyKey()

/** An attachment key to indicate that a ValDef originated from a pattern.
*/
val PatternVar: Property.Key[Unit] = Property.StickyKey()

/** An attachment key for Trees originating in for-comprehension, such as tupling of assignments.
*/
val ForArtifact: Property.Key[Unit] = Property.StickyKey()

/** An attachment key to indicate that a ValDef is an evidence parameter
* for a context bound.
*/
Expand Down Expand Up @@ -1507,7 +1515,7 @@ object desugar {
val matchExpr =
if (tupleOptimizable) rhs
else
val caseDef = CaseDef(pat, EmptyTree, makeTuple(ids))
val caseDef = CaseDef(pat, EmptyTree, makeTuple(ids).withAttachment(ForArtifact, ()))
Match(makeSelector(rhs, MatchCheck.IrrefutablePatDef), caseDef :: Nil)
vars match {
case Nil if !mods.is(Lazy) =>
Expand Down Expand Up @@ -1537,6 +1545,7 @@ object desugar {
ValDef(named.name.asTermName, tpt, selector(n))
.withMods(mods)
.withSpan(named.span)
.withAttachment(PatternVar, ())
)
flatTree(firstDef :: restDefs)
}
Expand Down Expand Up @@ -1922,6 +1931,7 @@ object desugar {
val vdef = ValDef(named.name.asTermName, tpt, rhs)
.withMods(mods)
.withSpan(original.span.withPoint(named.span.start))
.withAttachment(PatternVar, ())
val mayNeedSetter = valDef(vdef)
mayNeedSetter
}
Expand Down Expand Up @@ -2167,7 +2177,7 @@ object desugar {
case _ => Modifiers()
makePatDef(valeq, mods, defpat, rhs)
}
val rhs1 = makeFor(nme.map, nme.flatMap, GenFrom(defpat0, gen.expr, gen.checkMode) :: Nil, Block(pdefs, makeTuple(id0 :: ids)))
val rhs1 = makeFor(nme.map, nme.flatMap, GenFrom(defpat0, gen.expr, gen.checkMode) :: Nil, Block(pdefs, makeTuple(id0 :: ids).withAttachment(ForArtifact, ())))
val allpats = gen.pat :: pats
val vfrom1 = GenFrom(makeTuple(allpats), rhs1, GenCheckMode.Ignore)
makeFor(mapName, flatMapName, vfrom1 :: rest1, body)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/config/CliCommand.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Settings.*
import core.Contexts.*
import printing.Highlighting

import scala.util.chaining.given
import dotty.tools.dotc.util.chaining.*
import scala.PartialFunction.cond

trait CliCommand:
Expand Down
22 changes: 6 additions & 16 deletions compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import dotty.tools.io.{AbstractFile, Directory, JDK9Reflectors, PlainDirectory,
import Setting.ChoiceWithHelp
import ScalaSettingCategories.*

import scala.util.chaining.*
import dotty.tools.dotc.util.chaining.*

import java.util.zip.Deflater

Expand Down Expand Up @@ -173,28 +173,20 @@ private sealed trait WarningSettings:
choices = List(
ChoiceWithHelp("nowarn", ""),
ChoiceWithHelp("all", ""),
ChoiceWithHelp(
name = "imports",
description = "Warn if an import selector is not referenced.\n" +
"NOTE : overrided by -Wunused:strict-no-implicit-warn"),
ChoiceWithHelp("imports", "Warn if an import selector is not referenced."),
ChoiceWithHelp("privates", "Warn if a private member is unused"),
ChoiceWithHelp("locals", "Warn if a local definition is unused"),
ChoiceWithHelp("explicits", "Warn if an explicit parameter is unused"),
ChoiceWithHelp("implicits", "Warn if an implicit parameter is unused"),
ChoiceWithHelp("params", "Enable -Wunused:explicits,implicits"),
ChoiceWithHelp("patvars","Warn if a variable bound in a pattern is unused"),
//ChoiceWithHelp("inlined", "Apply -Wunused to inlined expansions"), // TODO
ChoiceWithHelp("linted", "Enable -Wunused:imports,privates,locals,implicits"),
ChoiceWithHelp(
name = "strict-no-implicit-warn",
description = "Same as -Wunused:import, only for imports of explicit named members.\n" +
"NOTE : This overrides -Wunused:imports and NOT set by -Wunused:all"
),
// ChoiceWithHelp("patvars","Warn if a variable bound in a pattern is unused"),
ChoiceWithHelp(
name = "unsafe-warn-patvars",
description = "(UNSAFE) Warn if a variable bound in a pattern is unused.\n" +
"This warning can generate false positive, as warning cannot be\n" +
"suppressed yet."
)
),
default = Nil
)
Expand All @@ -206,7 +198,6 @@ private sealed trait WarningSettings:
// Is any choice set for -Wunused?
def any(using Context): Boolean = Wall.value || Wunused.value.nonEmpty

// overrided by strict-no-implicit-warn
def imports(using Context) =
(allOr("imports") || allOr("linted")) && !(strictNoImplicitWarn)
def locals(using Context) =
Expand All @@ -220,9 +211,8 @@ private sealed trait WarningSettings:
def params(using Context) = allOr("params")
def privates(using Context) =
allOr("privates") || allOr("linted")
def patvars(using Context) =
isChoiceSet("unsafe-warn-patvars") // not with "all"
// allOr("patvars") // todo : rename once fixed
def patvars(using Context) = allOr("patvars")
def inlined(using Context) = isChoiceSet("inlined")
def linted(using Context) =
allOr("linted")
def strictNoImplicitWarn(using Context) =
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ class Definitions {
@tu lazy val Predef_undefined: Symbol = ScalaPredefModule.requiredMethod(nme.???)
@tu lazy val ScalaPredefModuleClass: ClassSymbol = ScalaPredefModule.moduleClass.asClass

@tu lazy val SameTypeClass: ClassSymbol = requiredClass("scala.=:=")
@tu lazy val SameType_refl: Symbol = SameTypeClass.companionModule.requiredMethod(nme.refl)
@tu lazy val SubTypeClass: ClassSymbol = requiredClass("scala.<:<")
@tu lazy val SubType_refl: Symbol = SubTypeClass.companionModule.requiredMethod(nme.refl)

Expand Down Expand Up @@ -834,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")
Expand Down Expand Up @@ -955,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
Expand Down Expand Up @@ -1062,6 +1066,7 @@ class Definitions {
@tu lazy val UntrackedCapturesAnnot: ClassSymbol = requiredClass("scala.caps.untrackedCaptures")
@tu lazy val UseAnnot: ClassSymbol = requiredClass("scala.caps.use")
@tu lazy val VolatileAnnot: ClassSymbol = requiredClass("scala.volatile")
@tu lazy val LanguageFeatureMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.languageFeature")
@tu lazy val BeanGetterMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.beanGetter")
@tu lazy val BeanSetterMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.beanSetter")
@tu lazy val FieldMetaAnnot: ClassSymbol = requiredClass("scala.annotation.meta.field")
Expand Down
16 changes: 8 additions & 8 deletions compiler/src/dotty/tools/dotc/report.scala
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
package dotty.tools.dotc

import reporting.*
import Diagnostic.*
import util.{SourcePosition, NoSourcePosition, SrcPos}
import core.*
import Contexts.*, Flags.*, Symbols.*, Decorators.*
import config.SourceVersion
import ast.*
import config.Feature.sourceVersion
import core.*, Contexts.*, Flags.*, Symbols.*, Decorators.*
import config.Feature.sourceVersion, config.{MigrationVersion, SourceVersion}
import reporting.*, Diagnostic.*
import util.{SourcePosition, NoSourcePosition, SrcPos}

import java.lang.System.currentTimeMillis
import dotty.tools.dotc.config.MigrationVersion

object report:

Expand Down Expand Up @@ -55,6 +52,9 @@ object report:
else issueWarning(new FeatureWarning(msg, pos.sourcePos))
end featureWarning

def warning(msg: Message, pos: SrcPos, origin: String)(using Context): Unit =
issueWarning(LintWarning(msg, addInlineds(pos), origin))

def warning(msg: Message, pos: SrcPos)(using Context): Unit =
issueWarning(new Warning(msg, addInlineds(pos)))

Expand Down
24 changes: 16 additions & 8 deletions compiler/src/dotty/tools/dotc/reporting/Diagnostic.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import dotty.tools.dotc.config.Settings.Setting
import dotty.tools.dotc.core.Contexts.*
import dotty.tools.dotc.interfaces.Diagnostic.{ERROR, INFO, WARNING}
import dotty.tools.dotc.util.SourcePosition
import dotty.tools.dotc.util.chaining.*

import java.util.{Collections, Optional, List => JList}
import scala.util.chaining.*
import core.Decorators.toMessage

object Diagnostic:
Expand All @@ -36,6 +36,18 @@ object Diagnostic:
pos: SourcePosition
) extends Error(msg, pos)

/** A Warning with an origin. The semantics of `origin` depend on the warning.
* For example, an unused import warning has an origin that specifies the unused selector.
* The origin of a deprecation is the deprecated element.
*/
trait OriginWarning(val origin: String):
self: Warning =>

/** Lints are likely to be filtered. Provide extra axes for filtering by `-Wconf`.
*/
class LintWarning(msg: Message, pos: SourcePosition, origin: String)
extends Warning(msg, pos), OriginWarning(origin)

class Warning(
msg: Message,
pos: SourcePosition
Expand Down Expand Up @@ -73,13 +85,9 @@ object Diagnostic:
def enablingOption(using Context): Setting[Boolean] = ctx.settings.unchecked
}

class DeprecationWarning(
msg: Message,
pos: SourcePosition,
val origin: String
) extends ConditionalWarning(msg, pos) {
class DeprecationWarning(msg: Message, pos: SourcePosition, origin: String)
extends ConditionalWarning(msg, pos), OriginWarning(origin):
def enablingOption(using Context): Setting[Boolean] = ctx.settings.deprecation
}

class MigrationWarning(
msg: Message,
Expand All @@ -104,5 +112,5 @@ class Diagnostic(
override def diagnosticRelatedInformation: JList[interfaces.DiagnosticRelatedInformation] =
Collections.emptyList()

override def toString: String = s"$getClass at $pos: $message"
override def toString: String = s"$getClass at $pos L${pos.line+1}: $message"
end Diagnostic
24 changes: 13 additions & 11 deletions compiler/src/dotty/tools/dotc/reporting/WConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ import scala.annotation.internal.sharable
import scala.util.matching.Regex

enum MessageFilter:
def matches(message: Diagnostic): Boolean = this match
def matches(message: Diagnostic): Boolean =
import Diagnostic.*
this match
case Any => true
case Deprecated => message.isInstanceOf[Diagnostic.DeprecationWarning]
case Feature => message.isInstanceOf[Diagnostic.FeatureWarning]
case Unchecked => message.isInstanceOf[Diagnostic.UncheckedWarning]
case Deprecated => message.isInstanceOf[DeprecationWarning]
case Feature => message.isInstanceOf[FeatureWarning]
case Unchecked => message.isInstanceOf[UncheckedWarning]
case MessageID(errorId) => message.msg.errorId == errorId
case MessagePattern(pattern) =>
val noHighlight = message.msg.message.replaceAll("\\e\\[[\\d;]*[^\\d;]","")
Expand All @@ -31,7 +33,7 @@ enum MessageFilter:
pattern.findFirstIn(path).nonEmpty
case Origin(pattern) =>
message match
case message: Diagnostic.DeprecationWarning => pattern.findFirstIn(message.origin).nonEmpty
case message: OriginWarning => pattern.findFirstIn(message.origin).nonEmpty
case _ => false
case None => false

Expand All @@ -56,12 +58,12 @@ object WConf:
private type Conf = (List[MessageFilter], Action)

def parseAction(s: String): Either[List[String], Action] = s match
case "error" | "e" => Right(Error)
case "warning" | "w" => Right(Warning)
case "verbose" | "v" => Right(Verbose)
case "info" | "i" => Right(Info)
case "silent" | "s" => Right(Silent)
case _ => Left(List(s"unknown action: `$s`"))
case "error" | "e" => Right(Error)
case "warning" | "w" => Right(Warning)
case "verbose" | "v" => Right(Verbose)
case "info" | "i" => Right(Info)
case "silent" | "s" => Right(Silent)
case _ => Left(List(s"unknown action: `$s`"))

private def regex(s: String) =
try Right(s.r)
Expand Down
24 changes: 13 additions & 11 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3290,22 +3290,24 @@ extends TypeMsg(ConstructorProxyNotValueID):
|companion value with the (term-)name `A`. However, these context bound companions
|are not values themselves, they can only be referred to in selections."""

class UnusedSymbol(errorText: String)(using Context)
class UnusedSymbol(errorText: String, val actions: List[CodeAction] = Nil)(using Context)
extends Message(UnusedSymbolID) {
def kind = MessageKind.UnusedSymbol

override def msg(using Context) = errorText
override def explain(using Context) = ""
}

object UnusedSymbol {
def imports(using Context): UnusedSymbol = new UnusedSymbol(i"unused import")
def localDefs(using Context): UnusedSymbol = new UnusedSymbol(i"unused local definition")
def explicitParams(using Context): UnusedSymbol = new UnusedSymbol(i"unused explicit parameter")
def implicitParams(using Context): UnusedSymbol = new UnusedSymbol(i"unused implicit parameter")
def privateMembers(using Context): UnusedSymbol = new UnusedSymbol(i"unused private member")
def patVars(using Context): UnusedSymbol = new UnusedSymbol(i"unused pattern variable")
}
override def actions(using Context) = this.actions
}

object UnusedSymbol:
def imports(actions: List[CodeAction])(using Context): UnusedSymbol = UnusedSymbol(i"unused import", actions)
def localDefs(using Context): UnusedSymbol = UnusedSymbol(i"unused local definition")
def explicitParams(using Context): UnusedSymbol = UnusedSymbol(i"unused explicit parameter")
def implicitParams(using Context): UnusedSymbol = UnusedSymbol(i"unused implicit parameter")
def privateMembers(using Context): UnusedSymbol = UnusedSymbol(i"unused private member")
def patVars(using Context): UnusedSymbol = UnusedSymbol(i"unused pattern variable")
def unsetLocals(using Context): UnusedSymbol = UnusedSymbol(i"unset local variable, consider using an immutable val instead")
def unsetPrivates(using Context): UnusedSymbol = UnusedSymbol(i"unset private variable, consider using an immutable val instead")

class NonNamedArgumentInJavaAnnotation(using Context) extends SyntaxMsg(NonNamedArgumentInJavaAnnotationID):

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import ExtractAPI.NonLocalClassSymbolsInCurrentUnits

import scala.collection.mutable
import scala.util.hashing.MurmurHash3
import scala.util.chaining.*
import dotty.tools.dotc.util.chaining.*

/** This phase sends a representation of the API of classes to sbt via callbacks.
*
Expand Down
Loading

0 comments on commit 9cb97ec

Please sign in to comment.