Fix the TypeConverter when enabling UseSystemResourceKeys

[tarekgh]

Fixes #68651

When enabling UseSystemResourceKeys msbuild property, the build will strip out all string resources and any resource lookup will report the resource key instead of the resource value. This is mainly used when reducing the size of the built binaries. In TypeConverter, this will cause a problem because there are some resources that have values with special meaning. Reporting the resource key in such cases will make the requested conversion operation fail as the converter will not recognize the value picked up from the resources.

The fix is to apply @jkotas suggestion mentioned in the comment #68651 (comment).

[ghost]

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

Issue Details

---

Fixes #68651

When enabling UseSystemResourceKeys msbuild property, the build will strip out all string resources and any resource lookup will report the resource key instead of the resource value. This is mainly used when reducing the size of the built binaries. In TypeConverter, this will cause a problem because there are some resources that have values with special meaning. Reporting the resource key in such cases will make the requested conversion operation fail as the converter will not recognize the value picked up from the resources.

The fix is to apply @jkotas suggestion mentioned in the comment #68651 (comment).

Author:	tarekgh
Assignees:	-
Labels:	area-System.ComponentModel
Milestone:	-

[tarekgh]

@jkotas could you please have a look at this one?

CC @eerhardt @vitek-karas

[jkotas]

What's the reason for omitting the apostrophes in the fallback string?

[jkotas]

Ah ok, this is the exception message. Makes sense.

[eerhardt]

It doesn't make sense to me why this change is necessary. Can someone help me understand?

[jkotas]

The format string from resources is: `Duplicate component name '{0}' ...``

The synthetized format string that you get with UseSystemResourceKeys is DuplicateComponentName {0}. Notice that the apostrophes are missing.

[eerhardt]

OK, that makes sense. But I guess I don't understand how this test is getting run with UseSystemResourceKeys=true.

[jkotas]

I assume that @tarekgh has done a one-off manual run with UseSystemResourceKeys=true. I do not think we have it automated.

[tarekgh]

That is right, I have run the test with and without enabling UseSystemResourceKeys manually and ensured it is passing in both ways. Enabling UseSystemResourceKeys in general should be done separately from this PR. I'll open issue to track that.

[tarekgh]

I have logged the issue #68707 to track enabling UseSystemResourceKeys on libraries tests.

[jkotas]

Thanks

[eerhardt]

Don't we need to run this test with UseSystemResourceKeys=true in order to test the bug?

[tarekgh]

I have manually run the test with and without UseSystemResourceKeys. As @jkotas indicated, such mode should be tested for all libraries and not just this one only. I'll open infrastructure issue to do that.

[eerhardt]

I'm not sure when we would enable a full test run with this setting. It doesn't seem like something that will happen anytime soon.

For now, we can run this one test with both UseSystemResourceKeys=true or false by doing something like the following code, only with the UseSystemResourceKeys switch:

runtime/src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestNotSupported.cs

Lines 18 to 30 in 66bd8b1

	/// <summary>
	/// Tests using EventSource when the System.Diagnostics.Tracing.EventSource.IsSupported
	/// feature switch is set to disable all EventSource operations.
	/// </summary>
	[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
	public void TestBasicOperations_IsSupported_False()
	{
	RemoteInvokeOptions options = new RemoteInvokeOptions();
	options.RuntimeConfigurationOptions.Add("System.Diagnostics.Tracing.EventSource.IsSupported", false);
	RemoteExecutor.Invoke(() =>
	{
	using (var log = new EventSourceTest())

[tarekgh]

ok, will do it.

[jkotas]

We should do a run of all libraries tests with UseSystemResourceKeys=true. I expect we will find more bugs like this one.

[tarekgh]

@jkotas @stephentoub @eerhardt I need your feedback on an issue I am seeing in the test run regarding the change here. If we have the following code and running it without my changes:

            var attribute = new TimersDescriptionAttribute(null!); // passing null for description
            string s = null;
            try {s = attribute.Description; } catch (Exception e) { Console.WriteLine($"{e.Message}"); } // accessing attribute.Description first time throw argument null exception
            s = attribute.Description; // access it second time will not throw :-(

This is happening because of the code

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/Timers/TimersDescriptionAttribute.cs

Line 40 in 8e624b3

if (!_replaced)

We are calling SR.Format(string) which will call internal static string Format(string resourceFormat, params object?[]? args) passing empty args array and then later will throw when trying to format the string.
With my changes, I removed the SR.Format as @eerhardt suggested and now we are not throwing it at all. Obviously, the current behavior is wrong. we have the following options:

• Throw in the attribute constructor when passing null parameter. This is the option I prefer.
• Don't throw at all and allow null description. This is the behavior with current changes.
• Keep the current behavior which throw first time trying to access the description and never throw again.

What you think?

[vitek-karas]

I just realized that this fix only fixes half of the problem.
It will fix the bug as written, that is without trimming the code will work. But with trimming on, it will still break. That is because of this:

runtime/eng/ILLink.Substitutions.Resources.template

Line 5 in 8e624b3

<resource name="{StringResourcesName}.resources" action="remove" />

This instructs the linker to remove the resource string - all of them. Linker currently doesn't have the logic to figure out which resource strings are used by analyzing the code (it's actually quite difficult), so instead we work around it by simply removing all of the resources.

But this will only work correctly for libraries which only have exception messages in the resources.

So I think the fix for this library (and any other which has non-exception resources) should be to simply disable the "optimization".
Basically follow #42274 as per Eric's suggestion.

[jkotas]

It will fix the bug as written, that is without trimming the code will work. But with trimming on, it will still break.

Why would it break? The library code does not load the strings from resources at all when you turn on UseSystemResourceKeys, so trimming the resources does not affect the behavior.

[jkotas]

Throw in the attribute constructor when passing null parameter. This is the option I prefer.

This is a breaking change for no strong reason. I think keeping this behavior as-is would be fine. Or if you want to do something about this, the fix should be about removing exceptions thrown that is a lot less breaking.

[vitek-karas]

Sorry @jkotas you're right. This change adds the default values for all of the resources it needs. I didn't notice that and thought we go to the resource.

[tarekgh]

This is a breaking change for no strong reason. I think keeping this behavior as-is would be fine. Or if you want to do something about this, the fix should be about removing exceptions thrown that is a lot less breaking.

With the current change is removing the exception throwing. but will have a risk of getting null description which can throw in other area in the app. If we are worried about the breaking, then we should keep the current behavior. I'll apply the fix to keep the current behavior then.