Guard DAM code fix locations by compilation - #131910
Open
agocke wants to merge 2 commits into
Open
Conversation
Only attach source locations used by the DAM code fixer when their syntax trees belong to the active compilation. Resolve guarded targets through their owning document and keep override fixes add-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df89f75b-e470-4821-9186-d01addeec5ea
github-actions
Bot
added
the
area-Tools-ILLink
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 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
Bot
added
the
linkable-framework
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ILLink DynamicallyAccessedMembers (DAM) analyzer + code fix pipeline so diagnostics only include code-fix target locations when the target syntax tree belongs to the active compilation, avoiding Roslyn failures when symbols originate from a referenced project’s source.
Changes:
- Guard “code fix target” additional locations by checking
Compilation.ContainsSyntaxTree(...)before attaching them to diagnostics. - Update the DAM code fix provider to resolve the correct
Documentfrom the additional location’sSyntaxTreebefore editing. - Extend analyzer/code-fix tests and utilities to cover referenced-compilation scenarios and adjust expected diagnostics accordingly.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/ReferenceCompatibilityTestUtils.cs | Adds a test helper to build a referenced project (compilation reference) for foreign-location coverage. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DynamicallyAccessedMembersCodeFixTests.cs | Updates baseline/fixed diagnostic expectations and adds coverage for several DAM code-fix scenarios (including metadata/reference interactions). |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DynamicallyAccessedMembersAnalyzerTests.cs | Adds compilation-reference regression coverage and relaxes expectations around additional locations. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TypeNameResolver.cs | Exposes the active Compilation internally to support guarded location production downstream. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs | Threads the active compilation into DiagnosticContext creation for guarded additional locations. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs | Adds optional compilation tracking and filters declaration locations to those in the active compilation. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DAM-code-fix.md | Updates docs to describe guarded additional locations and code-fix document resolution. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs | Guards override/interface code-fix target locations by compilation membership and adjusts origin handling. |
| src/tools/illink/src/ILLink.CodeFix/DynamicallyAccessedMembersCodeFixProvider.cs | Applies fixes via the document that owns the additional location’s syntax tree (rather than the triggering document). |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 3
Comment on lines
+354
to
+358
| [targetLocation], | ||
| new Dictionary<string, string?> | ||
| { | ||
| [attributeArgument] = annotation.ToString() | ||
| }.ToImmutableDictionary()); |
Comment on lines
+78
to
+81
| properties = new Dictionary<string, string?> | ||
| { | ||
| ["attributeArgument"] = expectedAnnotationsValue.DynamicallyAccessedMemberTypes.ToString(), | ||
| }; |
Comment on lines
+47
to
+51
| private static void IgnoreAdditionalLocations(DiagnosticResult[] diagnostics) | ||
| { | ||
| for (int i = 0; i < diagnostics.Length; i++) | ||
| diagnostics[i] = diagnostics[i].WithOptions(DiagnosticOptions.IgnoreAdditionalLocations); | ||
| } |
Make the active compilation a required DiagnosticContext constructor argument and thread it through all analyzer construction sites so future code-fix locations cannot silently skip compilation validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df89f75b-e470-4821-9186-d01addeec5ea
sbomer
reviewed
Aug 5, 2026
| if (root.FindNode(diagnostic.AdditionalLocations[0].SourceSpan, getInnermostNodeForTie: true) is not SyntaxNode targetNode) | ||
| if (diagnostic.AdditionalLocations[0].SourceTree is not { } targetTree) | ||
| return; | ||
| if (document.Project.Solution.GetDocument(targetTree) is not { } targetDocument) |
Member
There was a problem hiding this comment.
This means the target document could be a different file right? Do we have test coverage for that case?
| if (overrideParameterAnnotation == DynamicallyAccessedMemberTypes.None | ||
| && !overrideParam.ParameterSymbol!.TryGetAttribute(DynamicallyAccessedMembersAttribute, out _)) | ||
| { | ||
| (additionalLocations, properties) = CreateCodeFixArguments( |
Member
There was a problem hiding this comment.
Can this still offer a code fix on a base method when the base method implements an interface? Might be worth adding a test like this:
interface I
{
void M(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
Type type);
}
class Base
{
public void M(Type type) { }
}
class Derived : Base, I
{
}
Contributor
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs:80
- Use the shared
DynamicallyAccessedMembersAnalyzer.attributeArgumentconstant instead of a hard-coded string key, so the analyzer and code fix stay in sync if the key ever changes.
["attributeArgument"] = expectedAnnotationsValue.DynamicallyAccessedMemberTypes.ToString(),
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs:99
- Prefer returning
Location.Noneinstead of usingnull!for an out parameter. This avoids propagating a nullLocationif the method is ever refactored and makes the intent explicit.
location = null!;
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
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
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.
Fixes #109352
The DAM analyzer can encounter source-backed symbols from a referenced project compilation. Those symbols have source locations, but Roslyn rejects those locations when they are attached to diagnostics reported for the active compilation.
This change:
Tests:
DynamicallyAccessedMembersAnalyzerTestsandDynamicallyAccessedMembersCodeFixTests(108 passed)Note
This pull request description was created with GitHub Copilot.