-
Notifications
You must be signed in to change notification settings - Fork 188
Fix compilation breaking due to collisions in Scalafix symbol replacements #2260
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
Open
subhramit
wants to merge
13
commits into
scalacenter:main
Choose a base branch
from
subhramit:name-collision
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0d96c1e
Build solution - without scope awareness
subhramit d960708
Add implicit `SemanticDocument`
subhramit 125fbc5
Use `SemanticdbIndex` instead
subhramit a8e39db
Better name for method
subhramit 2de7be1
Simpler concat
subhramit 12569bb
Revert IDE formatting
subhramit 100b6d4
Revert IDE formatting
subhramit 26b2f88
scalafmt
subhramit e0aefe8
Fix test
subhramit 97dd899
scalafixall
subhramit aac4d5d
Fix scope issue
subhramit 3374091
Remove optional assignment
subhramit 85cd8bc
IntelliJ rename - method name `getGlobalImports`
subhramit File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,7 @@ | ||||||
package scalafix.internal.patch | ||||||
|
||||||
import scala.annotation.tailrec | ||||||
|
||||||
import scala.meta._ | ||||||
import scala.meta.internal.trees._ | ||||||
|
||||||
|
@@ -12,6 +14,11 @@ import scalafix.syntax._ | |||||
import scalafix.v0._ | ||||||
|
||||||
object ReplaceSymbolOps { | ||||||
private case class ImportInfo( | ||||||
globalImports: Seq[Import], | ||||||
globalImportedSymbols: Map[String, Symbol] | ||||||
) | ||||||
|
||||||
private object Select { | ||||||
def unapply(arg: Ref): Option[(Ref, Name)] = arg match { | ||||||
case Term.Select(a: Ref, b) => Some(a -> b) | ||||||
|
@@ -20,10 +27,47 @@ object ReplaceSymbolOps { | |||||
} | ||||||
} | ||||||
|
||||||
private def extractImports(stats: Seq[Stat]): Seq[Import] = { | ||||||
stats.collect { case i: Import => i } | ||||||
} | ||||||
|
||||||
private def extractImportInfo( | ||||||
tree: Tree | ||||||
)(implicit index: SemanticdbIndex): ImportInfo = { | ||||||
@tailrec | ||||||
def getGlobalImports(ast: Tree): Seq[Import] = ast match { | ||||||
case Pkg(_, Seq(pkg: Pkg)) => getGlobalImports(pkg) | ||||||
case Source(Seq(pkg: Pkg)) => getGlobalImports(pkg) | ||||||
case Pkg(_, stats) => extractImports(stats) | ||||||
case Source(stats) => extractImports(stats) | ||||||
case _ => Nil | ||||||
} | ||||||
|
||||||
val globalImports = getGlobalImports(tree) | ||||||
|
||||||
// pre-compute global imported symbols for O(1) collision detection | ||||||
// since ctx.addGlobalImport adds imports at global scope | ||||||
val globalImportedSymbols = globalImports.flatMap { importStat => | ||||||
importStat.importers.flatMap { importer => | ||||||
importer.importees.collect { | ||||||
case Importee.Name(name) => | ||||||
name.value -> name.symbol.getOrElse(Symbol.None) | ||||||
case Importee.Rename(_, rename) => | ||||||
rename.value -> rename.symbol.getOrElse(Symbol.None) | ||||||
} | ||||||
} | ||||||
}.toMap | ||||||
|
||||||
ImportInfo(globalImports, globalImportedSymbols) | ||||||
} | ||||||
|
||||||
def naiveMoveSymbolPatch( | ||||||
moveSymbols: Seq[ReplaceSymbol] | ||||||
)(implicit ctx: RuleCtx, index: SemanticdbIndex): Patch = { | ||||||
if (moveSymbols.isEmpty) return Patch.empty | ||||||
|
||||||
val importInfo = extractImportInfo(ctx.tree)(index) | ||||||
|
||||||
val moves: Map[String, Symbol.Global] = | ||||||
moveSymbols.iterator.flatMap { | ||||||
case ReplaceSymbol( | ||||||
|
@@ -126,10 +170,15 @@ object ReplaceSymbolOps { | |||||
if sig.name != parent.value => | ||||||
Patch.empty // do nothing because it was a renamed symbol | ||||||
case Some(_) => | ||||||
val causesCollision = | ||||||
importInfo.globalImportedSymbols.contains(to.signature.name) | ||||||
val addImport = | ||||||
if (n.isDefinition) Patch.empty | ||||||
if (n.isDefinition || causesCollision) Patch.empty | ||||||
else ctx.addGlobalImport(to) | ||||||
addImport + ctx.replaceTree(n, to.signature.name) | ||||||
if (causesCollision) | ||||||
addImport + ctx.replaceTree(n, to.owner.syntax + to.signature.name) | ||||||
else | ||||||
addImport + ctx.replaceTree(n, to.signature.name) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The collision handling logic creates duplicated code paths. Consider extracting the replacement logic into a helper function to reduce duplication between the collision and non-collision cases.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
case _ => | ||||||
Patch.empty | ||||||
} | ||||||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 11 additions & 0 deletions
11
scalafix-tests/output/src/main/scala/com/geirsson/immutable.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package com.geirsson | ||
|
||
import scala.collection.immutable.TreeMap | ||
|
||
object immutable { | ||
type SortedMap[A, B] = TreeMap[A, B] | ||
|
||
object SortedMap { | ||
def empty[A : Ordering, B]: SortedMap[A, B] = TreeMap.empty[A, B] | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String concatenation for building qualified names could be error-prone. Consider using a more robust method like
to.owner.syntax + "." + to.signature.name
or a dedicated method to ensure proper formatting.Copilot uses AI. Check for mistakes.