Repository navigation
Resolved the loaded assemblies not being concidered by the assembly resolver - #76
Conversation
Resolved the issue where the assemblies were not being populated
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes behavior of a process-wide assembly resolver that runs inside compiler servers/IDEs and re-enables a previously dead code path that calls Assembly.Load, whose runtime impact is not fully exercised by the existing tests and warrants human sign-off.
Review effort: Balanced
Findings: 1
What changed in this PR
This PR fixes a latent bug in the generated assembly-resolution hoist (SourceGeneratorHoistBase.cs template, emitted as SgfSourceGeneratorHoist.g.cs). The s_assembliesWithResources collection, which the process-wide AssemblyResolve handler iterates as a fallback to extract embedded dependency assemblies, was never populated — so that fallback branch was effectively dead. The PR adds the missing Add call and hardens the shared collections against concurrent access from the AppDomain AssemblyLoad/AssemblyResolve events.
Changes:
- Populate
s_assembliesWithResourcesfor assemblies that carrySGF.Assembly::resources, so the resolver's fallback extraction path actually runs. - Switch
s_assembliesWithResources/s_loadedAssembliestoConcurrentBag/ConcurrentDictionaryand useTryAddfor atomic insert-or-skip, making the resolver safe under concurrent assembly-load events. - Add an explanatory comment documenting the fallback intent.
| File | Description |
|---|---|
| src/SourceGenerator.Foundations/Templates/SourceGeneratorHoistBase.cs | Adds the missing s_assembliesWithResources.Add, moves the loaded/resource collections to concurrent types, and uses TryAdd in AddAssembly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // remember assemblies that contain embedded resources so the resolver can | ||
| // search them later as a fallback | ||
| s_assembliesWithResources.Add(assembly); |
…urce once With s_assembliesWithResources populated, the resource fallback runs, and it had two problems that the lookup above it does not: - It returned whatever it extracted, regardless of version: a request for 2.0.0.0 got the 1.0.0.0 copy. That is what fails Request_For_A_Newer_Version_Does_Not_Resolve_To_An_Older_One in CI. - Assembly.Load(byte[]) loads a new copy on every call and none unloads, so every unsatisfied request added another copy of an assembly that was already loaded. The fallback now skips a copy that does not satisfy the request (same Satisfies rule as the lookup), and a resource is unpacked at most once, whether by AddAssembly or by the fallback (s_extractedResources). Also removes the unused 'alreadyLoaded' local (CS0168). Tests: an unsatisfied request does not load the assembly again; the fallback unpacks a resource AddAssembly has not reached yet (the window another generator initialising on another thread can hit), and does not hand back an older version from there. Each fails with its guard removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for picking this up so quickly! The CI failure here comes from the version rule in #74 meeting the fallback this PR brings to life. The fallback returns whatever it extracts, so a request for 2.0.0.0 gets the 1.0.0.0 copy. It also extracts again on every unsatisfied request, and I opened #77 against this branch. It applies the same version rule in the fallback, unpacks each resource at most once, and drops the unused |
Apply the version rule to the resource fallback (fixes CI on #76)
|
What a hero. I was doing this between flights but your fix is less work for me. Thanks |
Resolved the issue where the assemblies were not being populated due to the array never adding assemblies to it.