Fix issue with duplicate filenames in MBT-based PC#8406
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| paths.zipWithIndex.map { case (p, i) => | ||
| if (i == 0) SourceFileEntryImpl(AbstractFile.getFile(p)) | ||
| else { | ||
| // AggregateClassPath deduplicates SourceFileEntry by filename, so duplicate-named |
There was a problem hiding this comment.
Could we instead implement our own AggregateClassPath that wouldn't do it? Basically duplicate it with the change logic?
There was a problem hiding this comment.
Done. I think it looks good, tested both via the unit test, and on the client's repo.
The presentation compiler puts the scala sources into AggregateClassPath object, which appears to be indexed by `(package, file_basename)`, so when we compile main/util/reporter.scala with test/util/reporter.scala (where both are actually put into the same package, just come from different modules/targets), the compiler loses track of one of those. We fix it by renaming the virtual file. This previously wasn't an issue, since before metals V2, we would not compile multiple sources at once, instead relying on classfiles.
| result += c | ||
| } | ||
|
|
||
| ClassPathEntries(pkgs, if (result.isEmpty) Nil else result.toIndexedSeq) |
There was a problem hiding this comment.
This if part looks a little weird, but it's a microoptimisation kept from the original AggregateClassPath. I doubt it makes much of a difference, but I decided to keep it (but I can be swayed).
There was a problem hiding this comment.
There is some optimization in the parent class like:
case ecp: EfficientClassPath => etc.
Is that not neccessary here? And it goes over each classpath.
There was a problem hiding this comment.
It previously didn't use the list() method, so we couldn't have that EfficientClassPath fast path, but now I changed it to be as similar to original implementation as possible, with added comments on where it differs. It should be slightly faster (1 walk through the class path instead of 3 + the mutable buffer benefits). The code is slightly blown out now though, but I added comments on where things differ compared to the parent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala (1)
577-608: ⚡ Quick winStrengthen this regression test with definition-location assertions.
Both hover expectations are identical (
val value: String), so this can still pass if both lookups resolve to the same symbol shape. AddassertDefinitionchecks forMainUtils.valueandTestUtils.valueto pin each reference to its respectivemain/src/util/Utils.scalaandtest/src/util/Utils.scala.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala` around lines 577 - 608, The hover checks currently only assert the symbol shape; add definition-location assertions to pin each reference to its correct source: after the first server.assertHover (the one with MainUtils.val@@ue) add a server.assertDefinition call targeting MainUtils.value and expect its definition to resolve to the main util implementation (the Utils in the main source), and after the second server.assertHover (the one with TestUtils.val@@ue) add a server.assertDefinition targeting TestUtils.value and expect its definition to resolve to the test util implementation (the Utils in the test source); use the existing server.assertDefinition helper (matching how other tests assert definitions) so the test validates both hover content and precise definition locations for MainUtils.value and TestUtils.value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala`:
- Around line 577-608: The hover checks currently only assert the symbol shape;
add definition-location assertions to pin each reference to its correct source:
after the first server.assertHover (the one with MainUtils.val@@ue) add a
server.assertDefinition call targeting MainUtils.value and expect its definition
to resolve to the main util implementation (the Utils in the main source), and
after the second server.assertHover (the one with TestUtils.val@@ue) add a
server.assertDefinition targeting TestUtils.value and expect its definition to
resolve to the test util implementation (the Utils in the test source); use the
existing server.assertDefinition helper (matching how other tests assert
definitions) so the test validates both hover content and precise definition
locations for MainUtils.value and TestUtils.value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c009c61-1fc8-4154-8bd1-daf725a323a0
📒 Files selected for processing (5)
mtags/src/main/scala-2.12/scala/meta/internal/pc/classpath/MtagsPathResolver.scalamtags/src/main/scala-2.13/scala/meta/internal/pc/classpath/MtagsPathResolver.scalamtags/src/main/scala-2/scala/tools/nsc/LogicalSourcePath.scalamtags/src/main/scala-2/scala/tools/nsc/classpath/MetalsAggregateClassPath.scalatests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
LGTM
One question from me otherwise.
| result += c | ||
| } | ||
|
|
||
| ClassPathEntries(pkgs, if (result.isEmpty) Nil else result.toIndexedSeq) |
There was a problem hiding this comment.
There is some optimization in the parent class like:
case ecp: EfficientClassPath => etc.
Is that not neccessary here? And it goes over each classpath.
3 participants
The presentation compiler puts the scala sources into AggregateClassPath object, which appears to be indexed by
(package, file_basename)(slight simplification here), so when we compile main/util/reporter.scala with test/util/reporter.scala (where both are actually put into the same package, just come from different modules/targets), the compiler loses track of one of those. We fix it byrenaming the virtual file.reimplementing the problematic parts of AggregateClassPath.This previously wasn't an issue, since before metals V2, we would not compile multiple sources at once, instead relying on classfiles.
Summary by CodeRabbit
Improvements
Tests