fix(generators): floor Microsoft.CodeAnalysis.CSharp to the .NET 8 SDK's Roslyn version - #136
Merged
Merged
Conversation
…K's Roslyn version Compono.Generators.dll (the packaged, shipped analyzer) was built against Microsoft.CodeAnalysis.CSharp 5.9.0. Roslyn's own analyzer-loading gate refuses to load an analyzer built against a NEWER compiler than the host's own - silently, with only a build warning (CS9057), never an error - so on the .NET 8/9/10 STABLE SDKs (bundled Roslyn 4.11.0.0/ 4.14.0.0/5.0.0.0, all older than 5.9.0.0) the generator simply never ran. A consuming Composer.Create<T>() then failed at runtime with a misleading "no generated plan" CompositionException instead of a build-time signal pointing at the real cause. This repo's own CI, and both real dogfood consumers (alexa-vox-craft, trivia-platform), all pin an 11.0-preview SDK, so nothing caught this - every officially-supported target (net8.0/net9.0/net10.0/net11.0) except the one this repo's own tooling happens to run on was silently broken. Confirmed empirically, before and after, via a throwaway consumer built against each of the four SDKs (8.0.408/9.0.304/10.0.103/11.0.100-preview.7), using a scenario that exercises constructor composition, Share<T>() graph-wide identity, and a Compono.TestDoubles-generated interface double - not just a trivial root type. Before: CS9057 + CompositionException on 8/9/10. After: identical, correct output (same deterministic seed) on all four. Also verified via a real PublishAot run under the .NET 10 SDK directly (not the repo's own 11.0-preview SDK). - Directory.Packages.props: floor Microsoft.CodeAnalysis.CSharp to 4.11.0 (the .NET 8 SDK's own bundled compiler version, not a version chosen arbitrarily) - Compono.Generators is the only project this centrally governs that's actually packed/shipped as an analyzer. - Compono.Benchmarks/Compono.Generators.Tests: neither is packed/shipped, and both need LanguageVersion.CSharp14 (undefined below ~5.x) for their own in-process CSharpGeneratorDriver use against net10.0/net11.0 targets - VersionOverride="5.9.0" keeps them on the newer version they actually need without affecting the shipped analyzer's own compile. - .github/dependabot.yml: ignore all automated updates to Microsoft.CodeAnalysis.CSharp - this floor is a deliberately cross-SDK-verified decision, not something that should move via an unattended bump (this repo's own CI wouldn't catch a regression here, since it pins the 11.0-preview SDK). Full solution: dotnet build (0 warnings/errors) and dotnet test (3737/3737 passed). Package-validation (inspect-packed-nupkgs.sh) green for all twelve packages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddad5a551f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… SDK minimum Addresses Codex review feedback on this PR (#136): 1. The prior comment on Directory.Packages.props implied 4.11.0 was chosen to match a specific SDK patch (8.0.408) rather than being the real floor. Investigated properly: attempting to lower it further (e.g. to 4.8.0, .NET 8 GA's bundled compiler) doesn't even compile - Compono.Generators/Discovery/TestDoubleAnalyzer.cs uses ITypeParameterSymbol.AllowsRefLikeType, a Roslyn API that only exists starting at compiler package version 4.11.0 (bisected empirically: 4.9.2/4.10.0 fail CS1061, 4.11.0 compiles). So 4.11.0 is a hard technical floor, not a preference - and since it first shipped with .NET SDK 8.0.4xx, Compono's real minimum-supported .NET 8 SDK is 8.0.400, not 8.0.100 (GA), which docs/getting-started/installation.md never stated. 2. Recorded the decision as ADR-0003 Amendment 1 - this repo's Milestone 1 review had already flagged pinning the generator's minimum Roslyn/SDK version as a real concern but explicitly deferred deciding it; that gap is what let the original 5.6.0-5.9.0 pin ship broken in every release since the very first one. Re-verified empirically across all four officially-supported SDKs (8.0.408, 9.0.304, 10.0.103, 11.0.100-preview.7) with the 4.11.0 pin - clean on all four, same as before this commit (no functional change to the pin itself, only to its documentation and justification). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s
Contributor
Author
|
@codex review commit 31cb7bc — addresses both prior findings: (1) the Roslyn floor is now confirmed as a hard technical minimum (4.11.0, verified by bisection against the generator's own compile), not a chosen one, and (2) the decision is recorded in ADR-0003 Amendment 1 with the corrected .NET 8 SDK minimum (8.0.400) documented in docs/getting-started/installation.md. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Compono.Generators.dll(the packaged, shipped analyzer) was built againstMicrosoft.CodeAnalysis.CSharp 5.9.0. Roslyn's own analyzer-loading gate refuses to load an analyzer built against a newer compiler than the host's own — silently, with only a build warning (CS9057), never an error. On the .NET 8/9/10 stable SDKs (bundled Roslyn4.11.0.0/4.14.0.0/5.0.0.0, all older than5.9.0.0) the generator simply never ran. A consumingComposer.Create<T>()then failed at runtime with a misleading"no generated plan"CompositionExceptioninstead of a build-time signal pointing at the real cause.This repo's own CI, and both real dogfood consumers (alexa-vox-craft, trivia-platform), all pin an
11.0-previewSDK — so nothing caught this. Every officially-supported target (net8.0/net9.0/net10.0/net11.0) except the one this repo's own tooling happens to run on was silently broken.Changes
Directory.Packages.props: floorMicrosoft.CodeAnalysis.CSharpto4.11.0— the .NET 8 SDK's own bundled compiler version, not a version chosen arbitrarily.Compono.Generatorsis the only project this centrally governs that's actually packed/shipped as an analyzer.Compono.Benchmarks/Compono.Generators.Tests: neither is packed/shipped, and both needLanguageVersion.CSharp14(undefined below ~5.x) for their own in-processCSharpGeneratorDriveruse againstnet10.0/net11.0targets —VersionOverride="5.9.0"keeps them on the newer version they actually need without affecting the shipped analyzer's own compile..github/dependabot.yml: ignore all automated updates toMicrosoft.CodeAnalysis.CSharp— this floor is a deliberately cross-SDK-verified decision, not something that should move via an unattended bump (this repo's own CI wouldn't catch a regression here, since it pins the 11.0-preview SDK).Test plan
Share<T>()graph-wide identity, and aCompono.TestDoubles-generated interface double — not just a trivial root type.CS9057warning +CompositionException(generator silently never ran) on net8/net9/net10.dotnet publish -p:PublishAot=truerun directly under the .NET 10 SDK (not this repo's own 11.0-preview SDK) — zero AOT/trim warnings, generator ran, real assertions passed.dotnet build Compono.slnx— 0 warnings, 0 errors.dotnet test Compono.slnx— full solution, 3737/3737 passed..github/scripts/inspect-packed-nupkgs.sh— green for all twelve packages after a real local pack.🤖 Generated with Claude Code
https://claude.ai/code/session_01NiVv392P3m46azTD1TpU3s