Winforms: UITypeEditor does not type-forward from System.Drawing

[weltkante]

• .NET Core Version: 5.0 master
• Have you experienced this same bug with .NET Framework?: No

Problem description:
For some reason UITypeEditor does not automatically type-forward from System.Drawing.Design to System.Windows.Forms.Design even though it is a public type. I was under the impression that public types get their type forwards automatically generated and manual entries only are required for internal types referenced by name.

This breaks ControlBindingsCollection which has an EditorAttribute referencing UITypeEditor itself. Technically this attribute is pretty much a no-op, its only effect is that a non-null editor is returned from TypeDescriptor.GetEditor but the editor itself has all methods no-op.

Either case I think the type forward for UITypeEditor should be put in place anyways, not just for attributes referencing by string but for general compatibility, since its a public type.

Actual behavior:
UITypeEditor does not type forward even though its a public type

Expected behavior:
UITypeEditor should type forward from System.Drawing.Design facade to System.Windows.Forms.Design. The type forward should be generated automatically and not have to be manually inserted in the project.

Minimal repro:
Fails test in PR #2230 when removing skip entry

[weltkante]

I think UITypeEditor originally comes from System.Drawing.dll, not from System.Drawing.Design.dll, that would explain why the type forward is not generated.

I think this needs to be coordinated with CoreFX, as they own System.Drawing now? Or does WinForms have a facade for System.Drawing.dll which takes priority over CoreFX? I remember the situation to be a bit complex. (If there is a facade then the question would again be, why doesn't it generate the forwarding automatically.)

Otherwise we can just remove the attribute from ControlBindingsCollection (or update the attribute to point at the new location of UITypeEditor), this editor is really a no-op and should have never been put into the attribute in the first place - unless having a no-op editor instead of null editor has some side effects I'm not aware of.

[RussKie]

I think the underlying issue that you have already discovered is that the ControlBindingsCollection decoration is incorrect:
https://github.com/dotnet/winforms/blob/b666dc7a94d8ac87a7d300cfb4fa86332fb79bae/src/System.Windows.Forms/src/System/Windows/Forms/ControlBindingsCollection.cs#L16-L19

In .NET Framework UITypeEditor was declared in System.Drawing.dll. However in .NET Core it was moved to System.Windows.Forms.dll, so the original decoration is incorrect.
@Tanya-Solyanik @dreddy-work @DustinCampbell should it be this?

     [DefaultEvent(nameof(CollectionChanged))]
-    [Editor("System.Drawing.Design.UITypeEditor, " + AssemblyRef.SystemDrawing, typeof(UITypeEditor))]
+    [Editor(typeof(UITypeEditor), typeof(UITypeEditor))]
     [TypeConverter("System.Windows.Forms.Design.ControlBindingsConverter, " + AssemblyRef.SystemDesign)]
     public class ControlBindingsCollection : BindingsCollection

Or can we just remove this line altogether?

[dreddy-work]

@RussKie , We were generating type-forwards in facad assembly for all public classes/methods that were moved around. In this case, I see system.windows.forms is added as reference for System.Drawing assembly. Was it not generating type-forward there?

And system.Drawing.dll overrides corefx one.

[Tanya-Solyanik]

Or can we just remove this line altogether?

Designer relies on this hard-coded text so far.

[weltkante]

@dreddy-work I think this redirect was either missing or broken, as tests showed this type didn't resolve correctly. Fix could be either adding a redirect, correcting the attribute, or removing it alltogether

@Tanya-Solyanik it does? the editor which is specified here is a no-op because it doesn't implement a subclass (none of the methods on the base class do anything), so unless the designer makes a difference between a null editor and an editor which does nothing, it shouldn't matter

[dreddy-work]

@weltkante , I see type forward in "System.Drawing.dll" . Were the tests failing on the latest runtime? I know we removed editors binary completely and merged it with winforms and design assemblies sometime back. But i see type forwards are working as expected here. Can you point to the exact test that is failing, to look further?

[weltkante]

Last line in issue description refers to this skip entry - if you remove it and the test no longer fails then it was solved by some other PR since the tests had been checked in

[weltkante]

Your redirects are from System.Drawing 5.0 but the string references look for 4.0 so I'm pretty sure all these references you are forwarding there will be failing to resolve when referenced via strings in attributes

@RussKie why was System.Drawing.dll leveled to 5.0 when the Desktop references it should be redirecting are looking for 4.0 ? Is .NET Core supposed to do some automatic forward rolling of versions when looking for redirects? (it clearly doesn't as the test is still failing)

Adjusting the string reference in WinForms will probably not be enough, leveling System.Drawing.dll to 5.0 will probably still break string-based designer attributes in consumer assemblies.

Assuming this isn't just a bug in the type resolution logic, if you still want to do it you'll need to treat it as a major breaking change and notify people in the release notes of 5.0 that designer attributes need to be updated.

If you don't just want to keep System.Drawing.dll at 4.0 forever I think its best to pull in someone from the runtime team to ask how the transition to 5.0 should happen in respect to type forwards existing for Desktop compatibility.

[RussKie]

@ericstj do you think you could give us a guidance here?

[ericstj]

For desktop compat shims don’t change the assembly version. Don’t remove types, maintain APICompat with previous release. APICompat should catch problems where you remove type forwards.

.NETCore does allow newer assembly versions to automatically satisfy older references, but there is no good reason to version the compat shims since they have no types.

[weltkante]

Thanks, I only tested this inside WinForms repo, so I didn't notice it does work outside this repo.

.NETCore does allow newer assembly versions to automatically satisfy older references

In the WinForms repository (which is hardcoded to use 5.0.0-alpha1.19514.1) this doesn't work. I checked both unit tests and the WinformsControlsTest standalone application, both fail to resolve the redirect.

Creating an independent application with 5.0.0-preview.2.20160.6 works however.

Is this a .NET Core version issue (needing an update in the repo) or is there some switch that the WinForms repo uses, turning off this roll-forward feature?

For desktop compat shims don’t change the assembly version.

There are type forwards in non-shim assemblies so the version forwarding definitely needs to work. Otherwise running 3.x on 5.0 will cause compatibility issues.

[weltkante]

Disregard the version quoted above, I was on a PR branch, testing on master uses 5.0.0-preview.4.20216.4 and still doesn't work. Must be some global switch in the WinForms repo which prevents rolling forward on type redirects?

I'm testing Type.GetType("System.Drawing.Design.UITypeEditor, System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", false) and it returns null when called within the WinForms repo but resolves properly in a standalone project.

Considering the redirects are actually in place and working, the only thing remaining for this issue would be to make the unit tests have the same roll-forward behavior as a standalone project outside the repo.

(Also the version could be rolled back on shim assemblies as suggested above, but it works without doing that, the unit tests just don't.)

[ericstj]

I'm testing Type.GetType("System.Drawing.Design.UITypeEditor, System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", false) and it returns null when called within the WinForms repo but resolves properly in a standalone project.

You should probably see what version of System.Drawing is being loaded. I bet you're loading the one from the base shared framework rather than the one you're building in the winforms repo. This is probably happening because your test isn't running against a WindowsDesktop shared framework and hasn't referenced the System.Drawing assembly you build in the winforms repo. Does your test actually reference winforms's System.Drawing

[weltkante]

Yup, thats it, it picks up the wrong System.Drawing.dll - and you're also right that the reference is missing from the projects.

Just adding the reference like done for the other facades isn't enough though, it gets a System.Drawing.dll with the right redirects copied into the output folder but it has version 42.42.42.42 - while the shared framework has 4.0.0.0 and gets loaded anyways, I assume because the loader finds an exact match and never tries the System.Drawing.dll from the output folder?

So it will obviously be necessary to revert the 5.0 version on System.Drawing (WinForms should use the same version as the shared framework it is replacing), but I have no idea how to get unit tests working when local builds use a 42.42.42.42 placeholder version. I'll leave that for someone to figure out who understands the WinForms build infrastructure.

[ericstj]

but it has version 42.42.42.42 - while the shared framework has 4.0.0.0 and gets loaded anyways, I assume because the loader finds an exact match and never tries the System.Drawing.dll from the output folder?

No, that shouldn't happen. The host should decide one version to use. Can you share the deps file for the test?

[weltkante]

Sure, it says "System.Drawing": "5.0.0-dev" (which is what I'd expect when referencing the facade) but thats only present in the AssemblyInformationalVersion not in the real assembly version of System.Drawing.dll copied into the output folder. At runtime it loads the shared framework one in 4.0.0.0 either way.

WinformsControlsTest.deps.txt

[ericstj]

Deps file doesn’t list version info:

        "runtime": {
          "System.Drawing.dll": {}
        }

Normally file and assembly version info should be included to allow the host to choose which version should win. Your build process isn’t including that.

[weltkante]

@RussKie for the record, I've been adding <ProjectReference Include="..\..\..\..\System.Drawing\src\System.Drawing.Facade.csproj" /> to the relevant test projects WinformsControlsTest and System.Windows.Forms.Tests. They were already referencing other facades and I just added the missing one. Those other facades would have the same problem, they also don't specify a version in the deps.

Regarding how to fix the build process to include the required data you'll have to redirect the issue to someone who is familiar with it. I don't think it has high priority though.

[ericstj]

https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/GenerateDepsFile.cs
@wli3 @dsplaisted

[ericstj]

Might be this https://github.com/dotnet/sdk/issues/3254

[weltkante]

yup, looks like WPF has the exact same problem with WindowsBase, which is also in the shared framework like System.Drawing

[weltkante]

Looks like you can work around by adding both a ProjectReference a direct Reference to the assembly file. I'll try to figure out what the right msbuild variables are and make a PR.

[weltkante]

The PR does not include reverting the 5.0 versions back to 4.0, unit tests are running without doing that (mostly because I don't know how to override the version for a project, it seems to be set globally). If desired the versioning issue for facades can be moved to a separate issue so someone else can look at it who knows more about the WinForms build infrastructure.

I still think it should be done since currently the console SDK has a System.Drawing 4.0 and winforms SDK has a System.Drawing 5.0, it would be better if different parts of the SDK agree about which version to use for facades.