test: pin diagnostic-location safety across incremental updates#144
Conversation
Mirrors the bug class fixed by dotnet/roslyn#82113 (issue #82032), from the consumer side. A generator diagnostic whose location was captured against a longer tree must not survive a shrinking incremental edit as an out-of-range span. Our DiagnosticInfo/LocationInfo reconstruct an external-file location (never source-tree-bound), so the driver never range-validates it and a shrink can neither crash the generator nor report a stale span. The test drives CSharpGeneratorDriver across a shrinking ReplaceSyntaxTree edit and asserts the re-reported AL0301 location is external (IsInSource == false) and in range of the new tree, with no generator-failure diagnostic. It fails if LocationInfo.ToLocation() ever regresses to a source-tree-bound location. Bonus: extract a public GeneratorTestHelper.CreateCompilation primitive (reused by RunGenerator) so tests can drive the driver directly for incremental scenarios, and declare the now-direct Microsoft.CodeAnalysis.CSharp dependency on the DU test project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/.github/workflows/**/*.{yml,yaml}📄 CodeRabbit inference engine (Custom checks)
Files:
.github/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
Summary by CodeRabbit
WalkthroughCompilation creation is extracted into a reusable public helper to enable direct incremental generator driver invocation. A regression test then validates that AL0301 diagnostics remain valid after syntax-tree shrinking without crashing or reporting out-of-range locations. CI is gated on test passage. ChangesIncremental Diagnostic Location Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs`:
- Around line 63-67: The test currently inspects generator-only diagnostics via
driver.GetRunResult().Diagnostics which won't surface compiler-level failures
like CS8785; change the test to call RunGeneratorsAndUpdateCompilation to obtain
the updated outputCompilation and assert that outputCompilation.GetDiagnostics()
(or the compilation diagnostics collection returned) contains no CS8785, and
additionally replace the manual CSharpGeneratorDriver plumbing with the
project's Test<TGenerator> harness (or adjust to the canonical test runner) so
the test follows repo conventions; update references to
driver.RunGenerators(...) and driver.GetRunResult() accordingly and remove the
old generator-only assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3e65723-7e31-47d2-b756-f74fd4f93e3f
📒 Files selected for processing (3)
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cstests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests.csprojtests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.Tests.csproj
📄 CodeRabbit inference engine (global.json)
Use Microsoft.Testing.Platform as the test runner
Files:
tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests.csproj
**
⚙️ CodeRabbit configuration file
**: # ANcpLua.Roslyn.UtilitiesMinimal navigation for Claude/Codex agents. Keep policy in
AGENTS.md; keep findings in issues, PRs, or tests.Project Index
- AOT reflection generator
- AOT reflection attributes
- Discriminated union generator
- Extensible enum mirror generator
- Core Roslyn utilities
- Polyfills package
- Source-only package
- Testing utilities
- AOT testing utilities
Nearby Repos
- ANcpLua.NET.Sdk: shared SDK/version truth for the ANcpLua repos.
- ANcpLua.Analyzers: analyzer consumer of the source-only utilities.
- ANcpLua.Agents: successor location for agent workflow/test helpers; do not describe this repo as the MAF runtime home.
**:
999.9.9
10.0.203
latestMinor
<PropertyGroup Label="Roslyn"> <RoslynVersion>5.3.0</RoslynVersion> <RoslynAnalyzersVersion>5.3.0</RoslynAnalyzersVersion> </PropertyGroup> <!-- ═══════════════════════════════════════════════════════════════════════ ROSLYN ANALYZER TESTING Used by: Roslyn.Utilities.Testing, Analyzers.Tests ════════════════════════════════════════════════════════════════════...
Files:
tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests.csprojsrc/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cstests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
**/*.{ts,tsx,js,jsx,cs,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Skip files whose first ~3 lines contain
// Ported from <upstream>,// Generated, or// Auto-generated— surface a one-line note instead of line-level findings
Files:
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cstests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
.NET code: enable nullable reference types, use central package management via
Directory.Packages.props, and treatVersion.propsas the single owner of versions — never edit<Version>lines directly
**/*.cs: Scan added/modified C# files for.Result,.Wait(), or.GetAwaiter().GetResult()and fail if any are found
Scan added/modified C# files forDateTime.NoworDateTime.UtcNowand fail if any usage found — useTimeProvider.System(or polyfill in netstandard2.0) instead
Scan added/modified C# files for: ISourceGeneratorpatterns and fail if a new ISourceGenerator is added — must use IIncrementalGenerator instead
Scan added/modified C# files for!(null-forgiving operator) and fail if any!is added without an inline comment
**/*.cs: UseDiagnosticFlow<T>for railway-oriented error accumulation in incremental generator pipelines
UseEquatableArray<T>and value-equatable collections to ensure incremental generator caching actually hits
UseIndentedStringBuilderwithBeginBlock()/EndBlock()scopes instead of hardcoded indentation strings for code emission
UseMatch.Type(),Match.Method(), and fluent symbol matching instead of string comparisons for symbol analysis
Embed utilities viaANcpLua.Roslyn.Utilities.Sourcespackage asinternalsource code in source generators instead of loading NuGet DLLs at runtime
UseValueStringBuilderfor stack-allocated string building in analyzer hot paths to reduce allocations
Use closure-freeGetOrInsert<TContext>(key, context, factory)for dictionary inserts in analyzer hot paths instead of lambda closures
UseGeneratorCachingReportto compare two generator runs and detect cache hits instead of hand-rolled cache assertions
UseTest<TGenerator>fluent runner for generator tests instead of hand-rolledCSharpGeneratorDriverplumbing
UseForAttributeWithMetadataNameto filter syntax before expensive semantic work in incremental generators
Compile-time reflection takes precedence o...
Files:
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cstests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
**/*.{cs,ts,tsx,py}
📄 CodeRabbit inference engine (Custom checks)
Fail C#/TS/Python changes that introduce sync-over-async, unobserved fire-and-forget work, missing CancellationToken/AbortSignal propagation on public/internal async boundaries, sleeps for synchronization, or resource disposal paths that can drop in-flight work
Files:
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cstests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
src/**/*.cs
⚙️ CodeRabbit configuration file
src/**/*.cs: C# 14 / .NET 10 codebase. Review for: idiomatic modern C#, proper async/await (no sync-over-async, no fire-and-forget without justification), correct IDisposable/IAsyncDisposable, null safety (NRTs enabled), and adherence to existing patterns. Flag new public API surface that lacks XML doc comments. Check DI lifetime correctness (scoped vs singleton vs transient).
ARCHITECTURAL INVARIANTS — flag violations as blocking: 1. Every new injectable service must register OpenTelemetry instrumentation
(ActivitySource or Meter).
2. Every new DuckDB write path must handle backpressure (bounded channel or semaphore). 3. No hardcoded connection strings, paths, or magic strings — use IOptions or
IConfiguration.
4. No new dependencies on Sentry-specific types in core/ — Sentry is a comparison
target, not an identity.
5. CancellationToken must be threaded through all async public methods. 6. No sync-over-async (.Result, .GetAwaiter().GetResult()) outside of
well-documented infrastructure code.
Files:
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cs
src/ANcpLua.Roslyn.Utilities.Testing/**
⚙️ CodeRabbit configuration file
src/ANcpLua.Roslyn.Utilities.Testing/**: # ANcpLua.Roslyn.Utilities.TestingTesting support package for Roslyn utilities and related generators.
- Project: ANcpLua.Roslyn.Utilities.Testing.csproj
- Keep test helpers focused on this repo's utilities.
- Agent workflow helpers belong in ANcpLua.Agents, not here.
Files:
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cs
tests/**/*.cs
⚙️ CodeRabbit configuration file
xUnit.v3 test projects using xunit.v3.mtp-v2 with AwesomeAssertions and NSubstitute. Follow Arrange-Act-Assert pattern. Use descriptive test names: MethodName_Scenario_ExpectedResult. Test async methods with async Task, not async void. Flag hardcoded test data that should come from the seeded DuckDB file (the single source of truth for demo/test data). Prefer [Theory] with [InlineData] or [MemberData] over duplicated [Fact] methods testing variations of the same behavior. Flag tests that depend on test execution order.
Files:
tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs
🔇 Additional comments (3)
src/ANcpLua.Roslyn.Utilities.Testing/GeneratorHelpers/GeneratorTestHelper.cs (1)
39-39: LGTM!Also applies to: 49-59
tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests.csproj (1)
19-20: LGTM!tests/ANcpLua.Roslyn.Utilities.DiscriminatedUnion.Tests/IncrementalDiagnosticLocationTests.cs (1)
45-47: Don’t forceTest<TGenerator>here: this test needsReplaceSyntaxTreebetween runs.
Test<TGenerator>/GeneratorTestEngine.RunTwiceAsynconly reruns on a cloned compilation with the same syntax trees (no hook to swap in the “shrunk” tree), so the directCSharpGeneratorDriverflow is appropriate for validating the shrinking-edit diagnostic span.> Likely an incorrect or invalid review comment.
…pile The build job compiled the test projects but never ran them, so a runtime regression could pass CI and auto-publish to nuget.org. Add a Test step after Build on both ubuntu + windows. publish `needs: build`, so a failing test now fails the job and blocks the release — closing the gap without touching the fleet-managed branch protection. CrefRegression.Tests stays a compile-only guard covered by Build (it has no test-platform reference). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Was
Regression test inspired by dotnet/roslyn#82113 (fixes roslyn#82032) — "Validate generator diagnostics after incremental updates".
That Roslyn fix is host-side: it re-validates every generator diagnostic's
Locationagainst the current compilation on every run, so a span captured against a longer tree can't survive a shrinking edit and crash. We can't apply that patch (we don't ownGeneratorDriver), but we are already on the safe side of the bug: our diagnostics flow as value-equatableDiagnosticInfo/LocationInfo, which reconstruct an external-file location (Location.Create(path, span, lineSpan)→SourceTree == null) at report time. The driver never range-validates external locations, so a shrink can neither crash the generator nor emit a stale span.This PR pins that guarantee.
The test
Shrinking_Edit_Keeps_Diagnostic_Location_In_Range_Without_CrashingdrivesCSharpGeneratorDriveracross a shrinkingReplaceSyntaxTreeedit (long union root → tiny one, both reportingAL0301) and asserts on the re-reported diagnostic:Location.IsInSource == false— it's an external location the driver never range-validates (this is the assertion that fails ifLocationInfo.ToLocation()ever regresses to a source-tree-bound location).Location.SourceSpan.End <= newTree.Length— the location stays in range after the edit.AL0301— noCS8785/crash diagnostic.Bonus
GeneratorTestHelper.CreateCompilation(params string[])primitive (reused byRunGenerator) so tests can drive the driver directly for incremental scenarios.Microsoft.CodeAnalysis.CSharpdependency on the DU test project (the test constructsCSharpGeneratorDriver/CSharpSyntaxTreedirectly rather than relying on transitive flow).Verification
dotnet build -c Debug→ 0 warnings, 0 errorsdotnet test(DU project) → 9/9 passed🤖 Generated with Claude Code