Add Link-time framework feature removal spec

[MichalStrehovsky]

A sample of how this would work is here: MichalStrehovsky/corefx@2a7fa76

If we had a switch like the one in the linked sample usage commit, the People app that ships with Windows would get 1.2 MB smaller on AMD64 with post-NetStandard2.0 .NET Native because we would no longer need to carry the full HTTP/HTTPS/FTP stack just because the People app uses DataContractSerializer.

Other samples:

• Making the reflection fallback in DataContractSerializers optional saves 335 kB.
• Making the reflection fallback in XmlSerializers optional saves 410 kB.

[MichalStrehovsky]

Cc @jkotas @marek-safar @morganbr @davidwrighton @stephentoub

[sergiy-k]

Thanks @MichalStrehovsky! This looks great. The only question I have is what everyone thinks about the names of attributes? After reviewing multiple samples, it seems to me that the 'Removable' name is quite ambiguous and it does not speak clearly for its purpose. Maybe something more concrete like RemovableFeatureRoot attribute or just FeatureRoot attribute would be better? I'm not super good at coming up with good names so please feel to ignore. Thank you.

[MichalStrehovsky]

The only question I have is what everyone thinks about the names of attributes?

I could file an issue over in the CoreFX repo and run the attribute through the API review process. They tend to also comment on names. It's not critical, but maybe having something approved saves us from having to process two attributes (unofficial and official one) in the future.

[marek-safar]

@MichalStrehovsky please run the names via API review. We have more attributes we want to add/import and it might even make a sense to have them in a new namespace.

[MichalStrehovsky]

Filed dotnet/corefx#30643.

[terrajobst]

My primary concern with the feature as proposed is that it's hard to diagnose issues introduced by incorrect attribution if the default behavior is returning zero/null.

I'd rather we FailFast by default. Looking at your example I do however buy the scenario that you want to define query methods for features, which is as easy as doing this:

private static bool SupportsReflectionBasedSerializationFeature
{
    [Removable(ReflectionBasedSerializationFeatureName)]
    get
    {
        return true;
    }
}

My gut feel is -- and that might be wrong -- that the vast majority of attribute applications will be implementations of regular methods. So maybe we should be using two different attributes or just reverse the FailFast property:

private static bool SupportsReflectionBasedSerializationFeature
{
    [Removable(ReflectionBasedSerializationFeatureName, DoNotFail=true)]
    get
    {
        return true;
    }
}

[terrajobst]

Do we provide an accordance where the developer can manually specify the list of features the linker has to keep regardless of reachability? This would allow developers to work around problems with the linker in case a method fails fast by just keeping the feature entirely.

[MichalStrehovsky]

Do we provide an accordance where the developer can manually specify the list of features the linker has to keep regardless of reachability? This would allow developers to work around problems with the linker in case a method fails fast by just keeping the feature entirely.

Feature removal is opt in - the developer has to specify the name of the feature to remove and only then the body replacement happens. If we want the linker not to do that, we would simply not ask it to strip the feature.

Or are you asking for this attribute to serve a double purpose (acting like a conditional [Preserve] attribute?). Do you have a scenario where this would be useful? The motivation behind the proposed feature is to allow treeshaking in places that are reachable in the static call graph (the fact that we're keeping the methods is the problem; the problem is not their unintended removal) - I don't see a scenario where this would be beneficial.

[danmoseley]

What's the status of this ?