Add InternalsVisibleTo to the VSMac Razor assembly.

[KirillOsenkov]

@NTaylorMullen

[JoeRobich]

LGTM

[KirillOsenkov]

@jinujoseph would you please help with landing this? Thanks!

[sandyarmstrong]

@jinujoseph would it be possible to get this for 16.4?

[jasonmalinowski]

@sandyarmstrong It's a simple change from the tech side, but that may require filling out a QB template internally to get approval. (not sure if we cover this under that rule or not). @jinujoseph?

[NTaylorMullen]

I'm not convinced this warrants a QB insertion/exception. I've worked around the limitation on the Razor side and even if the change were to make it into 16.4 we wouldn't be able to consume it on the Razor side because of the Roslyn binary mismatches in VSwin/VSmac.

@sandyarmstrong was there something in particular you were concerned about?

[KirillOsenkov]

We need IVTs regardless because I don't want to be cornered into committing design crimes again like we did last week.

[sandyarmstrong]

There are three other projects missing this change:

• src/VisualStudio/Core/Def/Microsoft.VisualStudio.LanguageServices.csproj
• src/VisualStudio/Razor/Microsoft.VisualStudio.LanguageServices.Razor.RemoteClient.csproj
• src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj

[jasonmalinowski]

@sandyarmstrong What are those needed for? IVT policy for those is very different and would result in this PR being held up likely.

[KirillOsenkov]

We've realized that IVTs are now specified in more than one place. It used to be that we could go to a single file and it managed all the IVTs conveniently. Now we found the places Sandy found, and while we don't need MS.VS.LanguageServices and Razor.RemoteClient, we do need Microsoft.CodeAnalysis.Workspaces.dll, for IDynamicFileInfoProvider which is internal.

[jasonmalinowski]

@KirillOsenkov Are those new ones you added being built with the MonoDevelop key, or are new Razor-build binaries?

[KirillOsenkov]

@jasonmalinowski these are all in the Razor codebase and built with the Razor key. The Razor team owns the VSMac experience ;)

[jasonmalinowski]

Is there a reason the existing assemblies that already have IVT aren't being used? I would have imagined that code was all shared between both platforms.

My line of questioning here mostly is us adding new IVTs is something we aren't doing freely anymore; if there are different projects perhaps because they're coming from different builds, then it means we need to at least somewhat understand what is going on around us.

[KirillOsenkov]

Unfortunately yes, we have a reason. The Windows assemblies use CPS for the project system functionality, and the implementation of IDynamicFileInfoProvider on the Windows side is all in the Roslyn VS layer (Microsoft.VisualStudio.LanguageServices.dll) that VSMac can't use.

Believe me, we've committed quite a few design crimes to work around the IVTs and what we came up with is unfortunately the most realistic tactical solution. We placed a base class into the assembly that can see Roslyn's internals, and derived from it in the assembly that doesn't see Roslyn internals. Basically our design was guided by where we have IVTs and where we don't. I personally think this is not a nice world to live in.

As an aside I'm starting to plan a campaign to implement Roslyn support for IgnoresAccessChecksTo, Eli has even submitted a PR: https://github.com/aelij/IgnoresAccessChecksToGenerator. There's just so much unnecessary friction in the ecosystem due to the IVTs that frankly we could all save quite a lot of time and resources if we had IACT.

[sharwell]

The IVTs fall into four categories for VS for Mac:

1. IVT from Microsoft.CodeAnalysis.ExternalAccess.Razor to partner code: These are allowed
2. Restricted IVT to partner code: These are allowed
3. IVT from another assembly to partner code that never loads in Visual Studio on Windows: These are allowed under the MonoDevelop exclusion, but without warranty. If we change this internal code and the partner assembly breaks (source or binary breaking change), the partner bears full responsibility for updating to account for the change since they opted out of the compatibility policy
4. IVT from another assembly to partner code that may load in Visual Studio on Windows, either in box or as part of a Microsoft-supported extension: These are not allowed (restricted IVTs must be used instead)

[KirillOsenkov]

OK we missed 16.4, can we merge this into master please?

[sharwell]

@KirillOsenkov Can you clarify which of the four categories the Workspaces IVT falls into?

[KirillOsenkov]

@sharwell #3, MonoDevelop exclusion please

[sharwell]

Thanks, can you move it into the exclusions block and mark it as not loading in Visual Studio?

roslyn/src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj

Lines 278 to 293 in 10fab37

	<!-- BEGIN MONODEVELOP
	These MonoDevelop dependencies don't ship with Visual Studio, so can't break our
	binary insertions and are exempted from the ExternalAccess adapter assembly policies.
	-->
	<InternalsVisibleTo Include="MonoDevelop.Ide" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.Refactoring" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.CSharpBinding" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.VBNetBinding" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.Ide.Tests" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.Refactoring.Tests" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.CSharpBinding.Tests" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="MonoDevelop.VBNetBinding.Tests" Key="$(MonoDevelopKey)" LoadsWithinVisualStudio="false" />
	<!-- this is currently built externally, as part of the MonoDevelop build -->
	<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures.Cocoa" LoadsWithinVisualStudio="false" />
	<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures.Cocoa.UnitTests" LoadsWithinVisualStudio="false" />
	<!-- END MONODEVELOP -->

[sharwell]

If it doesn't directly apply to MonoDevelop, you could create a new block with a similar clarifying comment following the example from MonoDevelop.