-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Warn on field keyword references in trim analyzer #117072
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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 introduces a new warning for dataflow assignments that use the literal 'field' keyword as a source and updates many test cases and diagnostics accordingly. Key changes include:
- Updating ExpectedWarning attributes in numerous test files to include GitHub URLs (e.g. Consider detecting unnecessary annotations linker#1971, Trim analyzer has better control flow graph than illink and ilc #102830) and "NativeAOT-specific warning" strings.
- Modifying the trim analysis visitor and related classes to better handle backing field access and property dataflow.
- Adding new overloads and adjustments related to generic argument dataflow and backing field annotations.
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs | Added VisitInvalid override to detect a 'field' keyword and updated GetFieldTargetValue to use a new helper for backing fields. |
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FieldValue.cs | Introduced a constructor overload for IPropertySymbol to support backing field analysis. |
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs | Added a branch for backing field annotation that currently throws NotImplementedException. |
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs | Added abstract GetBackingFieldTargetValue to support property backing field dataflow. |
Multiple test files under src/tools/illink/test/Mono.Linker.Tests.Cases/… | Updated warning attributes to align with new diagnostic messages and include GitHub issue URLs. |
Comments suppressed due to low confidence (3)
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs:97
- Consider documenting the rationale for checking if operation.Syntax.ToString() equals 'field' in VisitInvalid, to ensure that this string-based check remains robust against potential syntax changes.
public override MultiValue VisitInvalid(IInvalidOperation invalidOperation, StateValue argument)
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FieldValue.cs:19
- The addition of a constructor overload in FieldValue to support IPropertySymbol is clear; please ensure that treating property backing fields similar to regular fields in dataflow analysis is intended and sufficiently documented.
public FieldValue(IPropertySymbol propertySymbol)
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs:134
- The introduction of GetBackingFieldTargetValue improves handling of property backing fields in dataflow analysis. Please verify that all call sites invoking field dataflow now account for property references appropriately.
public abstract TValue GetBackingFieldTargetValue(IPropertyReferenceOperation propertyReference, in TContext context);
throw new NotImplementedException($"Field {field.GetDisplayName()} is not implemented in FlowAnnotations.GetFieldAnnotation. This should never happen, please report a bug."); | ||
// return property.GetDynamicallyAccessedMemberTypes (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch in GetFieldAnnotation for fields that have an associated symbol (i.e. backing fields for properties) currently throws a NotImplementedException. Ensure that this scenario is properly handled before finalizing the change or add a detailed TODO comment for future implementation.
throw new NotImplementedException($"Field {field.GetDisplayName()} is not implemented in FlowAnnotations.GetFieldAnnotation. This should never happen, please report a bug."); | |
// return property.GetDynamicallyAccessedMemberTypes (); | |
if (field.AssociatedSymbol is IPropertySymbol property) | |
{ | |
return GetBackingFieldAnnotation(property); | |
} | |
else | |
{ | |
// TODO: Handle other cases of associated symbols if necessary | |
Debug.Fail($"Unexpected associated symbol type for field {field.GetDisplayName()} in FlowAnnotations.GetFieldAnnotation."); | |
return DynamicallyAccessedMemberTypes.None; | |
} |
Copilot uses AI. Check for mistakes.
Tagging subscribers to this area: @dotnet/illink |
Depends on #117034
Adds warning when a 'field' keyword is the source in a dataflow assignment.
Does not warn when it is the target of an assignment yet.