Adding generation of substitution files for trimming out resources when feature switch is present

[joperezr]

Contributes to #34057
Fixes #37587

cc: @eerhardt @tarekgh

These changes will make it such that all of the assemblies we ship which also have resources will no also contain a Substitutions file embedded such that if the user selects either the appContext switch or sets a property (that needs to be defined and added on the linker targets) on the project, then all resources will be removed and only resource keys will be used instead. I'm waiting on an arcade change in order to be able to add tests for this which will make sure that setting the runtimeconfig switch will cause this to happen but that will be added into this PR.

FYI: @danmosemsft @vitek-karas @marek-safar

[ghost]

Tagging subscribers to this area: @tarekgh, @buyaa-n, @krwq
Notify danmosemsft if you want to be subscribed.

[tarekgh]

added comment/question. otherwise LGTM.

[joperezr]

@tarekgh I made this change due to the discussion #37587 (comment)

Do you mind taking a look at this?

[tarekgh]

Is it possible to get rid of this file and use the one in the common? we can look quickly at the differences. both should be providing the same logic. or if you want we can do that in a separate PR.

[marek-safar]

This file is extension of the common file, we cannot get rid of it

[joperezr]

From the discussion on that issue it may be that it there is a problem as some codegen from arcade might be calling this method specifically so removing might not be so simple, although I may have misinterpreted the discussion. I would prefer to scope the change to only this for now so that if Arcade gets broken by removing the method we don't have to revert this whole PR

[joperezr]

This file is extension of the common file, we cannot get rid of it

I may have misread but I thought what Tarek meant was more to remove this method, in favor of the one in the other overload defined in Common.

[tarekgh]

if this is the case, the SR.cs that in the common already have the method:

  internal static string GetResourceString(string resourceKey, string? defaultString = null)

so I guess we should delete GetResourceString from here then.

---

In reply to: 445786809 [](ancestors = 445786809)

[tarekgh]

Also the SR.cs in common is just ifdef stuff for corelib which we can get rid of now as the ResourceManager became a part of corelib already. that means we can get rid of SR.cs which is in the corelib. as I mentioned we can make this in other PR if we need to.

---

In reply to: 445792039 [](ancestors = 445792039,445786809)

[marek-safar]

I don't think such change should be part of this PR

[jkotas]

"SR" sounds pretty cryptic. Is System.SR.UsingResourceKeys a good public facing name? Should it rather be System.Resources. ... ?

[tarekgh]

SR in general is internal and not public. Are you talking about the name used here in these targets? does it matter much?

[joperezr]

it will be public in the sense that people can pass that in as extra args to the trimmer if they want to manually define it that way. Most people though will be instead using a property that will be added to the SDK that will control this feature switch, with a name like:

<PropertyGroup>
  <TrimResources>true</TrimResources>
</PropertyGroup>

or something like that. All that said though, I do agree with @jkotas point plus we are also using the same name witht he AppContext switch which will be definitely "more public" as people will be setting this on their runtimeconfig file. I will change the name.

[eerhardt]

One concern I have with System.Resources.UseResourceKeys is that it sounds like it affects all resources, not just the exception messages coming from the runtime.

Is there a word/term/name we can use to make it specific to the runtime resources?

• System.Resources.UseSystemResourceKeys
• System.Resources.UseBclResourceKeys
• System.Resources.UseSRResourceKeys

[vitek-karas]

AppContext switch which will be definitely "more public" as people will be setting this on their runtimeconfig file. I will change the name

while it's possible to set this directly in .runtimeconfig.json I don't expect many people actually doing it. We expect the vast majority to use MSBuild property to drive this.

So we need a really good "customer facing" name for the MSBuild property.
And we need a good "technical" name for the runtime option/AppContext switch.

What the options does:
When enabled, all string resources in our framework assemblies will be removed and replaced with the resource keys instead. Framework only uses string resources for exception messages (I'm not aware of any other usage), so the effect of this change is for example (aside from saving quite a bit of size):

System.ArgumentNullException: Value cannot be null. (Parameter 'args')

turns into

System.ArgumentNullException: ArgumentNull_Generic Arg_ParamName_Name, args

Customer facing name should reflect the behavioral change - so maybe something along the lines of UseResourceKeysForExceptionMessages (this is a bad name, but the intent is there).

Technical name should be something closer to the code base, so maybe System.Resources.UseStringSystemResourceKeys...

Adding @samsp-msft who might have better ides about naming.

[jkotas]

Framework only uses string resources for exception messages (I'm not aware of any other usage)

Here are examples of resource uses for strings that are not exception messages:

• https://github.com/dotnet/runtime/search?q=Globalization_cp_&unscoped_q=Globalization_cp_
• https://github.com/dotnet/runtime/search?q=Word_At&unscoped_q=Word_At

[vitek-karas]

Thanks @jkotas! I guess it's OK to remove resources for those as well... it just makes the naming more complicated...

[eerhardt]

Here are examples of resource uses for strings that are not exception messages:

I assume for .NET Native, which used this same switch, those places were broken as well? For example in a StackTrace.ToString() you would get Word_At instead of at?

[jkotas]

That's what defaultString argument of GetResourceString method was designed for. IIRC, the most egregious cases were fixed by passing in a sensible default string into this API in .NET Native. I do not remember whether Word_At got this treatment.

[eerhardt]

That's what defaultString argument of GetResourceString method was designed for.

I see. My assumption was that defaultString was hard-coded in C# during Debug builds, so we had nice exception messages when we were debugging. But I guess it would make sense to use it elsewhere as well.

[eerhardt]

LGTM.

It would be great if we can analyze the places that use SR for non-exception messages to ensure we aren't breaking something there.

[joperezr]

Merging given the only change since clean CI was fixing a typo on a test name and removing a diagnostic message in one of our targets.