Skip to content

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

Merged
merged 3 commits into from
Jun 30, 2025
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jun 26, 2025

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.

@sbomer sbomer requested review from a team and Copilot June 26, 2025 01:06
@sbomer sbomer requested a review from marek-safar as a code owner June 26, 2025 01:06
@github-actions github-actions bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jun 26, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 26, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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;

- GetBackingFieldAnnotation instead of GetFieldAnnotation
- Rename other "Property" types/methods to  "BackingField" for consistency
…alysisBackingFieldAccessPattern.cs

Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
Copy link
Member

@agocke agocke left a 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?

@sbomer
Copy link
Member Author

sbomer commented Jun 30, 2025

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 IPropertyReferenceOperation, and there's no way I could find to get the compiler-generated backing field in the latter case.

@sbomer sbomer merged commit cf9a0e1 into dotnet:main Jun 30, 2025
67 of 76 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

Trim analyzer should warn about assignment to get-only property
3 participants