Update inappropriate ComVisible attributes

[weltkante]

Fixes #1878

Proposed changes

The changeset is broken down into separate commits with a rationale for each category:

• Remove ComVisible(true) from enums, they were used for TLB generation/lookup. Since there is no WinForms TLB provided by .NET Core there is no value in keeping this attribute.
• Remove ComVisible(true) from AccessibleObject classes. They are not CoCreateable (no parameterless constructor) and exposing an already existing instance to COM does not require that attribute.
• Remove ComVisible(true) from Control/Component classes. This was used to allow hosting of WinForms controls via OLE/ActiveX, for example in VB6 or MFC. However this requires a TLB for WinForms which is no longer provided, as well as registry-based activation which also would not work out of the box. Generally there was no maintenance of COM based hosting of WinForms controls so its likely to be broken, therefore removing support for COM based hosting of WinForms controls instead of leaving it in an unsupported state.
  • This commit also removes ClassInterface attributes from controls. If hosting via OLE/ActiveX is not supported these attributes are not needed anymore. They are kept in other places where objects are still exposed to COM and the attribute may be relevant.
• Remove ComVisible(true) from EventArgs. They were most likely used with OLE/ActiveX hosting, which is no longer supported. They are not CoCreateable either so the attribute has no purpose. Exposing existing instances without providing a TLB makes no sense either.
• Remove ComVisible(true) from delegates. Purpose is unknown but since ActiveX hosting of WinForms Controls is no longer supported its unlikely to have any purpose any more.
• Remove ComVisible(true) from some non-public code. The only potential consumer would be the new VS designer, but without a GUID specified its unlikely that its still needed. Can provide a GUID if required.
• Remove ComVisible(true) from some arbitrary public designer classes. The old VS designer may have been using COM interop to talk to them, but the old designer doesn't support .NET Core so there is most likely no one who needs these ComVisible.
• IWin32Window had a conflict with Desktop Framework which has dangerous consequences, see discussion in issue. Resolution can be either removing ComVisible(true) if not required ore generating a new GUID not conflicting with Desktop Framework.
• The WinForms managed IDataObject was made ComVisible. This is not required, there is a separate ComImport interface declaration for IDataObject COM interop. Having the managed IDataObject being ComVisible is actually counterproductive since no TLB is provided and marshaling will always fail. Also the GUID was not specified and differed from Desktop Framework so its unlikely that removing an undocumented IID will affect customers negatively.
• Remove ComVisible(false) - those are placed in seemingly arbitrary places and are redundant when the default is to not expose classes to COM interop (if the default is to expose, then current set of annotations would also be wrong because way too few classes have this annotation)

Customer Impact

May break customers which were doing unsupported COM interop against WinForms. It is unexpected that any exist, if they do it is desireable to gather feedback of what people are doing and then evaluate whether these scenarios are to be supported.

Regression?

partial

There was one interface which was sharing a GUID with Desktop Framework, IWin32Window, this is a conflict that could cause damage. (The regression part is that trying to register an application this way could damage the Desktop Framework installation, see discussion in linked issue.)

Risk

assumed low, may be breaking unknown usage patterns

Before

• redundant ComVisible attributes
• ComVisible(true) attributes without GUID, causing no harm but COM interop not being actively supported either
• conflicts in GUID with Desktop Framework

After

• redundant ComVisible removed
• unsupported ComVisible(true) removed, may be reconsidered in the future together with what is considered a supported scenario
• resolve all GUID conflicts by generating a separate GUID für .NET Core

Test methodology

• seaching for ComVisible in the codebase
• manual evaluation of scenarios (see above)
• no automated testing since most of the scenarios were not actively supported

Accessibility testing

Not performed. If removal of ComVisible on AccessibleObject requires testing to confirm general functionality is maintained this testing has yet to be performed.

Microsoft Reviewers: Open in CodeFlow

[weltkante]

Annotated the interesting cases, otherwise reviewing individual commits helps a lot because within a commit all changes are similar.

[weltkante]

This does not share the GUID with Desktop Framework so I really doubt anyone took a dependency on it. If we want we could specify the GUID and keep it ComVisible, but it won't work cross-process without a TLB, and COM based Clipboard/Drag'n'drop are mostly used cross-process, so I don't know what the point is. There is already a native interface shared via ComImport so I think this can be made purely managed.

[weltkante]

I have no idea why this was ComVisible, probably was a mistake, most functionality which requires ComVisible(true) skips private classes and the functionality that doesn't also works without having the attribute.

[RussKie]

👍

[RussKie]

This test is failing:

    System.Windows.Forms.Tests.WebBrowserTests.WebBrowser_ObjectForScripting_Set_GetReturnsExpected(value: CustomScriptingObject { }) [FAIL]
      System.ArgumentException : ObjectForScripting's class must be visible to COM.  Verify that the object is public, or consider adding the ComVisible attribute to your class. (Parameter 'value')
      Stack Trace:
        F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\WebBrowser.cs(513,0): at System.Windows.Forms.WebBrowser.set_ObjectForScripting(Object value)
        F:\workspace\_work\1\s\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\WebBrowserTests.cs(1271,0): at System.Windows.Forms.Tests.WebBrowserTests.WebBrowser_ObjectForScripting_Set_GetReturnsExpected(Object value)

[weltkante]

This test is failing:

Yup, noticed, the last commit yesterday was supposed to fix it, tests started passing locally after doing it, I'm not sure why the CI still fails, still trying if there is some way to repro it.

I'll rename the files like you suggested and see if the failure is consistent in the next CI build.

[weltkante]

I have no idea how the commit yesterday was succeeding locally, maybe I had a bad nightly build installed or something is still wrong with running tests inside VS. The ComVisible class has to be public in order to be recognized for browser scripting (contrary to the exception message which says "public or ComVisible")

I didn't rename the InternalsVisibleTo files yet since you wanted to investigate if its possible to autogenerate the attribute.

[RussKie]

Thank you

[RussKie]

Docs: dotnet/docs#22423