Skip to content

Commit

Permalink
Lint pattern varids which are backquotable
Browse files Browse the repository at this point in the history
For the `case x =>` where user intended to match
a value `x` in scope.
  • Loading branch information
som-snytt committed Dec 14, 2023
1 parent 43aa816 commit 2e18a11
Show file tree
Hide file tree
Showing 22 changed files with 169 additions and 37 deletions.
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2225,7 +2225,7 @@ self =>
else atPos(p.pos.start, p.pos.start, body.pos.end) {
val t = Bind(name, body)
body match {
case Ident(nme.WILDCARD) if settings.warnUnusedPatVars => t updateAttachment NoWarnAttachment
case Ident(nme.WILDCARD) if settings.warnUnusedPatVars || settings.warnPatternShadow => t.updateAttachment(NoWarnAttachment)
case _ => t
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/scala/tools/nsc/settings/Warnings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ trait Warnings {
val ArgDiscard = LintWarning("arg-discard", "-Wvalue-discard for adapted arguments.")
val IntDivToFloat = LintWarning("int-div-to-float", "Warn when an integer division is converted (widened) to floating point: `(someInt / 2): Double`.")
val NamedBooleans = LintWarning("named-booleans", "Boolean literals should be named args unless param is @deprecatedName.")
val PatternShadow = LintWarning("pattern-shadow", "Pattern variable id is also a term in scope.")

def allLintWarnings = values.toSeq.asInstanceOf[Seq[LintWarning]]
}
Expand Down Expand Up @@ -254,6 +255,7 @@ trait Warnings {
def lintArgDiscard = lint.contains(ArgDiscard)
def lintIntDivToFloat = lint.contains(IntDivToFloat)
def lintNamedBooleans = lint.contains(NamedBooleans)
def warnPatternShadow = lint.contains(PatternShadow)

// The Xlint warning group.
val lint = MultiChoiceSetting(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ trait PatternMatching extends Transform

class MatchTransformer(unit: CompilationUnit) extends TypingTransformer(unit) {
override def transform(tree: Tree): Tree = tree match {
case Match(sel, cases) =>
case Match(selector, cases) =>
val origTp = tree.tpe

// setType origTp intended for CPS -- TODO: is it necessary?
val translated = translator(sel.pos).translateMatch(treeCopy.Match(tree, transform(sel), transformCaseDefs(cases)))
val translated = translator(selector.pos).translateMatch(treeCopy.Match(tree, transform(selector), transformCaseDefs(cases)))
try {
// Keep 2.12 behaviour of using wildcard expected type, recomputing the LUB, then throwing it away for the continuations plugins
// but for the rest of us pass in top as the expected type to avoid waste.
Expand Down
10 changes: 10 additions & 0 deletions src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,16 @@ abstract class RefChecks extends Transform {
if (isMultiline && settings.warnByNameImplicit) checkImplicitlyAdaptedBlockResult(expr)

tree
case Match(selector, cases) =>
def checkShadowedSymbol(sym: Symbol): Boolean =
sym.ne(selector.symbol) &&
sym.accessedOrSelf.ne(selector.symbol.accessedOrSelf) &&
!sym.hasPackageFlag && sym.isStable && !isByName(sym)
def checkShadowedRef(sym: Symbol): Boolean =
if (sym.isOverloaded) sym.alternatives.exists(checkShadowedSymbol) else checkShadowedSymbol(sym)
for (cdef <- cases; bind @ Bind(name, _) <- cdef.pat; shadow <- bind.getAndRemoveAttachment[PatShadowAttachment] if checkShadowedRef(shadow.shadowed))
refchecksWarning(bind.pos, s"Name $name is already introduced in an enclosing scope as ${shadow.shadowed.fullLocationString}. Did you intend to match it using backquoted `$name`?", WarningCategory.OtherShadowing)
tree
case _ => tree
}

Expand Down
13 changes: 10 additions & 3 deletions src/compiler/scala/tools/nsc/typechecker/Typers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2708,16 +2708,23 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
}
initChildren(selectorTp.typeSymbol)

def finish(cases: List[CaseDef], matchType: Type) =
treeCopy.Match(tree, selector1, cases) setType matchType
def finish(cases: List[CaseDef], matchType: Type) = {
if (!isPastTyper && settings.warnPatternShadow && !context.owner.isSynthetic && selector.symbol != null)
for (cdef <- cases; bind @ Bind(name, _) <- cdef.pat if !bind.hasAttachment[NoWarnAttachment.type])
context.lookupSymbol(name, _ => true) match {
case LookupSucceeded(_, sym) => bind.updateAttachment(PatShadowAttachment(sym))
case _ =>
}
treeCopy.Match(tree, selector1, cases).setType(matchType)
}

if (isFullyDefined(pt))
finish(casesTyped, pt)
else packedTypes(casesTyped) match {
case packed if sameWeakLubAsLub(packed) => finish(casesTyped, lub(packed))
case packed =>
val lub = weakLub(packed)
finish(casesTyped map (adaptCase(_, mode, lub)), lub)
finish(casesTyped.map(adaptCase(_, mode, lub)), lub)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/partest/scala/tools/partest/ReplTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ abstract class ReplTest extends DirectTest {
if (getClass.getClassLoader.getParent != null) {
s.classpath.value = s.classpath.value match {
case "" => testOutput.toString
case s => s + java.io.File.pathSeparator + testOutput.toString
case cp => cp + java.io.File.pathSeparator + testOutput.toString
}
s.usejavacp.value = true
}
Expand Down
6 changes: 3 additions & 3 deletions src/partest/scala/tools/partest/ScaladocModelTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,11 @@ abstract class ScaladocModelTest extends DirectTest {

def countLinks(c: Comment, p: EntityLink => Boolean): Int = countLinksInBody(c.body, p)

def countLinksInBody(body: Body, p: EntityLink => Boolean): Int = {
def countLinksInBody(body: Body, linkTest: EntityLink => Boolean): Int = {
def countLinks(b: Any): Int = b match {
case el: EntityLink if p(el) => 1
case el: EntityLink if linkTest(el) => 1
case s: collection.Seq[_] => s.toList.map(countLinks(_)).sum
case p: Product => p.productIterator.toList.map(countLinks(_)).sum
case p: Product => p.productIterator.map(countLinks(_)).sum
case _ => 0
}
countLinks(body)
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/Kinds.scala
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ trait Kinds {
case 2 => "X"
case 3 => "Y"
case 4 => "Z"
case n if n < 12 => ('O'.toInt - 5 + n).toChar.toString
case x if x < 12 => ('O'.toInt - 5 + x).toChar.toString
case _ => "V"
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/reflect/scala/reflect/internal/ReificationSupport.scala
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ trait ReificationSupport { self: SymbolTable =>
}

def mkAnnotation(tree: Tree): Tree = tree match {
case SyntacticNew(Nil, SyntacticApplied(SyntacticAppliedType(_, _), _) :: Nil, noSelfType, Nil) =>
case SyntacticNew(Nil, SyntacticApplied(SyntacticAppliedType(_, _), _) :: Nil, `noSelfType`, Nil) =>
tree
case _ =>
throw new IllegalArgumentException(s"Tree ${showRaw(tree)} isn't a correct representation of annotation." +
Expand Down Expand Up @@ -876,10 +876,10 @@ trait ReificationSupport { self: SymbolTable =>
// drop potential @scala.unchecked annotation
protected object MaybeUnchecked {
def unapply(tree: Tree): Some[Tree] = tree match {
case Annotated(SyntacticNew(Nil, ScalaDot(tpnme.unchecked) :: Nil, noSelfType, Nil), annottee) =>
case Annotated(SyntacticNew(Nil, ScalaDot(tpnme.unchecked) :: Nil, `noSelfType`, Nil), annottee) =>
Some(annottee)
case Typed(annottee, MaybeTypeTreeOriginal(
Annotated(SyntacticNew(Nil, ScalaDot(tpnme.unchecked) :: Nil, noSelfType, Nil), _))) =>
Annotated(SyntacticNew(Nil, ScalaDot(tpnme.unchecked) :: Nil, `noSelfType`, Nil), _))) =>
Some(annottee)
case annottee => Some(annottee)
}
Expand All @@ -904,7 +904,7 @@ trait ReificationSupport { self: SymbolTable =>
case Typed(
Block(
List(ClassDef(clsMods, tpnme.ANON_FUN_NAME, Nil, Template(
List(abspf: TypeTree, ser: TypeTree), noSelfType, List(
List(abspf: TypeTree, ser: TypeTree), `noSelfType`, List(
DefDef(_, nme.CONSTRUCTOR, _, _, _, _),
DefDef(_, nme.applyOrElse, _, _, _,
Match(_, cases :+
Expand Down
4 changes: 4 additions & 0 deletions src/reflect/scala/reflect/internal/StdAttachments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ trait StdAttachments {
*/
case object NoWarnAttachment extends PlainAttachment

/** A pattern binding that shadows a symbol in scope. Removed by refchecks.
*/
case class PatShadowAttachment(shadowed: Symbol)

/** Indicates that a `ValDef` was synthesized from a pattern definition, `val P(x)`.
*/
case object PatVarDefAttachment extends PlainAttachment
Expand Down
4 changes: 2 additions & 2 deletions src/reflect/scala/reflect/internal/TreeInfo.scala
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ abstract class TreeInfo {
@tailrec
def loop(tree: Tree): Tree = tree match {
case Apply(fn, _) => loop(fn)
case tree => tree
case _ => tree
}
loop(tree)
}
Expand All @@ -852,7 +852,7 @@ abstract class TreeInfo {
def core: Tree = callee match {
case TypeApply(fn, _) => fn
case AppliedTypeTree(fn, _) => fn
case tree => tree
case _ => callee
}

/** The type arguments of the `callee`.
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1827,7 +1827,7 @@ trait Types
val flattened: LinkedHashSet[Type] = LinkedHashSet.empty[Type]
def dealiasRefinement(tp: Type) = if (tp.dealias.isInstanceOf[RefinedType]) tp.dealias else tp
def loop(tp: Type): Unit = dealiasRefinement(tp) match {
case RefinedType(parents, ds) if ds.isEmpty => parents.foreach(loop)
case RefinedType(ps, ds) if ds.isEmpty => ps.foreach(loop)
case tp => flattened.add(tp)
}
parents foreach loop
Expand Down
4 changes: 2 additions & 2 deletions src/reflect/scala/reflect/internal/util/ChromeTrace.scala
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ final class ChromeTrace(f: Path) extends Closeable {
case oc @ ObjectContext(first) =>
if (first) oc.first = false
else traceWriter.write(",")
case context =>
throw new IllegalStateException("Wrong context: " + context)
case kontext =>
throw new IllegalStateException(s"Wrong context: $kontext")
}
traceWriter.write("\"")
traceWriter.write(name)
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
this.InfixAttachment
this.AutoApplicationAttachment
this.NoWarnAttachment
this.PatShadowAttachment
this.PatVarDefAttachment
this.ForAttachment
this.SyntheticUnitAttachment
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ trait LoopCommands {
def complete(buffer: String, cursor: Int, filter: Boolean) =
CompletionResult(buffer, cursor = 1, List(CompletionCandidate(completion)), "", "")
}
case cmd :: rest =>
case cmd :: _ =>
new Completion {
def complete(buffer: String, cursor: Int, filter: Boolean) =
CompletionResult(buffer, cursor = 1, cmds.map(cmd => CompletionCandidate(":" + cmd.name)), "", "")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class Scripted(@BeanProperty val factory: ScriptEngineFactory, settings: Setting
def dynamicContext_=(ctx: ScriptContext): Unit = intp.call("set", ctx)

def dynamicContext: ScriptContext = intp.call("value") match {
case Right(ctx: ScriptContext) => ctx
case Right(scriptctx: ScriptContext) => scriptctx
case Left(e) => throw e
case Right(other) => throw new ScriptException(s"Unexpected value for context: $other")
}
Expand Down
8 changes: 4 additions & 4 deletions src/repl/scala/tools/nsc/interpreter/IMain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1087,12 +1087,12 @@ class IMain(val settings: Settings, parentClassLoaderOverride: Option[ClassLoade
cur.typeSignature.decls.find(x => x.name.toString == last && x.isAccessor).map { m =>
mirrored.reflectMethod(m.asMethod).apply()
}
case next :: rest =>
case next :: more =>
val s = cur.typeSignature.member(TermName(next))
val i =
if (s.isModule) {
if (inst == null) null
else runtimeMirror.reflect((runtimeMirror reflectModule s.asModule).instance)
else runtimeMirror.reflect(runtimeMirror.reflectModule(s.asModule).instance)
}
else if (s.isAccessor) {
runtimeMirror.reflect(mirrored.reflectMethod(s.asMethod).apply())
Expand All @@ -1101,11 +1101,11 @@ class IMain(val settings: Settings, parentClassLoaderOverride: Option[ClassLoade
assert(false, fullName)
inst
}
loop(i, s, rest)
loop(i, s, more)
case Nil => None
}
}
loop(null, top, rest)
loop(inst = null, top, rest)
}
Option(symbolOfTerm(id)).filter(_.exists).flatMap(s => Trying(value(originalPath(s))).toOption.flatten)
}
Expand Down
16 changes: 8 additions & 8 deletions src/repl/scala/tools/nsc/interpreter/ReplGlobal.scala
Original file line number Diff line number Diff line change
Expand Up @@ -80,19 +80,19 @@ trait ReplGlobal extends Global {
}

override def transform(tree: Tree): Tree = super.transform(tree) match {
case tree @ Template(parents, self, body) if nme.isReplWrapperName(tree.symbol.name) =>
case t @ Template(parents, self, body) if nme.isReplWrapperName(t.symbol.name) =>
val unusedPrivates = newUnusedPrivates
unusedPrivates.traverse(tree)
unusedPrivates.traverse(t)
val unusedSyms = unusedPrivates.unusedTerms.iterator.map(_.symbol)
val unusedLineReadVals = unusedSyms.filter(sym => isLineReadVal(sym.name)).flatMap(sym => List(sym, sym.accessedOrSelf)).toSet
val (removedStats, retainedStats) = tree.body.partition (t => unusedLineReadVals(t.symbol))
if (removedStats.isEmpty) tree
val (removedStats, retainedStats) = body.partition(stat => unusedLineReadVals(stat.symbol))
if (removedStats.isEmpty) t
else {
val decls = tree.symbol.info.decls
removedStats.foreach(tree => decls.unlink(tree.symbol))
deriveTemplate(tree)(_ => retainedStats)
val decls = t.symbol.info.decls
removedStats.foreach(stat => decls.unlink(stat.symbol))
deriveTemplate(t)(_ => retainedStats)
}
case tree => tree
case t => t
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/sbt-bridge/scala/tools/xsbt/ExtractAPI.scala
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ class ExtractAPI[GlobalType <: Global](
xsbti.api.Annotated.of(processType(in, at.underlying), mkAnnotations(in, annots))
}
case rt: CompoundType => structure(rt, rt.typeSymbol)
case t: ExistentialType => makeExistentialType(in, t)
case et: ExistentialType => makeExistentialType(in, et)
case NoType =>
Constants.emptyType // this can happen when there is an error that will be reported by a later phase
case PolyType(typeParams, resultType) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ trait ModelFactoryTypeSupport {
case _ => Tooltip(bTpl.qualifiedName)
}
case _ =>
val oTpl = findTemplateMaybe(owner)
(oTpl, oTpl flatMap (findMember(bSym, _))) match {
val oTpl0 = findTemplateMaybe(owner)
(oTpl0, oTpl0.flatMap(findMember(bSym, _))) match {
case (Some(oTpl), Some(bMbr)) =>
// (1) the owner's class
LinkToMember(bMbr, oTpl)
Expand Down
12 changes: 12 additions & 0 deletions test/files/neg/t11850.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
t11850.scala:9: warning: Name x is already introduced in an enclosing scope as value x in trait T. Did you intend to match it using backquoted `x`?
case x => 1 // warn
^
t11850.scala:27: warning: Name x is already introduced in an enclosing scope as value x in class CT. Did you intend to match it using backquoted `x`?
case x => 1 // warn
^
t11850.scala:81: warning: Name fn is already introduced in an enclosing scope as value fn. Did you intend to match it using backquoted `fn`?
case Apply(fn, Nil) => println(fn)
^
error: No warnings can be incurred under -Werror.
3 warnings
1 error

0 comments on commit 2e18a11

Please sign in to comment.