Load only assemblies from closure. - #11965
Conversation
commit: |
There was a problem hiding this comment.
🔵 Needs a closer look
Unbounded dependency ranges may be omitted from the closure, preventing valid external types from loading.
Pull request overview
Limits NuGet assembly resolution to the project's dependency closure.
Changes:
- Removes fallback probing of arbitrary cached package versions.
- Restricts assembly lookup to closure-recorded versions.
File summaries
| File | Summary |
|---|---|
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetAssemblyResolver.cs |
Restricts probing to dependency-closure assemblies; unbounded dependency ranges still require handling. |
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetAssemblyResolver.cs:359
WalkPackageonly records dependencies whenVersionRange.MinVersionis non-null (lines 214-220), so a nuspec dependency declared without a lower bound (<dependency id="..." />or*) is omitted even though it is part of the package closure. After this fallback is deleted, the resolver never probes that dependency and a valid external type whose base/interface is in that package fails to load; derive the closure from the project's resolved assets (or otherwise pin unbounded ranges to their selected versions) before enforcing closure-only probing.
This issue also appears on line 356 of the same file.
if (assemblyPath == null)
{
_debug($"Could not locate dependency assembly '{simpleName}' under '{_globalPackagesFolder}'.");
return null;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetAssemblyResolver.cs:359
- With this fallback removed,
Resolvenow returns before reading a cached assembly unlessRegisterPackageClosurehas populated_closureVersions. The existingNugetAssemblyResolverTests.Resolve_ReturnsNullAndLogsWhenAssemblyCannotBeLoadedconstructs the resolver without registering a closure and expects the invalid cached file to be loaded far enough to logFailed to load dependency assembly; this now logsCould not locate dependency assemblyand fails. Update that test/call contract and add explicit closure-only coverage in this PR.
if (assemblyPath == null)
{
_debug($"Could not locate dependency assembly '{simpleName}' under '{_globalPackagesFolder}'.");
return null;
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No changes needing a change description found. |
0c1af9d to
e8abcd1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved closure/version handling issues and a failing test setup block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetAssemblyResolver.cs:359
- Removing the fallback exposes that
RegisterPackageClosurerecords each dependency'sVersionRange.MinVersion(seeNugetAssemblyResolver.cs:214-220), not the version NuGet actually selected inproject.assets.json. For a dependency declared as>= 1.0.0but resolved by the project to 2.0.0,FindPackageAssemblyInVersionnow probes only 1.0.0 and returns null when that version is absent, so valid external types fail to load. Build the closure from the resolved asset graph (or otherwise constrain probing to the selected project version) before removing this fallback, and cover a minimum-version-below-selected-version case.
if (assemblyPath == null)
{
_debug($"Could not locate dependency assembly '{simpleName}' under '{_globalPackagesFolder}'.");
return null;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetAssemblyResolver.cs:359
- After removing the fallback,
Probecan resolve a package only when_closureVersionscontains its simple name. However,WalkPackageonly adds dependencies whoseVersionRange.MinVersionis non-null (NugetAssemblyResolver.cs:216-220), so a valid unbounded nuspec dependency such as<dependency id="B" />is omitted even when NuGet restored B in the project's closure. External types inheriting from or implementing a type in B will now fail to load; the closure needs to be populated from the resolved project assets (or otherwise retain unbounded dependencies) before this fallback is removed.
if (assemblyPath == null)
{
_debug($"Could not locate dependency assembly '{simpleName}' under '{_globalPackagesFolder}'.");
return null;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
e8abcd1 to
f253682
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The resolver can fail to locate packages when the selected version differs from the dependency range minimum.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
During package generation the NuGetPackage resolver is trying to load all available packages, including the incompatible. In this PR we are limiting loading packages to the project dependencies.
This problem results in cryptic failure if NuGet cache folder contains incompatible packages. Here is an example
This error is machine specific, because on different environment there was single issue in the attempt to load System.Range from assembly 'Microsoft.CodeAnalysis.Workspaces, Version=4.8.0.0.
In all the cases the issue happens while AdhocWorkspace creation. It creates DefaultHostServices, which loads the assemblies, resulting in crash above.