Fix feature switch IL when feature attributes are removed

[sbomer]

When removing FeatureSwitchDefinition and FeatureGuard attributes, we need to ensure that the body rewriting logic still respects these attributes even though the rewriting happens after attribute removal. Caching the substituted constant value fixes this.

More detail:

When feature switches or feature guards are substituted with a constant, the IL rewriting of the feature property happens after SweepStep, during CodeRewriterStep. At this point, any attributes marked for removal by RemoveAttributeInstancesAttribute have already been swept. When removing FeatureSwitchDefinitionAttribute and FeatureGuardAttribute, as we do with AggressiveAttributeTrimming, this means that CodeRewriterStep fails to substitute a constant for attributed feature check or feature guard properties.

Normally, the attributed feature properties would be removed entirely when substituted, so CodeRewriterStep wouldn't even visit the property. However, it's possible that the caller of the property was a copy assembly where the callsite can't be rewritten to return a constant.

When this happens, we can end up with a property that's not stubbed out during CodeRewriterStep, even though we didn't mark a callee of the property (because MarkStep did treat the property as a constant and didn't mark its instructions). When trying to write out the property's IL this results in a failure because we see a reference to a method that was removed.

Fixes #104945

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[vitek-karas]

How do we guarantee this will be called at least once on a given method before the CodeRewriterStep?
If I understand the repro for this problem, the callsites in trimmed assemblies are all removed due to branch removal, so we won't mark them and thus won't call this either. This leaves copy assemblies and their references. I don't remember exactly what type of processing we do there - if we end up calling MarkMethod then this should be fine.

I guess the reason why this works is:
If the CoreRewriterStep gets to process a method, that method had to be marked (otherwise it would have been removed) and thus MarkMethod ended up calling this TryGetFeatureCheckValue and thus populate the cache within.

I must admit this makes me a bit nervous - it feels pretty fragile. Maybe we should make the link from MarkMethod to this a bit more obvious - currently it goes through GetMethodAction which doesn't "seem" like being related to this that much.

[sbomer]

If the CoreRewriterStep gets to process a method, that method had to be marked (otherwise it would have been removed) and thus MarkMethod ended up calling this TryGetFeatureCheckValue and thus populate the cache within.

Yes, exactly.

It is a bit fragile. I thought of a way to get to to TryGetFeatureCheck in CodeRewriterStep that's not cached - it's possible to stub a body (with XML or SetAction) without also setting a stubbed value (even though we usually do both). If that's done for a method that also has a feature attribute, the stub value might not have been precomputed.

I added a test for this scenario and added a call to TryGetMethodStubValue in MarkMethod. I think that addresses your concern.

[vitek-karas]

Thanks!