Fix the TypeConverter when enabling UseSystemResourceKeys by tarekgh · Pull Request #68687 · dotnet/runtime

tarekgh · 2022-04-28T23:46:37Z

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 · 2022-04-28T23:46:45Z

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 · 2022-04-28T23:50:21Z

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

CC @eerhardt @vitek-karas

jkotas · 2022-04-29T00:43:29Z

src/libraries/System.ComponentModel.TypeConverter/tests/ContainerTests.cs

            Assert.Null(ex.InnerException);
            Assert.NotNull(ex.Message);
-            Assert.True(ex.Message.IndexOf("'dup'") != -1);
+            Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);


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

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

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

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.

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

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

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.

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

jkotas

Thanks

...ibraries/System.ComponentModel.TypeConverter/src/System/Timers/TimersDescriptionAttribute.cs

eerhardt · 2022-04-29T14:16:23Z

src/libraries/System.ComponentModel.TypeConverter/tests/CultureInfoConverterTests.cs

+        {
+            using (new ThreadCultureChange(null, CultureInfo.InvariantCulture))
+            {
+                Assert.Equal("", ((CultureInfo)TypeDescriptor.GetConverter(typeof(System.Globalization.CultureInfo)).ConvertFrom(null, null, "(Default)")).Name);


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

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.

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())

ok, will do it.

jkotas · 2022-04-29T14:37:15Z

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

tarekgh · 2022-04-29T23:20:42Z

@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 · 2022-04-29T23:26:51Z

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

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 · 2022-04-29T23:46:36Z

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 · 2022-04-29T23:51:12Z

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 · 2022-04-30T00:02:35Z

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 · 2022-04-30T00:13:14Z

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.

…emResourceKeys on

Fix TypeConverter when enabling UseSystemResourceKeys

4dcb885

ghost added the area-System.ComponentModel label Apr 28, 2022

ghost assigned tarekgh Apr 28, 2022

tarekgh added this to the 7.0.0 milestone Apr 28, 2022

jkotas reviewed Apr 29, 2022

View reviewed changes

jkotas approved these changes Apr 29, 2022

View reviewed changes

eerhardt reviewed Apr 29, 2022

View reviewed changes

...ibraries/System.ComponentModel.TypeConverter/src/System/Timers/TimersDescriptionAttribute.cs Show resolved Hide resolved

eerhardt reviewed Apr 29, 2022

View reviewed changes

runfoapp bot mentioned this pull request Apr 29, 2022

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

Address the feedback

48e84a3

stephentoub approved these changes Apr 29, 2022

View reviewed changes

Keep original behavior and enable running the added test with UseSyst…

d977a03

…emResourceKeys on

tarekgh merged commit 5c2a738 into dotnet:main Apr 30, 2022

ghost locked as resolved and limited conversation to collaborators May 30, 2022

	/// <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())

Conversation

tarekgh commented Apr 28, 2022

Uh oh!

ghost commented Apr 28, 2022

Uh oh!

tarekgh commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Apr 29, 2022

Uh oh!

tarekgh commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitek-karas commented Apr 29, 2022

Uh oh!

jkotas commented Apr 29, 2022

Uh oh!

jkotas commented Apr 29, 2022

Uh oh!

vitek-karas commented Apr 30, 2022

Uh oh!

tarekgh commented Apr 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tarekgh Apr 29, 2022 •

edited

Loading

tarekgh commented Apr 29, 2022 •

edited

Loading