Fix compilation breaking due to collisions in Scalafix symbol replacements #2260
Conversation
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
Signed-off-by: subhramit <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a compilation issue where Scalafix symbol replacements would cause naming collisions with existing imports. The fix implements collision detection by pre-computing a map of globally imported symbols for O(1) lookup performance.
- Adds collision detection to prevent import conflicts when replacing symbols
- Uses fully qualified names when collisions are detected instead of adding conflicting imports
- Optimizes collision checking from O(n) to O(1) by pre-computing global import mappings
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ReplaceSymbolOps.scala | Core logic implementing collision detection and import info extraction |
| test/ReplaceSymbol.scala (input) | Test case adding TreeMap symbol replacement scenario |
| test/ReplaceSymbol.scala (output) | Expected output showing collision handling with fully qualified name |
| com/geirsson/immutable.scala | New test file providing the replacement symbol definition |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| 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.
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.
| addImport + ctx.replaceTree(n, to.signature.name) | |
| patchForCollision(n, to, addImport, causesCollision) |
Copilot uses AI. Check for mistakes.
| else ctx.addGlobalImport(to) | ||
| addImport + ctx.replaceTree(n, to.signature.name) | ||
| if (causesCollision) | ||
| addImport + ctx.replaceTree(n, to.owner.syntax + to.signature.name) |
There was a problem hiding this comment.
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.
| addImport + ctx.replaceTree(n, to.owner.syntax + to.signature.name) | |
| addImport + ctx.replaceTree(n, to.owner.syntax + "." + to.signature.name) |
Copilot uses AI. Check for mistakes.
|
Hey @subhramit, thanks for reviving that and for your patience - as you noticed, I didn't have much free time to review, but I expect to be able to look at that this week or next one. I just ran copilot as an experiment, the comments might be totally off of course. |
Hey @bjaglin, that's totally fine. I understand and appreciate however much energy maintainers are able to put into open source projects. It's a free time endeavour for most of us. |
2 participants
Fixes #1305
Supersedes #1695
I attempted to finish the PR. It is not perfect, but I tried taking into account reviews that were "simple" to tackle such as #1695 (comment), #1695 (comment) and #1695 (comment), and also implementing the collision detection in O(1) by pre-computing the map for global imported symbols, instead of O(n) lookup for each symbol replacement during collision checking.