-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Track assignment to property backing field #117034
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
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 addresses #93277 by ensuring that writes to property backing fields are tracked consistently with field access. Key changes include updating ExpectedWarning attributes in test cases, refactoring methods in TrimAnalysisVisitor and LocalDataFlowVisitor to use new overloads for property access, and adding support for property-specific dataflow patterns.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs | Removes extra tool flags from ExpectedWarning attributes to match the new warning format. |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructorDataFlow.cs | Updates ExpectedWarning attributes by removing tool flags while preserving CompilerGeneratedCode marking. |
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs | Updates GetFieldTargetValue usage to use IFieldReferenceOperation and adds new property access handling. |
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs | Introduces a branch for handling property assignments via GetPropertyTargetValue to correctly capture backing-field writes. |
Other files | Introduce new overloads and helper methods for processing property symbols in generic dataflow and flow annotations. |
Comments suppressed due to low confidence (4)
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs:492
- The ExpectedWarning attribute now omits the Tool flags and additional parameters; please confirm that this change is intentional and that test filtering or message interpretation isn’t affected.
[ExpectedWarning("IL2074", nameof(WriteToGetOnlyProperty), nameof(GetUnknownType))]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructorDataFlow.cs:42
- Similar to the PropertyDataFlow changes, the removal of Tool flags from the ExpectedWarning attribute should be verified to ensure that it remains aligned with the test expectations for compiler-generated code.
[ExpectedWarning("IL2074", nameof(GetUnknown), nameof(AnnotatedProperty), CompilerGeneratedCode = true)]
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs:179
- The API change to call GetFieldTargetValue with an IFieldReferenceOperation instead of separately passing the field symbol is acceptable, but please double-check that all necessary field-related details are handled appropriately with this new signature.
return GetFieldTargetValue(fieldRef, in current.Context);
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs:288
- The new branch for handling property assignments through GetPropertyTargetValue is a good step toward addressing backing field writes; please ensure that there are adequate unit tests verifying the behavior in both property initializer and constructor scenarios.
var current = state.Current;
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
Outdated
Show resolved
Hide resolved
- GetBackingFieldAnnotation instead of GetFieldAnnotation - Rename other "Property" types/methods to "BackingField" for consistency
...tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisBackingFieldAccessPattern.cs
Show resolved
Hide resolved
…alysisBackingFieldAccessPattern.cs Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
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.
LGTM
I'm curious about the property/backing field complexity. Is this all because compiler-generated backing fields don't get the attributes applied to properties?
It's because a write to a property can lower to either a call to the getter or a write to the compiler-generated backing field (the case this PR is addressing). The CFG represents both with an |
Fixes #93277.
This detects the case where a write to a property actually becomes a write to the compiler-generated backing field. It follows the same logic and produces the same warning as field access, matching ILLink/ILC.