search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.29k stars 4.74k forks source link

ComponentModel.DataAnnotations resources are probably not formatted right #46495

Open MichalStrehovsky opened 3 years ago

MichalStrehovsky commented 3 years ago

<GenerateResxSourceIncludeDefaultValues>true</GenerateResxSourceIncludeDefaultValues > in combinations with UseSystemResourceKeys produces messages like this:

Unhandled Exception: System.Exception: Exception of type '{0}' was thrown., System.Exception - we don't run String.Format in SR.Format when UseSystemResourceKeys is set because we don't expect that string to have formatting characters.

https://github.com/dotnet/runtime/pull/42274 is probably not the right fix without changes to the SR class.

Found this by accident when doing something in NativeAOT branch and GenerateResxSourceIncludeDefaultValues was causing me trouble.

Cc @eerhardt

ghost commented 3 years ago

Tagging subscribers to this area: @ajcvickers See info in area-owners.md if you want to be subscribed.

Issue Details
`true` in combinations with `UseSystemResourceKeys` produces messages like this: `Unhandled Exception: System.Exception: Exception of type '{0}' was thrown., System.Exception` - we don't run String.Format in `SR.Format` when `UseSystemResourceKeys` is set because we don't expect that string to have formatting characters. https://github.com/dotnet/runtime/pull/42274 is probably not the right fix without changes to the SR class. Found this by accident when doing something in NativeAOT branch and GenerateResxSourceIncludeDefaultValues was causing me trouble. Cc @eerhardt
Author: MichalStrehovsky
Assignees: -
Labels: `area-System.ComponentModel.DataAnnotations`
Milestone: -
eerhardt commented 3 years ago

tagging @joperezr

Thinking off the top of my head, we could possibly:

  1. Introduce a new internal "feature switch" to SR that is set when GenerateResxSourceIncludeDefaultValues is set on a library that tells SR.Format to ignore if UsingResourceKeys() is set and always call string.Format.
  2. We could change SR.Format when UsingResourceKeys() is set to look for literally {0, and use string.Format if it sees an argument placeholder.
  3. To fix this specific case, we could change all usages of SR.Format in ComponentModel.DataAnnotations to call string.Format directly.
MichalStrehovsky commented 3 years ago

We should ideally avoid bringing an unconditional reference to string.Format - besides the formatting implementation itself, it brings in implementations of IFormattable on everything. It's not very trim friendly. UsingResourceKeys should ideally be as low overhead as possible.

I would lean towards 3 - the reason why we have SR.Format in the first place is to make things work when UsingResourceKeys, and here we explicitly don't want that behavior.

It would be great if we could add some sort of poison that would cause a build break if someone "fixes" this back to SR.Format. Maybe we could put SR.Format implementation in the common directory under an #ifndef NO_SR_FORMAT and define that symbol for the DataAnnotations assembly.