StronglyTypedResourceBuilder output should be strongly typed #4588

rainersigwald · 2019-08-06T15:06:22Z

This regressed (#4582) with #4420, because the (untyped, .NET 1.0) Dictionary passed to Create never contained ResXDataNodes any more, but the new type LiveObjectResource that holds live deserialized objects in memory.

This fixes only the full-framework scenarios that use $(GenerateResourceUsePreserializedResources)=false. When building for .NET Core with the default settings, or using .NET Core to build, StronglyTypedResourceBuilder still doesn't work (#2272).

Description

Enable StronglyTypedResourceBuilder when running on .NET Core and ensure that projects built using the .NET Core codepath get code generated using the expected types for resources. Fixes #4588, fixes #2272.

Customer Impact

Projects that used ResXFileCodeGenerator could fail to compile due to generated code losing type information. This impacted WPF.

Projects affected by either problem would have to workaround by:

Build using desktop MSBuild.exe, not dotnet build
Explicitly disable the new .NET Core resource-handling codepath in the projects.

Regression

Regression from .NET Framework functionality; bugs in features new to crossplatform .NET Core MSBuild.

Risk

Low--impacts only the new resource codepaths.

src/Tasks/system.design/stronglytypedresourcebuilder.cs

rainersigwald · 2019-08-08T16:45:56Z

I just rewrote this including the things we discussed. Annoyingly, I think a downcast to LiveObjectResource is still required because that's the only case where we can meaningfully call GetType(), preserving the old semantics.

In this iteration, I enabled StronglyTypedResourceBuilder for .NET Core, because it was easy to do so and fills a longstanding gap.

With a private build of this, I'm able to build WPF successfully without any workarounds applied to the repo.

rainersigwald · 2019-08-08T16:54:28Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

@ericstj I'd love your input on this aspect in particular. Right now it results in generated code like

/// <summary> /// Looks up a localized resource of type System.String. /// </summary> internal static string String1 { get { object obj = ResourceManager.GetObject("String1", resourceCulture); return ((string)(obj)); } }

Instead of using GetString. I think I can sidestep this instance of the problem by making StringResource inherit from LiveObjectResource so it falls into that codepath, but I don't have a clear picture what to do about streams.

Is this different than what it generated before? Calling GetObject instead of GetString is a little concerning as GetString is a separate codepath. I haven't dug into the details to understand concretely if there's functional significance, but this difference is concerning enough that I bet its noticeable. /cc @tarekms

GetStream is less concerning as this just builds on top of GetObject I don't see this as a big problem.

Yes. In the old days and with my latest push 06ce897df, it generates:

/// <summary> /// Looks up a localized string similar to Value1. /// </summary> internal static string String1 { get { return ResourceManager.GetString("String1", resourceCulture); } }

If string is the more worrisome case, I think it's fixed.

So I took a closer look at GetStream and found that it does do something special.
https://github.com/dotnet/corefx/blob/46d3f57a9bba6c79ee494996d465e33fd8effa32/src/Common/src/CoreLib/System/Resources/ResourceManager.cs#L753-L760

It does just call GetObject but it doesn't wrap the underlying UnmanagedMemoryStream.

Contrast this to calling GetObject and casting, and it will wrap the UMS and prevent access to the backing pointer:
https://github.com/dotnet/corefx/blob/46d3f57a9bba6c79ee494996d465e33fd8effa32/src/Common/src/CoreLib/System/Resources/ResourceManager.cs#L703

This would break the scenario where someone is down-casting back to UMS to get the pointer to an embedded stream. So I think we do need to preserve calls to GetStream too if we can.

@ericstj Would you be comfortable with plumbing the assembly qualified type name down to here and doing matches on that?

IIUC I'd need to compare to either the full or core identities, right? so:

string: System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 System.String, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e MemoryStream: System.IO.MemoryStream, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 System.IO.MemoryStream, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e UMS: System.IO.UnmanagedMemoryStream, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 System.IO.UnmanagedMemoryStream, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e

And there won't be a way to do the chain-up-to-a-public-type dance at all.

danmoseley · 2019-08-08T17:50:50Z

src/Tasks/GenerateResource.cs

further down it's using StringComparer.InvariantCultureIgnore case. Should they use the same comparer, given both are resource ID's?

Yes, I think that makes sense, but I'm tempted to leave this alone since it's all preexisting.

We should probably use OrdinalIgnoreCase throughout.

rainersigwald · 2019-08-19T17:20:14Z

With my latest push, this is now entangled with and a superset of #4607. I needed some of the constants and functions from there here and vice versa. If you'd like to diff unique changes in this PR: rainersigwald/msbuild@fileref-special-cases...StronglyTypedResourceBuilder-ResXDataNode

eng/Packages.props

src/Tasks/Microsoft.Build.Tasks.csproj

This is what the field always represented, but that wasn't obvious.

Helps provide more guarantees about the value types extracted from these members. Also deleted an unused overload of Create.

This adds `TypeFullName` to `IResource` and consumes it when emitting code of specific types in StronglyTypedResourceBuilder. There are two special cases: * Unknown types (because the type is known only after deserializing a BinaryFormatter blob): just use `object`. This is less good than the old Full Framework implementation, which did the deserialize, got live objects, and then could look at types. * Live objects: get the actual type from the object, use that. This is how it always worked before on full framework, so it preserves behavior in that case.

rainersigwald · 2019-08-20T16:37:13Z

Accidentally closed due to miswording in another PR :(

ericstj · 2019-08-20T18:34:35Z

src/Tasks/GenerateResource.cs

This will catch a lot more than just ConfigurationException, most exceptions derive from SystemException. If anything I'd say just catch all exceptions and log an error and avoid the ifdef but that's cleanup.

Please please please do not log just Exception.Message and some generic text for anything but a specific exception. This just makes bugs harder to diagnose

Even when this is a catch around a single method call? Nick: for background CodeDom is throwing a private ConfigurationErrorsException https://github.com/dotnet/corefx/blob/2fac89f63c644ab81d936d62c91e6003302e718d/src/System.CodeDom/src/System/CodeDom/Compiler/CodeDomProvider.cs#L215-L222. I suspect @stephentoub did this to avoid the dependency on ConfigurationManger. Catching SystemException is nearly as bad as catching Exception, so your feedback should apply the same to the current PR. You could check the type name (hack) and rethrow, maybe file an issue for us to push ConfigurationErrorsException down and throw the public version.

yes, my feedback was to both the diff as is and the suggestion of changing to Exception.

It's better if it's a single method call, but then we'll end up with a bug in that method that is obscured. More hair falls out every time a bloody program tells me "object reference is not set to an instance of an object" like I did something wrong.

Can we do catch (Exception ex) when (ex.GetType().Name == "ConfigurationErrorsException")

Sure, and I'll file a corefx bug.

dotnet/corefx#40456 filed.

I suspect @stephentoub did this to avoid the dependency on ConfigurationManger.

Yes, System.Configuration didn't exist in corefx at the time. It wasn't brought in until several releases later.

src/Tasks/Microsoft.Build.Tasks.csproj

ericstj

Approve modulo the exception comments.

In .NET Core, CreateProvider throws a private exception derived from SystemException instead of one derived from ConfigurationException. This simplified catch removes duplication in the catch blocks.

This allows STRB to generate the right specialized accessor GetString() instead of returning (string)GetObject(...).

dsplaisted · 2019-08-20T19:17:38Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

                ValueAsString = valueAsString;
            }

+            internal ResourceData(string typeName, string typeAssemblyQualifiedName)


When reading the code that calls the constructors, it's confusing because the second argument means something entirely different depending on whether the first argument is a Type or just a string type name. I'm not sure there's a great way to make this clearer though.

dsplaisted · 2019-08-20T19:20:58Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

+                    type = typeof(UnmanagedMemoryStream);
+                }
+
+                // Ensure type is internalally visible.  This is necessary to ensure


Typo: internalally (also further down on comment)

Existing, but I'd love to clean it up for 16.4.

dsplaisted · 2019-08-20T19:27:26Z

src/Tasks/ResourceHandling/MSBuildResXReader.cs

+
+        internal static bool IsMemoryStream(string fileRefType)
        {
            return fileRefType.Equals(MemoryStreamTypeNameDesktopFramework, StringComparison.Ordinal);


Does this only work when building on the Desktop framework? Or should MemoryStreamTypeNameDesktopFramework be renamed?

This is very confusing. This codepath works on both core and desktop, but it only supports type names serialized into the resx that match the desktop names. Other tooling is responsible for ensuring that those names are used at resx-writing time. Some further discussion at #4607 (comment)

rainersigwald added this to the MSBuild 16.3 milestone Aug 6, 2019

rainersigwald force-pushed the StronglyTypedResourceBuilder-ResXDataNode branch from 579ce4b to 161660b Compare August 6, 2019 15:13

rainersigwald mentioned this pull request Aug 6, 2019

StronglyTypedResourceBuilder/ResXCodeFileGenerator output isn't strongly typed #4582

Closed

ericstj reviewed Aug 6, 2019

View reviewed changes

src/Tasks/system.design/stronglytypedresourcebuilder.cs Outdated Show resolved Hide resolved

rainersigwald force-pushed the StronglyTypedResourceBuilder-ResXDataNode branch 2 times, most recently from c4cef31 to 9cf2500 Compare August 8, 2019 16:40

rainersigwald commented Aug 8, 2019

View reviewed changes

rainersigwald mentioned this pull request Aug 8, 2019

dotnet-wpf-int builds fail due to ResXCodeFileGenerator regression in recent dev16.3p2 builds dotnet/wpf#1492

Closed

danmoseley reviewed Aug 8, 2019

View reviewed changes

vatsan-madhavan mentioned this pull request Aug 10, 2019

Undo workaround for ResXCodeFileGenerator regression dotnet/wpf#1588

Open

rainersigwald mentioned this pull request Aug 19, 2019

Generating strongly typed resource files not currently supported on .NET Core MSBuild #2272

Closed

This was referenced Aug 19, 2019

ResXFileRef special cases #4607

Merged

Permissive string types #4638

Merged

rainersigwald added Area: Task: GenerateResource Problems with the task itself, resgen.exe, and resx resources in general. ask-mode labels Aug 19, 2019

rainersigwald mentioned this pull request Aug 19, 2019

End-to-end tests for resx scenarios dotnet/sdk#3549

Open

livarcocc reviewed Aug 19, 2019

View reviewed changes

eng/Packages.props Outdated Show resolved Hide resolved

livarcocc reviewed Aug 19, 2019

View reviewed changes

src/Tasks/Microsoft.Build.Tasks.csproj Outdated Show resolved Hide resolved

rainersigwald added 4 commits August 19, 2019 21:07

Regression test for full framework instance of dotnet#4582

02f147c

Rename TypeName->AssemblyQualifiedTypeName

7a55c72

This is what the field always represented, but that wasn't obvious.

Use generic collections in ReaderInfo

50e3f90

Helps provide more guarantees about the value types extracted from these members. Also deleted an unused overload of Create.

rainersigwald force-pushed the StronglyTypedResourceBuilder-ResXDataNode branch from 76c1377 to 6b8d22f Compare August 20, 2019 02:16

nguerrera mentioned this pull request Aug 20, 2019

WriteCodeFragment fails in .NET Core MSBuild if input has braces #937

Closed

rainersigwald closed this in #4607 Aug 20, 2019

rainersigwald added the Approved to merge Approved by .NET Tactics for servicing or QB mode checkins label Aug 20, 2019

rainersigwald reopened this Aug 20, 2019

ericstj reviewed Aug 20, 2019

View reviewed changes

src/Tasks/Microsoft.Build.Tasks.csproj Outdated Show resolved Hide resolved

rainersigwald commented Aug 20, 2019

View reviewed changes

src/Tasks/Microsoft.Build.Tasks.csproj Outdated Show resolved Hide resolved

Turn on StronglyTypedResourceBuilder for Core

c109dc1

rainersigwald force-pushed the StronglyTypedResourceBuilder-ResXDataNode branch from 6b8d22f to cb8a512 Compare August 20, 2019 18:48

ericstj approved these changes Aug 20, 2019

View reviewed changes

rainersigwald added 6 commits August 20, 2019 14:09

Catch different exceptions around CodeDom.CreateProvider

1b007b6

In .NET Core, CreateProvider throws a private exception derived from SystemException instead of one derived from ConfigurationException. This simplified catch removes duplication in the catch blocks.

Make StringResource a LiveObjectResource

2043685

This allows STRB to generate the right specialized accessor GetString() instead of returning (string)GetObject(...).

Rename ResourceData.TypeName -> TypeFullName

bdc8b00

Plumb assembly-qualified name for resources through

648f173

Extract IsString

278ad41

Check string/stream type in Core codepath

5dfbd71

rainersigwald force-pushed the StronglyTypedResourceBuilder-ResXDataNode branch from c13516d to 5dfbd71 Compare August 20, 2019 19:09

dsplaisted approved these changes Aug 20, 2019

View reviewed changes

rainersigwald merged commit b28ad85 into dotnet:master Aug 20, 2019

rainersigwald deleted the StronglyTypedResourceBuilder-ResXDataNode branch August 20, 2019 20:14

StronglyTypedResourceBuilder output should be strongly typed #4588

StronglyTypedResourceBuilder output should be strongly typed #4588

Uh oh!

Conversation

rainersigwald commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Customer Impact

Regression

Risk

Uh oh!

Uh oh!

rainersigwald commented Aug 8, 2019

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!

danmoseley Aug 8, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Aug 19, 2019

Uh oh!

Uh oh!

Uh oh!

rainersigwald commented Aug 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Aug 20, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Aug 20, 2019 • 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!

Uh oh!

Uh oh!

ericstj left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rainersigwald commented Aug 6, 2019 •

edited

Loading

danmoseley Aug 8, 2019 •

edited

Loading

ericstj Aug 20, 2019 •

edited

Loading

stephentoub Aug 20, 2019 •

edited

Loading