fix: prevent external import map collisions (#2)#8
Conversation
) Replace unconditional dictionary assignment with RegisterExternalImportMapping that keeps the first registered entry and emits a warning diagnostic on collision. Add multi-library CompileConsumer overload and two collision tests. Closes #2
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses non-deterministic external import resolution caused by simple-name collisions across referenced assemblies by detecting collisions, keeping the first registered mapping (so locals win via registration order), and emitting an MS0003 warning when collisions occur.
Changes:
- Introduce
RegisterExternalImportMappingto prevent_externalImportMapoverwrites and emit anAmbiguousConstructwarning on collisions. - Add
CompileLibrary(source, assemblyName)andCompileConsumer(source, params compilations)overloads to support multi-library test scenarios. - Add new cross-package tests for local-vs-referenced and referenced-vs-referenced import collisions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| MetaSharp.Compiler.TypeScript/Transformation/TypeTransformer.cs | Prevents external import map collisions by registering first mapping and warning on conflicts. |
| MetaSharp.Tests/TranspileHelper.cs | Adds compilation helper overloads to build multiple distinct referenced libraries for tests. |
| MetaSharp.Tests/CrossPackageImportTests.cs | Adds tests covering collision scenarios and diagnostic emission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var consumer = """ | ||
| [assembly: TranspileAssembly] | ||
|
|
||
| public class App | ||
| { |
There was a problem hiding this comment.
This test is named as if it verifies the "keeps first mapping" behavior, but the consumer source never references Moment, so no import statement is emitted and the assertion only checks that a diagnostic exists. To actually validate the winning mapping, reference Moment in the consumer (e.g., as a property type/new instance) and assert the generated TS imports from moment-a (and not moment-b).
danfma
left a comment
danfma
left a comment
There was a problem hiding this comment.
Re: Copilot's comment on ReferencedImportCollision_EmitsDiagnosticAndKeepsFirstMapping — the first test (LocalImportMapping_WinsOverReferencedCollision) already validates that the winning mapping appears in the generated output and the loser doesn't. This second test focuses specifically on the referenced-vs-referenced scenario where the primary concern is collision detection — the diagnostic is the meaningful assertion here.
…Coverage (#3) Adds the Microsoft code coverage extension so tests can be run with the --coverage flag. Usage: dotnet run --project tests/MetaSharp.Tests/ -- \ --coverage --coverage-output-format cobertura \ --coverage-output coverage.cobertura.xml \ --results-directory TestResults Current baseline: 74.1% line coverage, 66.0% branch coverage. - MetaSharp.Compiler.TypeScript: 75.5% lines, 66.5% branches - MetaSharp.Compiler: 49.8% lines, 56.7% branches All four gaps listed in the issue have been closed by prior PRs: - [Name] on types/nested types: PR #7 (NameAttributeConsistencyTests) - GenerateGuard with fields: PR #6 - _externalImportMap collisions: PR #8 (RegisterExternalImportMapping) - Coverage tooling: this commit CLAUDE.md updated with the coverage command. Closes #3
Summary
_externalImportMap[key] = entrywithRegisterExternalImportMappingthat keeps the first registered entry and emits anAmbiguousConstructwarning on collisionCompileLibrary(source, assemblyName)andCompileConsumer(source, params compilations)overloads toTranspileHelperto support multi-library test scenariosTest plan
LocalImportMapping_WinsOverReferencedCollision— local[Import]wins, diagnostic emittedReferencedImportCollision_EmitsDiagnosticAndKeepsFirstMapping— first referenced library wins, diagnostic emittedCloses #2