Skip to content

Don't attach code-fix locations from outside the analyzed compilation - #131828

Open
sbomer wants to merge 7 commits into
dotnet:mainfrom
sbomer:illink-analyzer-compilation-reference-location
Open

Don't attach code-fix locations from outside the analyzed compilation#131828
sbomer wants to merge 7 commits into
dotnet:mainfrom
sbomer:illink-analyzer-compilation-reference-location

Conversation

@sbomer

@sbomer sbomer commented Aug 4, 2026

Copy link
Copy Markdown
Member

Fixes #109352

The crash

The DynamicallyAccessedMembers analyzer crashes with AD0001 in Visual Studio when it reports a diagnostic against a symbol that lives in a referenced project:

System.ArgumentException: Reported diagnostic 'IL2075' has a source location in file
'...\MessagePack.Annotations\Attributes.cs', which is not part of the compilation being analyzed.

Root cause

When the analyzer reports a data flow warning, it also attaches an extra location pointing at the declaration of the symbol that should be annotated, so the code fixer knows where to insert the attribute. Roslyn validates that every location on a reported diagnostic — including AdditionalLocations — belongs to the compilation being analyzed, so attaching a declaration from another compilation throws.

Two guards were used to decide whether that extra location was safe to attach, and both are wrong:

Site Guard
DiagnosticContext.CreateDiagnostic symbol.DeclaringSyntaxReferences.Length == 0
DynamicallyAccessedMembersAnalyzer.VerifyDamOnMethodsMatch Location.IsInSource

Both are really asking "is this symbol part of the compilation I am analyzing?", but neither answers that question. Under a CompilationReference — which is how Visual Studio models a project-to-project reference — a symbol from another project is still a source symbol with real declaring syntax references, so both guards happily let the foreign location through.

This is why the crash only reproduced in Visual Studio, and only with "run analysis on unopened files" enabled — which matches the reproduction history on the issue. Command-line builds reference other projects through their emitted assemblies, so cross-project symbols are metadata symbols with no declaring syntax references, and the old guard tripped correctly.

The fix

Replace both guards with Compilation.ContainsSyntaxTree, which asks the question directly.

This requires threading the Compilation into DiagnosticContext. I made it a required constructor parameter rather than an optional one so the compiler points at every construction site — that immediately surfaced three target-typed new(...) sites that a grep for new DiagnosticContext( had missed.

The second guard affects IL2092/IL2093/IL2094. GetTargetAndRequirements picks the base method as the attribute target when an override is annotated and the base is not, so an override in the current project whose base lives in a referenced project hit the same crash. I found this while auditing for other instances of the bug class, and it reproduces independently.

Suppressing the extra location only means the code fix is not offered for declarations the fixer could not have edited anyway. The underlying warning is still reported, and DynamicallyAccessedMembersCodeFixProvider already returns early when there are no additional locations.

Tests

Two regression tests, one per bug site (IL2075 and IL2092). Both were confirmed to fail before the fix with the exact ArgumentException from the issue, and to pass after.

They use TestState.AdditionalProjects / AdditionalProjectReferences, which the Roslyn testing SDK models as a real CompilationReference. Worth noting for anyone testing in this area: the existing ReferenceCompatibilityTestUtils helper cannot reproduce this, because it emits to a stream and creates a MetadataReference.

Full ILLink.RoslynAnalyzer.Tests suite: 1204 passed, 0 failed, 9 skipped.

Other sites audited

I checked the remaining diagnostic sites for the same bug class and believe they are safe:

  • RequiresAnalyzerBase — safe by construction; when an interface implementation is inherited from a base class, it already redirects origin to the current type rather than reporting on the base member.
  • DynamicallyAccessedMembersTypeHierarchy and ReflectionAccessAnalyzer — only use locations from symbols already established to be within the type being analyzed.

Note

This pull request was created with assistance from GitHub Copilot.

The DynamicallyAccessedMembers analyzer crashes with AD0001 in Visual
Studio when a diagnostic is reported against a symbol that lives in a
referenced project:

    System.ArgumentException: Reported diagnostic 'IL2075' has a source
    location in file '...\Attributes.cs', which is not part of the
    compilation being analyzed.

The analyzer attaches an extra location pointing at the declaration of
the symbol that should be annotated, so that the code fixer knows where
to insert the attribute. Roslyn validates that every location on a
reported diagnostic (including AdditionalLocations) belongs to the
compilation being analyzed, so attaching a declaration from another
compilation throws.

Two guards were used to decide whether that extra location was safe to
attach, and both are wrong:

  * DiagnosticContext.CreateDiagnostic checked
    symbol.DeclaringSyntaxReferences.Length == 0
  * DynamicallyAccessedMembersAnalyzer.VerifyDamOnMethodsMatch checked
    Location.IsInSource

Both are really asking "is this symbol part of the compilation I am
analyzing?", but neither answers that question. Under a
CompilationReference - which is how Visual Studio models a
project-to-project reference - a symbol from another project is still a
source symbol with real declaring syntax references, so both guards
happily let the foreign location through.

This is why the crash only reproduced in Visual Studio, and only with
"run analysis on unopened files" enabled. Command-line builds reference
other projects through their emitted assemblies, so cross-project
symbols are metadata symbols with no declaring syntax references and the
old guard tripped correctly.

Replace both guards with Compilation.ContainsSyntaxTree, which asks the
question directly. This requires threading the Compilation into
DiagnosticContext; it is a required constructor parameter rather than an
optional one so that the compiler points at every construction site.

The second guard affects IL2092/IL2093/IL2094. GetTargetAndRequirements
picks the base method as the attribute target when an override is
annotated and the base is not, so an override in the current project
whose base lives in a referenced project hit the same crash.

Both cases are covered by new regression tests, which use
TestState.AdditionalProjects to get a real CompilationReference. Note
that ReferenceCompatibilityTestUtils cannot reproduce this, as it emits
to a stream and creates a MetadataReference.

Suppressing the extra location only means the code fix is not offered
for declarations the fixer could not have edited anyway; the underlying
warning is still reported. The code fix provider already returns early
when there are no additional locations.

Fixes dotnet#109352

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed
Assisted-by: GitHub Copilot CLI:claude-opus-5
Copilot AI review requested due to automatic review settings August 4, 2026 19:21
@github-actions github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Aug 4, 2026
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 4, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Roslyn analyzer crash caused by reporting diagnostics whose AdditionalLocations point to source files outside the analyzed Compilation (e.g., when the symbol being annotated comes from a referenced project via CompilationReference). The change makes code-fix locations conditional on Compilation.ContainsSyntaxTree(...) and adds regression coverage for both affected diagnostic sites.

Changes:

  • Thread Compilation into TrimAnalysis.DiagnosticContext and gate code-fix location attachment on Compilation.ContainsSyntaxTree(...).
  • Update all DiagnosticContext construction sites in trim analysis / requires analyzers to pass the active compilation.
  • Add regression tests using AdditionalProjects/AdditionalProjectReferences to simulate project-to-project references.
Show a summary per file
File Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DynamicallyAccessedMembersAnalyzerTests.cs Adds regression tests ensuring no foreign AdditionalLocations are attached when the target symbol lives in a referenced project.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TypeNameResolver.cs Exposes the current compilation internally so downstream trim-analysis helpers can build safe DiagnosticContext instances.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisReflectionAccessPattern.cs Passes context.Compilation into DiagnosticContext so code-fix location gating can be compilation-aware.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisMethodCallPattern.cs Same as above for method-call pattern diagnostics.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisFieldAccessPattern.cs Same as above for field access diagnostics.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisBackingFieldAccessPattern.cs Same as above for backing-field access diagnostics.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs Ensures type-name resolution diagnostics use a compilation-aware DiagnosticContext.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs Updates diagnostic emission paths to construct DiagnosticContext with the correct compilation.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs Updates stored/constructed diagnostic contexts to include compilation.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FeatureCheckReturnValuePattern.cs Updates diagnostic context creation to include compilation.
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs Implements the new compilation-aware guard (ContainsSyntaxTree) before attaching code-fix locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Updates diagnostic context creation in implicit base-ctor analysis to pass context.Compilation.
src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs Replaces Location.IsInSource with a compilation-aware guard for code-fix locations in override/virtual mismatch diagnostics.

Copilot's findings

  • Files reviewed: 13/13 changed files
  • Comments generated: 1

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed
Assisted-by: GitHub Copilot CLI:claude-opus-5
Copilot AI review requested due to automatic review settings August 4, 2026 22:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new

@sbomer
sbomer requested a review from jtschuster August 4, 2026 22:25
The tests only need the referenced symbols to come from a
CompilationReference, which Compilation.ToMetadataReference() produces
directly. Building one and passing it through the existing analyzer
verification path avoids the AdditionalProjects plumbing, and with it
the EmptyCodeFixProvider alias that only existed because the code fix
test infrastructure requires a code fix provider type argument.

This also lets the tests reuse TestCaseUtils.UseMSBuildProperties like
the rest of the file instead of hand-rolling an .editorconfig.

Verified that the tests still fail without the fix, with the same
ArgumentException as before.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed
Assisted-by: GitHub Copilot CLI:claude-opus-5
Copilot AI review requested due to automatic review settings August 4, 2026 22:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 13/13 changed files
  • Comments generated: 0 new

Comment thread src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs Outdated
Comment thread src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs Outdated

@jtschuster jtschuster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from a couple nits.

@agocke

agocke commented Aug 5, 2026

Copy link
Copy Markdown
Member

Heh, I was looking at this as well. Good news -- at first glance I think we have roughly the same implementation.

Comment thread src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs Outdated
Comment thread src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs Outdated
@@ -223,7 +223,7 @@ private static void VerifyDamOnMethodsMatch(SymbolAnalysisContext context, IMeth
Location attributableSymbolLocation = GetPrimaryLocation(attributableMethod.Locations);

// code fix does not support merging multiple attributes. If an attribute is present or the method is not in source, do not provide args for code fix.
(Location[]? sourceLocation, Dictionary<string, string?>? DAMArgs) = (!attributableSymbolLocation.IsInSource

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can stop attaching additional source locations here entirely. Symbols don't need locations because the code fixer can completely hydrate all necessary information without looking at locations at all.

The only place we might need auxiliary information is in data flow.

Same is true for DAM annotations -- on symbols this info is trivial to reconstruct in the fixer. It's only helpful to attach that info for flow analysis, where the fixer would have to re-do all of flow analysis to find the right data.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to avoid attaching additional locations for the override cases. The fixer will use the additional location when it's set (for dataflow), otherwise it uses the diagnostic location.

The code fix provider resolves the attached additional location's span
against the syntax root of the document containing the diagnostic's
primary location. So the real precondition for attaching a code fix
location is that it lives in the *same syntax tree* as the diagnostic,
which is stricter than "part of the same compilation".

Beyond the reported AD0001 (location in another compilation, which
Roslyn rejects outright), the weaker guard also allowed a silent
failure: when the location is in a different file of the same
compilation and its span happens to be in range, FindNode resolves the
wrong node and the fixer annotates an unrelated declaration. That
compiles cleanly, so it surfaces only as a silently different trimming
contract.

Comparing syntax trees covers both cases and removes the need to thread
the Compilation through DiagnosticContext. Code fix targets are
unchanged for the single-file case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed
Assisted-by: GitHub Copilot CLI:claude-opus-5
@agocke

agocke commented Aug 5, 2026

Copy link
Copy Markdown
Member

Added my version -- these are super similar. I ended up guarding the data flow as well, but then changed overrides to just avoid the problem #131910

Arguably this change is smaller

For DAM annotation mismatches between overrides or interface
implementations, only offer an additive code fix when the implementation
itself is missing the annotation. Do not offer to change the base or
interface member's contract.

Inherited interface implementations now omit code-fix data because the
member that would be changed is declared on the base type. Direct
implementations continue to receive a fix. Keep the same-syntax-tree
check local to DiagnosticContext for data-flow diagnostics whose source
symbol can be declared in another file or project.

Update the reversed mismatch tests to expect no fix and add coverage for
direct and inherited interface implementations and cross-file data flow.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed
Assisted-by: GitHub Copilot CLI:gpt-5.6-sol
Copilot AI review requested due to automatic review settings August 6, 2026 16:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed

Assisted-by: GitHub Copilot CLI:gpt-5.6-sol
Copilot AI review requested due to automatic review settings August 6, 2026 17:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 47e32591-30fa-4a23-b1c1-48d277b1a3ed

Assisted-by: GitHub Copilot CLI:gpt-5.6-sol
Copilot AI review requested due to automatic review settings August 7, 2026 15:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Suppressed comments (2)

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs:86

  • Use the shared DynamicallyAccessedMembersAnalyzer.attributeArgument constant instead of repeating the "attributeArgument" string literal here. This reduces the risk of the analyzer and code fix provider diverging on the property key in future edits.
            {
                DAMArgument.Add("attributeArgument", expectedAnnotationsValue.DynamicallyAccessedMemberTypes.ToString());
                sourceLocation = new Location[] { symbolLocation };

src/tools/illink/src/ILLink.CodeFix/DynamicallyAccessedMembersCodeFixProvider.cs:109

  • Solution.GetDocument(SyntaxTree) can pick an arbitrary document when the same file is linked into multiple projects. Since the diagnostic is reported in the current project's compilation, prefer document.Project.GetDocument(targetTree) so the fix is applied in the correct project/document.
            if (targetLocation.SourceTree is not { } targetTree
                || document.Project.Solution.GetDocument(targetTree) is not { } targetDocument)
                return;
  • Files reviewed: 16/16 changed files
  • Comments generated: 0 new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

DynamicallyAccessedMembersAnalyzer throws an internal exception due to source location outside compilation

4 participants