Skip to content

Fix #17667 account for import order #17951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 33 additions & 24 deletions compiler/src/dotty/tools/dotc/transform/CheckUnused.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import dotty.tools.dotc.core.Definitions
import dotty.tools.dotc.core.NameKinds.WildcardParamName
import dotty.tools.dotc.core.Symbols.Symbol
import dotty.tools.dotc.core.StdNames.nme
import scala.math.Ordering


/**
Expand Down Expand Up @@ -87,15 +86,15 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
val prefixes = LazyList.iterate(tree.typeOpt.normalizedPrefix)(_.normalizedPrefix).takeWhile(_ != NoType)
.take(10) // Failsafe for the odd case if there was an infinite cycle
for prefix <- prefixes do
unusedDataApply(_.registerUsed(prefix.classSymbol, None))
unusedDataApply(_.registerUsed(tree.symbol, Some(tree.name)))
unusedDataApply(_.registerUsed(prefix.classSymbol, None, Some(tree.srcPos)))
unusedDataApply(_.registerUsed(tree.symbol, Some(tree.name), Some(tree.srcPos)))
else if tree.hasType then
unusedDataApply(_.registerUsed(tree.tpe.classSymbol, Some(tree.name)))
unusedDataApply(_.registerUsed(tree.tpe.classSymbol, Some(tree.name), Some(tree.srcPos)))
else
ctx

override def prepareForSelect(tree: tpd.Select)(using Context): Context =
unusedDataApply(_.registerUsed(tree.symbol, Some(tree.name)))
unusedDataApply(_.registerUsed(tree.symbol, Some(tree.name), Some(tree.srcPos)))

override def prepareForBlock(tree: tpd.Block)(using Context): Context =
pushInBlockTemplatePackageDef(tree)
Expand All @@ -114,7 +113,7 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
ud.registerDef(tree)
if tree.name.mangledString.startsWith(nme.derived.mangledString + "$")
&& tree.typeOpt != NoType then
ud.registerUsed(tree.typeOpt.typeSymbol, None, true)
ud.registerUsed(tree.typeOpt.typeSymbol, None, Some(tree.srcPos), true)
ud.addIgnoredUsage(tree.symbol)
}

Expand Down Expand Up @@ -261,10 +260,10 @@ class CheckUnused private (phaseMode: CheckUnused.PhaseMode, suffix: String, _ke
/** This is a type traverser which catch some special Types not traversed by the term traverser above */
private def typeTraverser(dt: (UnusedData => Any) => Unit)(using Context) = new TypeTraverser:
override def traverse(tp: Type): Unit =
if tp.typeSymbol.exists then dt(_.registerUsed(tp.typeSymbol, Some(tp.typeSymbol.name)))
if tp.typeSymbol.exists then dt(_.registerUsed(tp.typeSymbol, Some(tp.typeSymbol.name), None))
tp match
case AnnotatedType(_, annot) =>
dt(_.registerUsed(annot.symbol, None))
dt(_.registerUsed(annot.symbol, None, None))
traverseChildren(tp)
case _ =>
traverseChildren(tp)
Expand Down Expand Up @@ -349,7 +348,7 @@ object CheckUnused:
*
* See the `isAccessibleAsIdent` extension method below in the file
*/
private val usedInScope = MutStack(MutSet[(Symbol,Boolean, Option[Name], Boolean)]())
private val usedInScope = MutStack(MutSet[(Symbol,Boolean, Option[Name], Boolean, Option[SrcPos])]())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make it a case class instead? This tuple got very long and hard to understand.

private val usedInPosition = MutSet[(SrcPos, Name)]()
/* unused import collected during traversal */
private val unusedImport = MutSet[ImportSelector]()
Expand Down Expand Up @@ -392,14 +391,14 @@ object CheckUnused:
* The optional name will be used to target the right import
* as the same element can be imported with different renaming
*/
def registerUsed(sym: Symbol, name: Option[Name], isDerived: Boolean = false)(using Context): Unit =
def registerUsed(sym: Symbol, name: Option[Name], srcPos: Option[SrcPos], isDerived: Boolean = false)(using Context): Unit =
if !isConstructorOfSynth(sym) && !doNotRegister(sym) then
if sym.isConstructor && sym.exists then
registerUsed(sym.owner, None) // constructor are "implicitly" imported with the class
registerUsed(sym.owner, None, srcPos) // constructor are "implicitly" imported with the class
else
usedInScope.top += ((sym, sym.isAccessibleAsIdent, name, isDerived))
usedInScope.top += ((sym.companionModule, sym.isAccessibleAsIdent, name, isDerived))
usedInScope.top += ((sym.companionClass, sym.isAccessibleAsIdent, name, isDerived))
usedInScope.top += ((sym, sym.isAccessibleAsIdent, name, isDerived, srcPos))
usedInScope.top += ((sym.companionModule, sym.isAccessibleAsIdent, name, isDerived, srcPos))
usedInScope.top += ((sym.companionClass, sym.isAccessibleAsIdent, name, isDerived, srcPos))
if sym.sourcePos.exists then
name.map(n => usedInPosition += ((sym.sourcePos, n)))

Expand Down Expand Up @@ -461,21 +460,24 @@ object CheckUnused:
val used = usedInScope.pop().toSet
// used imports in this scope
val imports = impInScope.pop()
val kept = used.filterNot { (sym, isAccessible, optName, isDerived) =>
val kept = used.filterNot { (sym, isAccessible, optName, isDerived, usageSymPosOpt) =>
// keep the symbol for outer scope, if it matches **no** import
// This is the first matching wildcard selector
var selWildCard: Option[ImportSelector] = None

val matchedExplicitImport = imports.exists { imp =>
sym.isInImport(imp, isAccessible, optName, isDerived) match
case None => false
case optSel@Some(sel) if sel.isWildcard =>
if selWildCard.isEmpty then selWildCard = optSel
// We keep wildcard symbol for the end as they have the least precedence
false
case Some(sel) =>
unusedImport -= sel
true
if usageSymPosOpt.map(pos => pos.isStartBefore(imp.srcPos)).getOrElse(false) then
false
else
sym.isInImport(imp, isAccessible, optName, isDerived) match
case None => false
case optSel@Some(sel) if sel.isWildcard =>
if selWildCard.isEmpty then selWildCard = optSel
// We keep wildcard symbol for the end as they have the least precedence
false
case Some(sel) =>
unusedImport -= sel
true
}
if !matchedExplicitImport && selWildCard.isDefined then
unusedImport -= selWildCard.get
Expand Down Expand Up @@ -761,6 +763,13 @@ object CheckUnused:
private def isWildcard: Boolean =
thisName == StdNames.nme.WILDCARD || thisName.is(WildcardParamName)

extension (thiz: SrcPos)
/** if `thiz` start position line (and then column) is before `that` */
private def isStartBefore(that: SrcPos)(using Context) =
(thiz.startPos.line < that.startPos.line) ||
(thiz.startPos.line == that.startPos.line && thiz.startPos.column <= that.startPos.column)


end UnusedData

private object UnusedData:
Expand Down
2 changes: 1 addition & 1 deletion tests/neg-custom-args/fatal-warnings/i15503a.scala
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ object InlineChecks:
inline def getSet = Set(1)

object InlinedBar:
import collection.mutable.Set // ok
import collection.mutable.Set // error
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szymon-rd
I was just wondering about this one, it seems like this import is not required. It can be removed and still get the correct result : https://scastie.scala-lang.org/9pyvzVWyTAyBsOPduePhqg
Am I missing something ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you are correct. Some of these tests lay out the expected false negatives. If this is not a false negative, then great.

import collection.mutable.Map // error
val a = InlineFoo.getSet

Expand Down
9 changes: 9 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i17667.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// scalac: -Wunused:imports

object MyImplicits:
extension (a: Int) def print: Unit = println(s"Hello, I am $a")

import MyImplicits.print // ok
object Foo:
def printInt(a: Int): Unit = a.print
import MyImplicits._ // error