Scrub [ComVisible] attribution from CoreFX library sources #782
Conversation
| @@ -26,5 +26,4 @@ | |||
| [assembly: NeutralResourcesLanguageAttribute("en-US")] | |||
|
|
|||
| [assembly: CLSCompliant(true)] | |||
| [assembly: ComVisible(false)] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks, Wes. Yes, I'll hold off on merging until the mirror is enabled. Sounds like we shouldn't need to add anything, but good to know we'll be able to do it in a single place if necessary.
There was a problem hiding this comment.
I've updated the PR after rebasing from #772.
I think it should be fine to strip the ComVisible(false) assembly attributes in corefx. ComVisible(false) disables the types in the assembly from being exposed as COM interfaces - e.g. it prevents somebody from running the tlbexp tool on the assembly and then use the types from C++ via COM. ComVisible(false) assembly attribute is important for desktop in-box assemblies where we go out of our way to avoid compatibility issues by explicitly disallowing fragile features that apps may take (intentional or accidental) dependency on. I do not think this concern applies for corefx. cc @yizhang82 Please speak up if you disagree. |
|
Thanks, Jan. |
|
I consider adding ComVisible(false) to every single assembly as good practice, regardless what platform it is. This would make sure we don't accidentally expose them as COM objects both statically and at runtime, and also avoid generating unnecessary COM interop marshalling code statically (such as in .NET native). Putting ComVisible(false) in a AssemblyInfo.cs that gets automatically included to every project sounds very reasonable if this is easy to do. |
|
It used to be a good practice for desktop libraries. The default template for desktop class libraries has the assembly ComVisible(false) attribute. However, none of the modern VS templates (e.g. portable libraries, Windows Store, ...) have the assembly ComVisible(false) attribute. I assume that it was removed as unnecessary cruft. It means that majority of the modern 3rd party code does not have the assemblu ComVisible(false), and so you cannot really depend on it to solve problems like avoid generating unnecessary COM interop marshalling code... |
|
Even if that weren't true, there's also the new world of Linux, Mac and other OSes to consider. Imagine that the entrance of .NET Core off of Windows truly explodes (that'd be awesome, right!). I don't think we can impose a Windows-ism (with either polarity of setting), particularly for a relatively soft reason. |
weshaggard
added
the
enhancement
stephentoub
force-pushed
the
remove_comvisible
branch
from
24c2c78
to
9189f0a
Compare
|
This has no impact on the contract. We already strip ComVisible from them. |
|
@jkotas The reason that Store/PLIB templates don't have ComVisible(false) is because the ComVisibleAttribute didn't exist in the surface area at the time those things were originally written. |
|
I've opened dotnet/buildtools#77 to track automatically adding [ComVisible(false)] to each assembly if we decide it's the right thing to do. In the meantime, I'll go ahead with this scrub. |
Scrub [ComVisible] attribution from CoreFX library sources
In dotnet#782 we scrubbed all of the [ComVisible] attributes from corefx, but more have snuck in since then. Doing another pass.
In dotnet#782 we scrubbed all of the [ComVisible] attributes from corefx, but more have snuck in since then. Doing another pass.
In dotnet/corefx#782 we scrubbed all of the [ComVisible] attributes from corefx, but more have snuck in since then. Doing another pass. Commit migrated from dotnet/corefx@db5a5ef
Per #779, this PR scrubs [ComVisible] attribute from the CoreFX library sources.
@jkotas, one thing I'm unsure of: since [ComVisible(true)] is the default for desktop, and since some of these libraries (e.g. immutable collections, dataflow, etc.) can be used with the desktop, should we actually be applying [ComVisible(false)] to every assembly? Some of the assemblies already had this, and with the current commit I've removed those, but if they're needed I can put them back and make sure the other libraries have such attribution, too.