Warn on field keyword references in trim analyzer

[jtschuster]

Adds warning when a 'field' keyword is the source or target in a dataflow assignment. The IFieldSymbol of in these scenarios take on the annotation of the associated property.

[Copilot]

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. https://github.com/dotnet/linker/issues/1971, #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);

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

Can we instead make equivalent update to ILC's copy of FlowAnnotations.cs? We don't allow these shared files to diverge; it's a recipe for trouble.

I'm a bit suspicious of the FlowAnnotations change. I would instead tighten the heuristic to make sure it really only works on get; set; properties like we originally intended before the field keyword. The ScanMethodBodyForFieldAccess is a heuristic that only really works for get; set; properties and can come up with confusing conclusions for arbitrary IL.

Or to put it this way: if we want to allow arbitrary IL in get/set accessors of properties marked DAM, why not drop the IsCompilerGenerated filter:

runtime/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs

Line 525 in c547a55

!found.IsCompilerGenerated())

[jtschuster]

Looks like some of the changes in ILCompiler's FlowAnnotations actually do unify the behavior, for some reason the build process wasn't copying the latest built ILCompiler.dll to the test directory in local tests.

I think it would still make sense to slightly loosen the heuristic to work on properties with the field keyword. I can see this being commonly used for lazy properties and input validation, which would have more than one store/read of the same field for the accessors. I can't think of any situation where a compiler generated field is the only field accessed in an accessor that isn't the compiler generated backing field.

[sbomer]

I overall prefer to avoid reversing compiler-generated code, but this feels aligned with what we already do for get; set; properties, and I also can't think of any problems with the current approach.

[sbomer]

Actually maybe there are problems if the generated field isn't used as a backing store for the value. I couldn't come up with a realistic example, but it's possible to write code where the generated field shouldn't necessarily inherit the property annotations:

using System;
using System.Diagnostics.CodeAnalysis;

C.Type = typeof(Value); // warns for Value.RUCMethod

public class C
{
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    static Type? type;

    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    static public Type? Type
    {
        get
        {
            if (field == typeof(Sentinel))
                return type;
            return null;
        }
        set
        {
            if (value == null)
            {
                field = null;
                type = null;
            }
            else
            {
                field = typeof(Sentinel); // with this change, would preserve Sentinel.Unused
                type = value;
            }
        }
    }
}

class Sentinel {
    [RequiresUnreferencedCode("Unused")]
    public static void Unused() { }
}

class Value()
{
    [RequiresUnreferencedCode("RUCMethod")]
    public static void RUCMethod() { }
}

[MichalStrehovsky]

Yep - that's what I mean - we currently restrict the propagation attempt to IsCompilerGenerated field.

But it's basically a dice roll (as far as the user is concerned) whether it will propagate correctly to where it should or propagate incorrectly and result in more preservation or warnings or fail to propagate.

At that point we could just drop the IsCompilerGenerated condition and let it try doing magic everywhere. (I am not a fan of doing that and not actually proposing it.)

[sbomer]

I agree with @MichalStrehovsky's comment - I'd prefer not to propagate the property annotation to field. The compiler makes no guarantee that it's actually used as a backing field for the property, so we shouldn't assume that (even if it's a nice convenience feature).

Could we limit the auto-propagation to compiler-generated properties?

[sbomer]

I think we shouldn't do this propagation at all, not just when it doesn't conflict with existing annotations. It shouldn't be required to place any annotations on the field.

[jtschuster]

I agree, I stuck with this because it felt like it would be consistent with the trimmer and ilc more often since the IL generated for semi-auto/field keyword properties can be the identical as full-auto properties. Whether we treat the field as having no annotation or treat it as having the same as the property, we get potential inconsistencies with the tools, at least the way they detect auto-properties now.

I think we can align behavior better if we tighten the compiler generated field finder to make sure the getter and setter are compiler generated as well. We currently don't check that, but it looks like only full-auto properties have the attribute so we can be sure we're only propagating the annotations for full-auto properties where all accessors are compiler generated. The only concern then is if that breaks existing code.

https://lab.razor.fyi/#vVLLbtRAEBSIQ3ZOiC9oTuxe7A-wlmiVKBGIVSKyKBIIidlxZ2gx7tlMz4RYUT4GCS6c-Yvc-R5kO9mHleUE8cVydbWrq7rVr4FSx8HboKvMyLMfgyTEFk5qiVgVav0r2_POoYnkWbJDZAxk_sJ4VVUp6rnDHmeftGUvkYxsr2R7vsQJa1cL9WlviM970OxzQF0S2214NtPyRQqljNMicOtYXSkAgA_7NeuKjHaunhiDIlhOsZpjkOHW0qxeoGTHae7IHBC6UkYf278tWggk6kgGGhqcYEWTFD10es1jMcL4JZw1ncUSlTV0DBfaJeyK1-phJj3EeMSubobg5FzxQLIHybkuIIuxaGIo_ovjPIdTHRiIG42IYReOApSeX0RYBL_QVkeE6LsNgA-3NMlt997d5mBKl6vhlzuEcZPoO9bMPuqI5XB0Z-z-8DeovS1sdlx4KuEtnicKKJ3J4T8Lqx1HBztqhVdXGzCmwBsnuT7OVBMP-z1NMuPWx_PVoed5i5MA-6-QVq6XjJ61KV2O7nSv1evH5N4Pdr7__nnz7ezpk0-P_gA

[sbomer]

My vote is for tightening it to work only for compiler-generated getters or setters. I would only be concerned with breaking existing code if the compiler didn't always emit CompilerGeneratedAttributes on the get_/set_ method - I think we should go ahead and try it. @MichalStrehovsky @agocke any opinions on this?

(for get_Mix I would probably leave the get_ case working as-is, and warn in the set_ accessor - anything else probably requires extra work)

[agocke]

What's the situation with CompilerLoweringPreserve? I would expect the compiler to automatically lower to the backing field now. That would probably influence my perspective.

[jtschuster]

CompilerLoweringPreserve doesn't propagate to otherwise annotatable things. Since the field can still be annotated with [field: DAM], the attribute isn't propagated.

[agocke]

Ah, well, in that case can we avoid being too clever, since the user can just annotate the field?

I'm not sure what all the options are and what are their consequences. What's the current behavior and what are the possible new behaviors? And would this introduce any breaks?

[MichalStrehovsky]

My vote is for tightening it to work only for compiler-generated getters or setters. I would only be concerned with breaking existing code if the compiler didn't always emit CompilerGeneratedAttributes on the get_/set_ method - I think we should go ahead and try it. @MichalStrehovsky @agocke any opinions on this?

(for get_Mix I would probably leave the get_ case working as-is, and warn in the set_ accessor - anything else probably requires extra work)

I would vote for this too. Add an extra restriction that we'll only look for the compiler generate field if the getter/setter is also compiler generated.

I think the only thing this could break (before the field keyword was introduced) is code where the user went and explicitly added a CompilerGenerated attribute to their field. Otherwise I don't see a way to access the backing field of a property from a non-compilergenerated method.

[jtschuster]

Closing this and tightening the propagation heuristic in #119329.