feature-switch-patterns.md

I took a closer look at how we handled this for resources and now I understand @vitek-karas' point that it's not so different from the StartupHook case. I personally find this mind-boggling and would like to solve it differently. In my mind (maybe this is naiive) we should be able to reason simply about dependencies between feature switches. Imagine features A and B. If A always depends on B, things are straightforward:

[RUC("A")]
void DoA() {
    if (!A.IsSupported) throw;
    AImpl();
    DoB(); // no warning
}

[RUC("A")]
void AImpl() {}

[RUC("B")]
void DoB() {
    if (!B.IsSupported) throw;
    // ...
}

This won't warn for the dependency A -> B, but it'll warn if anything else tries to use A or B.

Now imagine the dependency is optional:

[RUC("A")]
void DoA() {
    if (!A.IsSupported) throw;
    AImpl();
    if (B.IsSupported) DoB(); // no warning
}

Not much changes - the warning is still suppressed due to the RUC, whether B is enabled or disabled. Then let's say feature A doesn't have RUC:

[USM]
void DoA() {
    if (!A.IsSupported) throw;
    AImpl();
    if (B.IsSupported) DoB();
}

void AImpl() {} // trim-friendly

In this case, we have the problem that callers of DoA won't warn even if feature A is disabled, but that's a separate issue (we might want to add some tooling to give build-time warnings for unguarded calls to disabled features in general).

For the dependency from A -> B, is it ok to suppress the warning? Branch elimination means the linker won't warn if B.IsSupported == false, but the warning is still there when there are no feature settings, or if B.IsSupported == true.

I would argue yes - the author of A has gone through the trouble to guard calls to DoB, and DoA does useful work even without B. If the app author enables feature B, that will cause A to use the "unsafe" path for B. But we shouldn't blame A for this; we should blame the app author for turning on B which is unsupported when trimming.

What if there isn't a nice fallback behavior - what if it throws?

void DoA() {
    if (!A.IsSupported) throw;
    AImpl();
    if (B.IsSupported) DoB();
    else throw;
}

Then I'd argue A has a "hard" dependency on B, and the dependency should be expressed via RUC and the feature switch to turn the runtime failure into a build-time warning. It should not introduce a separate "category" of RUC or feature switch for this. Instead it should be written as:

[RUC("B")]
void DoA() {
    if (!B.IsSupported) throw;
    AImpl();
    DoB();
}

In none of the above cases would you get a warning from A's internals. I think we should follow the same patterns in the library feature switches.

Compare this with the current approach we have for custom resource types and binary formatter. There's a feature AllowCustomResourceTypes which when enabled lights up optional code paths for 1. reading custom resource types and 2. deserializing from the resources.

Here are the important bits extracted as pseudocode:

CreateResourceSet() {
    if (CanUseDefaultResourceClasses()) return;
    // ^In a lot of cases, we don't hit the custom resource type code
 
    if (AllowCustomResourceTypes) InternalGetResourceSetFromSerializeData() // warns if AllowCustomResourceTypes == true
    else throw;
}

[RUC("CustomResourceTypesSupport is incompatible with trimming")] // user-visible message
InternalGetResourceSetFromSerializedData() {
   new ResourceReader(..., permitDeserialization: true);
}

[USM("when AllowResourceTypes==true, we already warn above")]
ResourceReader.DeserializeObject() {
    if (!AllowCustomResourceTypes) throw;
    if (!permitDeserialization) throw;
    InitializeBinaryFormatter(); // warning suppressed
}

[RUC("CustomResourceTypesSupport is incompatible with trimming")] // we suppress warnings for calls to this method
ResourceReader.InitializeBinaryFormatter() {
    Type.GetType("BinaryFormatter").GetMethod("Deserialize").Invoke();
}

[RUC]
BinaryFormatter.Deserialize() {
    if (!BinaryFormatterEnabled) throw;
    // ...
}

Abstracting from this a little:

void DoA1() {
  if (HappyPath()) return; // happy path which is trim-friendly
  if (A.IsSupported) AImpl1(); // warns if A.IsSupported != false
  else throw;
}

[RUC("A")]
void AImpl1() {
  new FeatureA(useB: true);
}

[USM]
void FeatureA.DoA2() {
  if (!A.IsSupported) throw;
  if (!useB) throw;
  AImpl2(); // suppressed
}

[RUC("A")]
void AImpl2() {
  DoB(); // suppressed by RUC
}

[RUC("B")]
void DoB() {
  if (!B.IsSupported) throw;
  // ...
}

this is roughly equivalent to:

void DoA1() {
  if (HappyPath()) return; // happy path which is trim-friendly
  if (A.IsSupported) AImpl1(); // warns if A.IsSupported != false
  else throw;
}

[RUC("A")]
void AImpl1() {
  new FeatureA(useB: true);
}

[USM]
void FeatureA.DoA2() {
  if (A.IsSupported) DoB(); // suppressed
  else throw;
}

This pattern doesn't really extend well to other cases. The warning is suppressed only because we presumably have some guarantee that DoA2 can't be called independently of DoA1. (I am not even sure this is true for ResourceManager - couldn't you hit the deserialization code path even without custom resource types?) If there are other entry points to feature A that depend on B, then there would be multiple warnings if linking with A.IsSupported.

Could we instead change this to:

[USM]
void DoA1() {
  if (HappyPath()) return; // happy path which is trim-friendly
  if (B.IsSupported) AImpl1(); // suppressed
  else throw;
}

[RUC("B")]
void AImpl1() {
  new FeatureA(useB: true);
}

[USM]
void FeatureA.DoA2() {
  if (B.IsSupported) DoB(); // suppressed
  else throw;
}

or for ResourceManager:

[USM]
CreateResourceSet() {
    if (CanUseDefaultResourceClasses()) return;
    // ^In a lot of cases, we don't hit the custom resource type code
 
    if (BinaryFormatterEnabled) InternalGetResourceSetFromSerializeData() // suppressed
    else throw;
}

[RUC("BinaryFormatter")]
InternalGetResourceSetFromSerializedData() {
   new ResourceReader(..., permitDeserialization: true);
}

[RUC("BinaryFormatter")]
ResourceReader.DeserializeObject() {
    if (!BinaryFormatterEnabled) throw;
    InitializeBinaryFormatter(); // warning suppressed
}

[RUC("BinaryFormatter")]
ResourceReader.InitializeBinaryFormatter() {
    Type.GetType("BinaryFormatter").GetMethod("Deserialize").Invoke();
}

[RUC("BinaryFormatter")]
BinaryFormatter.Deserialize() {
    if (!BinaryFormatterEnabled) throw;
    // ...
}

This would produce no warnings from ResourceReader, even if BinaryFormatter is enabled. Then we would just need to define somewhere (probably in the SDK) the set of features which are incompatible with trimming, and warn if any of these are enabled.