Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sbomer wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
820e886
Don't attach code-fix locations from outside the analyzed compilation
sbomer Aug 4, 2026
a6d8298
Fix stale expected-location comment in IL2092 regression test
sbomer Aug 4, 2026
dc64050
Simplify regression tests to reference a CompilationReference directly
sbomer Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@agocke agocke Aug 5, 2026

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.

(Location[]? sourceLocation, Dictionary<string, string?>? DAMArgs) = (!CanOfferCodeFixAt(attributableSymbolLocation, context.Compilation)
|| (overrideMethod.TryGetReturnAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)
&& baseMethod.TryGetReturnAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _))
) ? (null, null) : CreateArguments(attributableSymbolLocation, missingAttribute);
Expand All @@ -247,7 +247,7 @@ private static void VerifyDamOnMethodsMatch(SymbolAnalysisContext context, IMeth
Location attributableSymbolLocation = attributableMethod.GetParameter(overrideParam.Index).Location!;

// 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
(Location[]? sourceLocation, Dictionary<string, string?>? DAMArgs) = (!CanOfferCodeFixAt(attributableSymbolLocation, context.Compilation)
|| (overrideParam.ParameterSymbol!.TryGetAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)
&& baseParam.ParameterSymbol!.TryGetAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _))
) ? (null, null) : CreateArguments(attributableSymbolLocation, missingAttribute);
Expand All @@ -273,7 +273,7 @@ private static void VerifyDamOnMethodsMatch(SymbolAnalysisContext context, IMeth
Location attributableSymbolLocation = GetPrimaryLocation(attributableSymbol.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
(Location[]? sourceLocation, Dictionary<string, string?>? DAMArgs) = (!CanOfferCodeFixAt(attributableSymbolLocation, context.Compilation)
|| (overrideMethod.TypeParameters[i].TryGetAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _)
&& baseMethod.TypeParameters[i].TryGetAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _))
) ? (null, null) : CreateArguments(attributableSymbolLocation, missingAttribute);
Expand Down Expand Up @@ -351,8 +351,16 @@ private static void VerifyDamOnPropertyAndAccessorMatch(SymbolAnalysisContext co
}
}

private static (IMethodSymbol Method, DynamicallyAccessedMemberTypes Requirements) GetTargetAndRequirements(IMethodSymbol method, IMethodSymbol overriddenMethod, DynamicallyAccessedMemberTypes methodAnnotation, DynamicallyAccessedMemberTypes overriddenMethodAnnotation)
{
/// <summary>
/// Determines whether a code fix location can be attached to a diagnostic. The location must point into the
/// compilation being analyzed, otherwise Roslyn rejects the reported diagnostic. Note that a location can be in
/// source and still belong to another project: the IDE models project-to-project references as compilation
/// references, which expose source symbols whose syntax trees belong to a different compilation.
/// </summary>
private static bool CanOfferCodeFixAt(Location location, Compilation compilation)
=> location.SourceTree is { } sourceTree && compilation.ContainsSyntaxTree(sourceTree);
Comment on lines +360 to +361

@jtschuster jtschuster Aug 4, 2026

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'm slightly concerned that this and CanOfferCodeFixOn are separate and could diverge. Could we make this one internal/public and call this from CanOfferCodeFixOn?


private static (IMethodSymbol Method, DynamicallyAccessedMemberTypes Requirements) GetTargetAndRequirements(IMethodSymbol method, IMethodSymbol overriddenMethod, DynamicallyAccessedMemberTypes methodAnnotation, DynamicallyAccessedMemberTypes overriddenMethodAnnotation) {

@jtschuster jtschuster Aug 4, 2026

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.

Suggested change
private static (IMethodSymbol Method, DynamicallyAccessedMemberTypes Requirements) GetTargetAndRequirements(IMethodSymbol method, IMethodSymbol overriddenMethod, DynamicallyAccessedMemberTypes methodAnnotation, DynamicallyAccessedMemberTypes overriddenMethodAnnotation) {
private static (IMethodSymbol Method, DynamicallyAccessedMemberTypes Requirements) GetTargetAndRequirements(IMethodSymbol method, IMethodSymbol overriddenMethod, DynamicallyAccessedMemberTypes methodAnnotation, DynamicallyAccessedMemberTypes overriddenMethodAnnotation)
{

DynamicallyAccessedMemberTypes mismatchedArgument;
IMethodSymbol paramNeedsAttributes;
if (methodAnnotation == DynamicallyAccessedMemberTypes.None)
Expand Down
3 changes: 2 additions & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public virtual void ProcessGenericInstantiation(

if (publicParameterlessConstructor != null)
{
var diagnosticContext = new DiagnosticContext(location, reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, typeNameResolver.Compilation, reportDiagnostic);
CheckAndCreateRequiresDiagnostic(
publicParameterlessConstructor,
owningSymbol,
Expand Down Expand Up @@ -201,6 +201,7 @@ private void AnalyzeImplicitBaseCtor(SymbolAnalysisContext context)

var diagnosticContext = new DiagnosticContext(
typeSymbol.Locations[0],
context.Compilation,
context.ReportDiagnostic);

CheckAndCreateRequiresDiagnostic(
Expand Down
22 changes: 20 additions & 2 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/DiagnosticContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@ public readonly partial struct DiagnosticContext
{
public readonly Location Location { get; }

private readonly Compilation _compilation;

private readonly Action<Diagnostic>? _reportDiagnostic;

public DiagnosticContext(Location location, Action<Diagnostic>? reportDiagnostic)
public DiagnosticContext(Location location, Compilation compilation, Action<Diagnostic>? reportDiagnostic)
{
Location = location;
_compilation = compilation;
_reportDiagnostic = reportDiagnostic;
}

Expand Down Expand Up @@ -68,7 +71,7 @@ private Diagnostic CreateDiagnostic(DiagnosticId id, ValueWithDynamicallyAccesse
Dictionary<string, string?>? DAMArgument = new Dictionary<string, string?>();

// not supporting merging differing attributes, check to make sure symbol has no other attributes
if (symbol.DeclaringSyntaxReferences.Length == 0
if (!CanOfferCodeFixOn(symbol)
|| (actualValue is not MethodReturnValue
&& symbol.TryGetAttribute(DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var _))
|| (actualValue is MethodReturnValue
Expand All @@ -88,5 +91,20 @@ private Diagnostic CreateDiagnostic(DiagnosticId id, ValueWithDynamicallyAccesse

return Diagnostic.Create(DiagnosticDescriptors.GetDiagnosticDescriptor(id), Location, sourceLocation, DAMArgument?.ToImmutableDictionary(), args);
}

/// <summary>
/// Determines whether a code fix location can be attached to a diagnostic for <paramref name="symbol"/>.
/// The symbol must be declared in source that is part of the compilation being analyzed, otherwise Roslyn
/// rejects the reported diagnostic. Note that a symbol from another project can still have declaring syntax
/// references: the IDE models project-to-project references as compilation references, which expose source
/// symbols whose syntax trees belong to a different compilation.
/// </summary>
private bool CanOfferCodeFixOn(ISymbol symbol)

@agocke agocke Aug 5, 2026

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.

Actually, I was taking a different approach. This is approaching the problem from trying to fix things in many different scenarios and then catching mistakes.

I think a more robust solution would be to use the inherent structure of the program to decide when a code fix should be offered.

So, an example: in a base/override scenario, we can always offer the code fixer on the override, and just ignore fixes that on the base entirely. We actually don't have to know about the compilation at all to know that the override always belongs to the current compilation.

{
if (symbol.DeclaringSyntaxReferences.Length == 0)

@agocke agocke Aug 5, 2026

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.

If we set things up right I think this should actually be impossible to hit.

return false;

return _compilation.ContainsSyntaxTree(symbol.DeclaringSyntaxReferences[0].SyntaxTree);
}
}
}
2 changes: 1 addition & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FeatureCheckReturnValuePattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public FeatureCheckReturnValuePattern(

public void ReportDiagnostics(DataFlowAnalyzerContext context, Action<Diagnostic> reportDiagnostic)
{
var diagnosticContext = new DiagnosticContext(Operation.Syntax.GetLocation(), reportDiagnostic);
var diagnosticContext = new DiagnosticContext(Operation.Syntax.GetLocation(), context.Compilation, reportDiagnostic);
// For now, feature check validation is enabled only when trim analysis is enabled.
if (context.TrimAnalyzer is null)
return;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public HandleCallAction(
_owningSymbol = owningSymbol;
_operation = operation;
_isNewObj = operation.Kind == OperationKind.ObjectCreation;
_diagnosticContext = new DiagnosticContext(location, reportDiagnostic);
_diagnosticContext = new DiagnosticContext(location, typeNameResolver.Compilation, reportDiagnostic);
_annotations = FlowAnnotations.Instance;
_reflectionAccessAnalyzer = new(reportDiagnostic, typeNameResolver, typeHierarchyType: null);
_typeNameResolver = typeNameResolver;
Expand Down
8 changes: 4 additions & 4 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ private void ReportRequiresUnreferencedCodeDiagnostic(Location location, Attribu
{
var message = RequiresUnreferencedCodeUtils.GetMessageFromAttribute(requiresAttributeData);
var url = RequiresAnalyzerBase.GetUrlFromAttribute(requiresAttributeData);
var diagnosticContext = new DiagnosticContext(location, _reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, _typeNameResolver.Compilation, _reportDiagnostic);
diagnosticContext.AddDiagnostic(DiagnosticId.RequiresUnreferencedCode, member.GetDisplayName(), message, url);
}

Expand Down Expand Up @@ -146,7 +146,7 @@ static bool IsDeclaredWithinType(ISymbol member, INamedTypeSymbol type)
if (reportOnMember)
location = DynamicallyAccessedMembersAnalyzer.GetPrimaryLocation(member.Locations);

var diagnosticContext = new DiagnosticContext(location, _reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, _typeNameResolver.Compilation, _reportDiagnostic);

if (member.IsInRequiresUnreferencedCodeAttributeScope(out AttributeData? requiresUnreferencedCodeAttribute))
{
Expand All @@ -166,7 +166,7 @@ static bool IsDeclaredWithinType(ISymbol member, INamedTypeSymbol type)

internal void GetDiagnosticsForReflectionAccessToDAMOnMethod(Location location, IMethodSymbol methodSymbol)
{
var diagnosticContext = new DiagnosticContext(location, _reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, _typeNameResolver.Compilation, _reportDiagnostic);
if (methodSymbol.IsVirtual && FlowAnnotations.GetMethodReturnValueAnnotation(methodSymbol) != DynamicallyAccessedMemberTypes.None)
{
diagnosticContext.AddDiagnostic(DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection, methodSymbol.GetDisplayName());
Expand Down Expand Up @@ -215,7 +215,7 @@ private void GetDiagnosticsForField(Location location, IFieldSymbol fieldSymbol)

if (FlowAnnotations.GetFieldAnnotation(fieldSymbol) != DynamicallyAccessedMemberTypes.None)
{
var diagnosticContext = new DiagnosticContext(location, _reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, _typeNameResolver.Compilation, _reportDiagnostic);
diagnosticContext.AddDiagnostic(DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection, fieldSymbol.GetDisplayName());
}
}
Expand Down
4 changes: 2 additions & 2 deletions .../illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ public RequireDynamicallyAccessedMembersAction(
_reportDiagnostic = reportDiagnostic;
_reflectionAccessAnalyzer = reflectionAccessAnalyzer;
_owningSymbol = owningSymbol;
_diagnosticContext = new(location, reportDiagnostic);
_diagnosticContext = new(location, typeNameResolver.Compilation, reportDiagnostic);
}

public partial bool TryResolveTypeNameAndMark(string typeName, bool needsAssemblyName, out TypeProxy type)
{
var diagnosticContext = new DiagnosticContext(_location, _reportDiagnostic);
var diagnosticContext = new DiagnosticContext(_location, _typeNameResolver.Compilation, _reportDiagnostic);
if (_reflectionAccessAnalyzer.TryResolveTypeNameAndMark(typeName, diagnosticContext, needsAssemblyName, out ITypeSymbol? foundType))
{
if (foundType is INamedTypeSymbol namedType && namedType.IsGenericType)
Expand Down
2 changes: 1 addition & 1 deletion ...ls/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisBackingFieldAccessPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public TrimAnalysisBackingFieldAccessPattern Merge(

public void ReportDiagnostics(DataFlowAnalyzerContext context, Action<Diagnostic> reportDiagnostic)
{
DiagnosticContext diagnosticContext = new(Operation.Syntax.GetLocation(), reportDiagnostic);
DiagnosticContext diagnosticContext = new(Operation.Syntax.GetLocation(), context.Compilation, reportDiagnostic);
foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
requiresAnalyzer.CheckAndCreateRequiresDiagnostic(Operation, Property, OwningSymbol, context, FeatureContext, in diagnosticContext);
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisFieldAccessPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public TrimAnalysisFieldAccessPattern Merge(

public void ReportDiagnostics(DataFlowAnalyzerContext context, Action<Diagnostic> reportDiagnostic)
{
DiagnosticContext diagnosticContext = new(Operation.Syntax.GetLocation(), reportDiagnostic);
DiagnosticContext diagnosticContext = new(Operation.Syntax.GetLocation(), context.Compilation, reportDiagnostic);
foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
requiresAnalyzer.CheckAndCreateRequiresDiagnostic(Operation, Field, OwningSymbol, context, FeatureContext, in diagnosticContext);
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisMethodCallPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public void ReportDiagnostics(DataFlowAnalyzerContext context, Action<Diagnostic
InvocationExpressionSyntax invocationSyntax => invocationSyntax.Expression.GetLocation(),
_ => location
};
var diagnosticContext = new DiagnosticContext(location, reportDiagnostic);
var diagnosticContext = new DiagnosticContext(location, context.Compilation, reportDiagnostic);
foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
{
if (!requiresAnalyzer.IsIntrinsicallyHandled(CalledMethod, Instance, Arguments))
Expand Down
2 changes: 1 addition & 1 deletion ...ools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisReflectionAccessPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void ReportDiagnostics(DataFlowAnalyzerContext context, Action<Diagnostic
reflectionAccessAnalyzer.GetDiagnosticsForReflectionAccessToDAMOnMethod(location, ReferencedMethod);
}

DiagnosticContext diagnosticContext = new(location, reportDiagnostic);
DiagnosticContext diagnosticContext = new(location, context.Compilation, reportDiagnostic);
foreach (var requiresAnalyzer in context.EnabledRequiresAnalyzers)
requiresAnalyzer.CheckAndCreateRequiresDiagnostic(Operation, ReferencedMethod, OwningSymbol, context, FeatureContext, diagnosticContext);
}
Expand Down
2 changes: 2 additions & 0 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TypeNameResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public struct TypeNameResolver
{
readonly Compilation _compilation;

internal readonly Compilation Compilation => _compilation;

static readonly TypeNameParseOptions s_typeNameParseOptions = new() { MaxNodes = int.MaxValue };

public TypeNameResolver(Compilation compilation)
Expand Down
Loading