Fix resolution of the external dependencies. - #11742
Fix resolution of the external dependencies.#11742Nikolay Rovinskiy (nick863) wants to merge 26 commits into
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
There was a problem hiding this comment.
Pull request overview
Updates the C# generator’s external NuGet dependency resolution so it can (when needed) fall back to the latest available version (optionally including prereleases) instead of only using a requested minimum version or whatever is already cached.
Changes:
- Added a helper to enumerate available package versions across enabled NuGet sources.
- Updated external type resolution to select a version based on
MinVersionpresence and prerelease status before downloading.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetPackageResolver.cs | Adds GetAllVersions helper for collecting versions from enabled NuGet sources. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs | Uses version enumeration to choose a download version when the requested MinVersion isn’t available and to include prereleases when appropriate. |
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:260
new NuGetVersion(external.MinVersion)will throw for an invalid/unsupported version string, which changes behavior compared to the previous string-based flow and ends up being swallowed by the broad catch (reported as "package not found"). Also,versions.Max()throws on an empty sequence, so missing packages/feeds can trigger an exception and skip the intended fallback selection.
NuGetVersion minVersion = new(external.MinVersion);
IList<NuGetVersion> versions = await NugetPackageResolver.GetAllVersions(external.Package!, nugetSettings, allowPrerelease: minVersion.IsPrerelease);
if (versions.Any(x => x == minVersion))
{
resolvedVersion = external.MinVersion;
}
else
{
resolvedVersion = versions.Max()?.ToString();
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No changes needing a change description found. |
|
The resolved version should be based on the csproj/central package management. The MinVersion from the decorator is only meant to be used as a safety check - it doesn't influence the version that is used, but it can trigger an error if the min version doesn't align with the resolved version. Can you clarify what issue this is solving? |
I have updated the description. |
The minVersion should not be used to influence the version that is downloaded by the generator. It is only meant to be used as a compatibility floor. It is optional - it doesn't have to be specified at all. I'm not sure what problem this is solving. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:260
- If the configured feeds return no versions (e.g., package doesn't exist, or only prerelease versions exist but
allowPrereleaseis false),versions.Max()will throw on an empty sequence and the resolver will fall into the catch path. Handle the empty list explicitly so resolution can fail cleanly without relying on exceptions.
NuGetVersion minVersion = new(external.MinVersion);
IList<NuGetVersion> versions = await NugetPackageResolver.GetAllVersions(external.Package!, nugetSettings, allowPrerelease: minVersion.IsPrerelease);
if (versions.Any(x => x == minVersion))
{
resolvedVersion = external.MinVersion;
}
else
{
resolvedVersion = versions.Max()?.ToString();
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:266
- New behavior adds multiple version-selection branches (minVersion present + exact version missing -> pick latest; prerelease minVersion -> include prerelease; minVersion absent -> latest stable). There are existing unit tests for ExternalTypeReferenceResolver, but none cover these new branches. Add tests that exercise: (1) minVersion not in feed selects latest available version; (2) prerelease minVersion allows selecting a prerelease latest; (3) stable minVersion does not select prerelease when only prerelease versions exist.
if (!string.IsNullOrEmpty(external.MinVersion))
{
// If min version was provided, we
// 1. Search if it is in our repositories;
// 2. Get the latest one if it is not.
// 3. If our version is a pre release, include pre released versions in our search.
NuGetVersion minVersion = new(external.MinVersion);
IList<NuGetVersion> versions = await NugetPackageResolver.GetAllVersions(external.Package!, nugetSettings, allowPrerelease: minVersion.IsPrerelease);
if (versions.Any(x => x == minVersion))
{
resolvedVersion = external.MinVersion;
}
else
{
resolvedVersion = versions.Max()?.ToString();
}
}
else
{
// If min version was not provided, get the latest stable version.
resolvedVersion = await NugetPackageResolver.ResolveLatestPackageVersion(external.Package!, nugetSettings);
}
The problem is that we did not released the new stable version yet, while the downloaded version is 2.0.0. The logic in |
|
Thanks, that clarifies the reproduction. I think the root fix should be in project-reference resolution rather than selecting a package version from
With that flow, the reported case resolves the project's --generated by Copilot |
f530a05 to
970c165
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:295
- Waiting for the process to exit before draining either redirected stream can deadlock once
dotnet restorefills an OS pipe buffer. This codebase already handles the same failure mode by reading stdout and stderr concurrently inGeneratorHandler.ReadProcessOutput(lines 340-346); start both reads before awaiting process exit here as well.
if (restore.Start())
{
await restore.WaitForExitAsync();
if (restore.ExitCode != 0)
{
string output = await restore.StandardOutput.ReadToEndAsync();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:697
- NuGet's global-packages layout uses the normalized lowercase package ID for the manifest filename (
test/common/FakeNuGetPackage.cs:111). With names such asFirst.Package, this writesFirst.Package.nuspec; on case-sensitive systemsNuGetv3LocalRepository.FindPackagecannot find the fake package, so the exact-version tests fail. Normalize this filename too.
File.WriteAllText(Path.Combine(metadataPath, $"{packageName}.nuspec"), $"""
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:477
- This local is never used, so it emits CS0219. Because
generator/Directory.Build.props:29treats warnings as errors, the test project will not compile; remove it.
var nugetCacheDir = Path.Combine(_tempDirectory!, "NuGetCache");
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:180
- This fake package declares a real Azure.Core dependency even though the test only needs the package DLL. Since
NUGET_PACKAGESpoints to an empty temporary cache and the production path now runsdotnet restore, this forces the unit test to contact a NuGet feed and fail in offline/restricted environments. Keep the fake package dependency-free, astest/common/FakeNuGetPackage.cs:90-111does when no dependencies are requested.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:715
- Every package created by this helper gains a real Azure.Core dependency. The tests now execute
dotnet restoreagainst an otherwise empty temporary cache, making them network-dependent even though none uses Azure.Core. Keep these fake packages dependency-free, matchingtest/common/FakeNuGetPackage.cs:90-111when no dependencies are requested.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:237
- Removing the download fallback makes package-backed external types fail on a clean machine whenever the package is not already represented by an existing
.csproj.CSharpGen.ExecuteAsyncinvokes this resolver before new-project scaffolding writes the project (CSharpGen.cs:34-42,165-168), soAddPackageReferencesFromProjectreturns early and nothing populates the cache. Keep a feed-resolution/download path driven byInputExternalTypeMetadata(including the stable/prerelease fallback rules), rather than relying exclusively on project restore.
string? assemblyPath = NugetPackageResolver.FindPackageAssembly(
globalPackagesFolder, external.Package!, external.MinVersion);
if (assemblyPath == null || !File.Exists(assemblyPath))
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:372
- Parsing framework versions as
doubledoes not preserve TFM ordering. For example, supported TFMsnet48andnet472become0.48and4.72, sonet472is incorrectly selected as newer; multi-digit minor versions similarly misorder. Parse TFMs withNuGetFramework.ParseFolder/Versionsemantics and apply the intended framework-family precedence explicitly.
double current = 0.0;
Match numeral = Regex.Match(name, "\\d+[.]*\\d*$");
if (numeral.Success)
{
current = double.Parse(numeral.Value);
}
if (name.StartsWith("net4", StringComparison.InvariantCultureIgnoreCase))
{
current /= 100;
current += 2000.0;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:284
projectFileDependencyGroupscontains the declared dependency constraint, not the version NuGet actually selected. For the reported case it remainsPackage >= 3.0.0-alpha...even when restore falls forward to another version, so this method later looks for the unavailable minimum and discards the successfully restored package. Read the resolved package version from the selectedtargetsgraph (or use NuGet's lock-file model) while using this section only to identify direct dependencies.
if (prop.Value.ValueKind == JsonValueKind.Object && prop.NameEquals("projectFileDependencyGroups"))
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:479
FindPackageAssemblyInVersiondeliberately chooses the asset nearest to the generator runtime (see NugetPackageResolver.cs:117-126), not the project target. Even for anetstandard2.0-only project this can register a package'snet8.0assembly, causing custom code to type-check against APIs unavailable in the generated project's target. Preserve the selected assets target and resolve the exact package version's assembly for that TFM (or add a separate exact compile-time lookup).
string? resolvedAssemblyPath = version is null
? NugetPackageResolver.FindPackageAssembly(globalPackagesFolder, refPackageName)
: NugetPackageResolver.FindPackageAssemblyInVersion(globalPackagesFolder, refPackageName, version);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:21
- Removing the feed fallback makes this resolver cache-only, so it cannot implement the PR's stated missing-version behavior.
AddPackageReferencesFromProjectrestores only packages already present as<PackageReference>items; a package supplied solely byInputExternalTypeMetadatais not added there, andCSharpGeninvokes this resolver separately. Consequently the example package remains unresolved when absent from the cache, and the described stable/prerelease fallback never runs. The download/version-selection logic needs to remain in this resolver (including prerelease-aware latest-version selection).
/// looking up the package in the NuGet global cache and loading the assembly via reflection.
/// Used by <c>TypeFactory.CreateExternalType</c>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:305
projectFileDependencyGroupscontains the declared dependency constraint, not the version NuGet actually selected. If3.5.0is unavailable and restore resolves3.6.0, this value remainsPackage >= 3.5.0; the later exactFindPackageAssemblyInVersion(..., "3.5.0")lookup therefore misses the package that was just restored. Read the selected package identity/version from the chosen target (or NuGet's lock-file model) while using this group only to identify direct dependencies.
string[] packageVersionRelation = (packageAndVersion.GetString() ?? "").Split();
// We only support the greater-than-or-equal relation.
// Example: "My.Package >= 1.1.1"
if (packageVersionRelation.Length == 3 && string.Equals(packageVersionRelation[1], ">="))
{
hshFrameworks[currentFramework.Framework][packageVersionRelation[0].ToLower()] = packageVersionRelation[2];
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:180
- This fake assembly does not reference
Azure.Core, but its new nuspec declares that real dependency. Because the test isolatesNUGET_PACKAGESand the production path now runsdotnet restore, the test must contact a feed forAzure.Coreand may fail—or bypass the assets-file path—offline. Keep the fake package hermetic by declaring no dependencies, as the shared helper does intest/common/FakeNuGetPackage.cs:84-126.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:715
- Every package produced by this helper now declares an unused real
Azure.Coredependency. Since these tests use an isolated package cache and calldotnet restore, that makes their success depend on remote feed availability instead of remaining hermetic. Use an empty dependency set (or the existingFakeNuGetPackage.Createhelper intest/common/FakeNuGetPackage.cs:41-76).
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
There was a problem hiding this comment.
🟡 Changes recommended
The fallback behavior is incomplete, first-time generation can no longer resolve external packages, and several new tests are invalid or non-hermetic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:261
- This branch returns immediately when the project's resolved version is below
MinVersion, which contradicts the stated fallback sequence: it should first tryMinVersion, then the latest eligible version, and finally an already available version. The newTryResolve_ReturnsNullForHigherMinVersiontest currently codifies the opposite behavior. Keep the NuGet download/version-selection fallback here instead of caching a terminal failure.
if (!versionAcceptable)
{
var versionQualifier = string.IsNullOrEmpty(external.MinVersion)
? string.Empty
: $"(>= {external.MinVersion})";
return CacheResult(state, key, new ResolutionResult(
null,
$"The package '{external.Package}' minimal version declared in a typespec {versionQualifier} is higher then the one defined in project dependencies \"{packageInfo.PackageVersion}\"."));
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:271
- Resolution now fails whenever the package was not already added from the project. This breaks first-time
--new-projectgeneration:AddPackageReferencesFromProjectreturns because the csproj does not exist yet, andCSharpGenonly writes that csproj at the end of generation (lines 165-168). Preserve a direct NuGet resolution fallback based onexternal.Package/MinVersionbefore reporting the package missing.
if (packageInfo.AssemblyPath == null || !File.Exists(packageInfo.AssemblyPath) || !versionAcceptable)
{
var versionQualifier = string.IsNullOrEmpty(external.MinVersion)
? string.Empty
: $" (>= {external.MinVersion})";
return CacheResult(state, key, new ResolutionResult(
null,
$"package '{external.Package}'{versionQualifier} is not present in package dependencies."));
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:186
- This fake package now depends on a real Azure.Core version even though its emitted source does not use Azure.Core. Since the test switches
NUGET_PACKAGESto a fresh directory and then runsdotnet restore, it must contact external feeds, making the unit test network/version dependent. Use the hermeticFakeNuGetPackage.Createpattern (test/common/FakeNuGetPackage.cs:41-76) with a locally created fake dependency, or omit this dependency.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:816
- This duplicated fake nuspec also introduces a real Azure.Core dependency into tests that run
dotnet restoreagainst a fresh package cache. That makes these unit cases depend on external feed availability. Followtest/common/FakeNuGetPackage.cs:41-76and create all needed packages locally, or remove the unused dependency declaration.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:792
- NuGet's global-packages layout uses the normalized lowercase package ID for the nuspec filename. Writing
First.Package.nuspecon a case-sensitive filesystem meansNuGetv3LocalRepository.FindPackagemay not recognize this fake package, causing the new restore-based tests to fail on Linux. Normalize the filename as the sharedFakeNuGetPackagehelper does.
File.WriteAllText(Path.Combine(metadataPath, $"{packageName}.nuspec"), $"""
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The documented package fallback behavior is not implemented, cache-only resolution regresses, and several tests use dependencies unavailable to their isolated NuGet source.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:261
- Returning here directly contradicts the PR's stated fallback behavior. When the project resolves a version below
MinVersion, this rejects the external type without trying the requested version or the latest qualifying version (including prereleases); the newTryResolve_ReturnsNullForHigherMinVersiontest codifies that incorrect outcome. Resolve/download the fallback version before reporting failure.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:261 - Correct the comparison phrase from “higher then” to “higher than.”
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/ExternalTypeReferenceResolverTests.cs:212 - Correct “onees” to “ones.”
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:245
- This now searches only references populated from the project's
project.assets.json. External types are allowed to name a package that is not already aPackageReference; for example,ModelProviderTests.ExternalTypePropertyResolvedFromNuGetCachecreates only a cached package and callsResolveAllAsync, so it will now fail even though the assembly is available. Retain the cache/feed lookup as a fallback and register the resolved assembly rather than requiring every external package to already be a project dependency.
(string AssemblyPath, string PackageVersion) packageInfo = CodeModelGenerator.Instance.AdditionalMetadataReferences
.Where(x => x.Properties.Kind == MetadataImageKind.Assembly
&& x.Display is not null
&& x.Display.Contains(packageFolder, StringComparison.InvariantCultureIgnoreCase)
&& x.Display.Substring(x.Display.LastIndexOf(packageFolder, StringComparison.InvariantCultureIgnoreCase)).Split(Path.DirectorySeparatorChar).Length > 2)
.Select(x => x.Display ?? "")
.Select(x => (AssemblyPath: x, PackageVersion: x.Substring(x.LastIndexOf(packageFolder, StringComparison.InvariantCultureIgnoreCase)).Split(Path.DirectorySeparatorChar)[2]))
.FirstOrDefault();
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:204
- This fake package declares
Azure.Core1.61.0, but the fixture clears all feeds and pointsNUGET_PACKAGESat a fresh temporary cache that never contains Azure.Core. Consequentlydotnet restorecannot succeed, so this test can pass while exercising a failed/partial restore instead of validating dependency loading. Add a hermetic fake dependency or remove this undeployable dependency.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:838
- Every package produced by this helper depends on
Azure.Core1.61.0, but the fixture's only configured source/cache is the fresh temporary directory and no Azure.Core package is created there. All restores using this helper therefore fail, which leaves the assertions validating partial/stale assets rather than the successful restore path. Use the sharedFakeNuGetPackage.Createhelper with explicit fake dependencies, or emit packages with no dependency.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:330
- Use invariant casing for NuGet package IDs. Culture-sensitive
ToLower()can create keys that do not match the invariant lowercase cache path on systems such as Turkish locales.
string packageName = packageVersionRelation[0].ToLower();
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/ExternalTypeReferenceResolverTests.cs:104
- Because the generated project is explicitly pinned to
highestVersion, the 1.0.0 and 2.5.0 cache entries cannot affect the result, so this no longer tests selecting the highest version at or aboveMinVersion. It also leaves the required prerelease fallback untested. Add cases where the requested minimum is unavailable and verify stable versus prerelease fallback selection.
await CreateProjectAndLoadDependencies([pkgName], [highestVersion]);
var external = new InputExternalTypeMetadata(typeName, pkgName, "2.0.0");
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Package-name lowercasing breaks Linux resolution, and the documented minimum-version fallback remains unimplemented.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:261
- This rejection leaves the motivating scenario unresolved.
AddPackageReferencesFromProjectonly restores the version selected by the project, and the download/query logic based onexternal.MinVersionwas removed, so an older project dependency causes an immediate failure even when the requested minimum or a newer prerelease is available (the newTryResolve_ReturnsNullForHigherMinVersiontest codifies that opposite behavior). Before returning, resolve the requested minimum, then the latest eligible stable/prerelease version as described by the PR, and finally fall back to a cached version; add a prerelease case covering that policy.
if (!versionAcceptable)
{
var versionQualifier = string.IsNullOrEmpty(external.MinVersion)
? string.Empty
: $"(>= {external.MinVersion})";
return CacheResult(state, key, new ResolutionResult(
null,
$"The package '{external.Package}' minimal version declared in a typespec {versionQualifier} is higher then the one defined in project dependencies \"{packageInfo.PackageVersion}\"."));
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
External packages unavailable through an existing project reference no longer use the promised cache/feed fallback, and several tests restore invalid fake dependency graphs.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:261
- The user-facing failure reason should say “higher than,” not “higher then”; “minimum version” and “TypeSpec” also make the sentence accurate and consistent.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs:330
- Package IDs are case-insensitive and are normalized with
ToLowerInvariant()elsewhere in this method. Culture-sensitiveToLower()can produce a different key under locales such as Turkish, causing dependency lookup to miss the package parsed fromtargets.
string packageName = packageVersionRelation[0].ToLower();
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:204
- The test config clears every feed except the isolated fake cache, but this fake package declares Azure.Core 1.61.0 without placing that dependency in the cache. Consequently
dotnet restorefails and this test exercises the post-failure/stale-assets path rather than a successful package restore. Remove the undeclared dependency (or create it explicitly).
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:245
- This now resolves external types only from assemblies already added from
project.assets.json, so the advertised MinVersion/latest/cache fallback is lost. In particular, new-project generation calls this before writing the.csproj(CSharpGen.cs:34-42versus165-168), leaving no metadata references to search and causing every package-backed alternate type to fail. Existing projects without an explicit PackageReference regress similarly. Please retain a direct cache/feed fallback based onexternal.MinVersionwhen no suitable project reference is found (including prerelease-aware latest selection).
(string AssemblyPath, string PackageVersion) packageInfo = CodeModelGenerator.Instance.AdditionalMetadataReferences
.Where(x => x.Properties.Kind == MetadataImageKind.Assembly
&& x.Display is not null
&& x.Display.Contains(packageFolder, StringComparison.InvariantCultureIgnoreCase)
&& x.Display.Substring(x.Display.LastIndexOf(packageFolder, StringComparison.InvariantCultureIgnoreCase)).Split(Path.DirectorySeparatorChar).Length > 2)
.Select(x => x.Display ?? "")
.Select(x => (AssemblyPath: x, PackageVersion: x.Substring(x.LastIndexOf(packageFolder, StringComparison.InvariantCultureIgnoreCase)).Split(Path.DirectorySeparatorChar)[2]))
.FirstOrDefault();
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:838
- Every package produced by this helper declares Azure.Core 1.61.0, but the fixture's only source is its isolated cache and the helper never creates Azure.Core. All restores using this helper therefore fail, which prevents these tests from validating the successful restore path. Keep the dependency groups empty unless a test explicitly creates the dependency.
<dependencies>
<group targetFramework="net10.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="Azure.Core" version="1.61.0" exclude="Build,Analyzers" />
</group>
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/GeneratedCodeWorkspaceTests.cs:814
- NuGet's global-packages layout uses a lowercase package ID for the
.nuspecfilename (as the sharedFakeNuGetPackagehelper does attest/common/FakeNuGetPackage.cs:111). Writing the mixed-case name here makes these fake packages invalid on case-sensitive systems, so restore/cache lookup can miss them.
File.WriteAllText(Path.Combine(metadataPath, $"{packageName}.nuspec"), $"""
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The dependency-resolution implementation has correctness issues that can select the wrong assembly or fail valid project configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Problem: Assume, we have the external assembly defined in a typespec as follows:
If the version 3.0.0-alpha.20260820.5 is not present in the repository, the
ExternalTypeReferenceResolverwill not download the needed assembly and the one already present will be used. This will result in some classes not being found as by default the latest stable version is being downloaded.Solution: Currently, the external package is resolved as follows:
In this PR we are adding more logic: