Treat IPeekResultFactory as optional import

[KirillOsenkov]

It is not available on Visual Studio for Mac so we want to gracefully degrade when it's not around.

[CyrusNajmabadi]

Lgtm

[CyrusNajmabadi]

I am logged in as me... But I'm missing the review button...

[sharwell]

⌚️

[sharwell]

The required imports for this type are used to prevent PeekableItemSourceProvider from becoming part of the MEF catalog when dependencies are not available. The MEF catalog already gracefully degrades for this case.

[sharwell]

📝 This is a required export. The implementation is defined and exported in the same assembly where the interface is defined.

[sharwell]

📝 This is intentionally not optional. By design, PeekableItemSourceProvider will be silently omitted from the catalog when IPeekResultFactory is not available. The type does not function correctly without it.

[KirillOsenkov]

I'm making the type work properly without it below, by just returning null if the part is null.

[sharwell]

@KirillOsenkov Note that we depend heavily on graceful degradation of the MEF catalog when imports are omitted. Much of the restricted IVT layers are implemented on this basis, so it will not be possible to compose Roslyn using a "strict" mechanism that expects all required imports to be satisfied.

[jasonmalinowski]

If we're going to allow nulls in like this, we should #nullable enable the file or else this is is laying a pretty awful trap.

Also: this seems like an ideal place where MEF part rejection is useful -- why aren't we doing that?

[KirillOsenkov]

VSMac relies heavily on inspecting MEF composition errors to find actual issues. This is a very valuable tool for us as we live in a world where we insert a patchwork of dependencies and we need to make sure they all work together well. So far we have been successful in maintaining error-free composition and this is the only issue.

Silent rejection terrifies me as large components of the graph can disappear and then we spend time investigating why a feature is missing or not behaving as intended. Those scare me way more than a hypothetical null-ref in a tiny class that will probably never change.

I'd like to continue to disable silent rejection in the VSMac MEF composition and I humbly ask the Roslyn team to allow this one little issue. AllowDefault is a pattern widely used already and I prefer defending against optional imports being null to investigating parts inexplicably missing from composition.

[KirillOsenkov]

@sharwell and I have been discussing this offline quite extensively and I want to make sure I get more input from other involved team members. @dotnet/roslyn-ide

I have to confess that the design approach of using part rejection to express exports that are only present when another component is present is new to me. I have heard of rejection since 2009 original MEF but it was always considered a huge source of unforeseen errors as rejection tends to cascade and large portions of the graph can just disappear without a trace and good luck debugging why is my type not loading.

This is why the original MEF has introduced the DisableSilentRejection flag and we've been trained to set it always to have explicit errors when parts were not loaded because of missing required imports. In my experience we have spent a lot of time investigating issues where a part was not loaded and turning errors on showed us why.

It looks like the Roslyn team has made a decision to deliberately introduce errors into the VSMEF composition by design as part of expressing "don't export this part unless someone will be actually importing this subsystem". I understand that the VS MEF composition may already contain lots of errors ("broken windows") but to my knowledge this is the first time someone is making more errors in the composition and its intentional.

I'm curious what is the proposal to debug real MEF failures when a type isn't loaded because an import is missing unintentially due to a real bug. I'm also curious if there could be other MEF patterns that would let you express this without introducing errors into the composition.

Additionally I still don't quite understand why this PR is a problem and what real issues will we have if we merge it in its current form. To me AllowDefault imports are an established MEF pattern widely used in Visual Studio and it clearly expresses that an import is optional in some hosts.

@sandyarmstrong @CyrusNajmabadi @tmat @jinujoseph

[KirillOsenkov]

Also @olegtk

[CyrusNajmabadi]

I have to confess that the design approach of using part rejection to express exports that are only present when another component is present is new to me. I have heard of rejection since 2009 original MEF but it was always considered a huge source of unforeseen errors as rejection tends to cascade and large portions of the graph can just disappear without a trace and good luck debugging why is my type not loading.

I strongly resonate with that. The number of times i've had mef composition failures without being able to figure out what was going on is in the triple digits. I remember losing hair over how awful this was.

If the situation still sucks, and part rejection is still an opaque and painful thing to figure out, i would opt toward avoiding rejection semantics. If, however, part rejection is very easy to understand now, i could feel differently.

[KirillOsenkov]

I filed a design suggestion on vs-mef to formalize the rejection semantics:
microsoft/vs-mef#178

This way you have to explicitly opt-in to rejection in a place where you expect it. Unforeseen rejections caused by actual bugs will still cause errors in the composition.

[sharwell]

@jasonmalinowski and I have proposed that VS for Mac exclude the type from the catalog prior to finalizing the composition. This allows Roslyn to continue operating under the current design, and allows VS for Mac to continue disabling silent rejection by excluding one or more specific types to avoid composition errors. Here is an example of a situation where parts are excluded from a MEF catalog:

roslyn/src/Workspaces/CoreTestUtilities/MEF/ExportProviderCache.cs

Lines 137 to 144 in 9bca5a2

	/// <summary>
	/// Creates a <see cref="ComposableCatalog"/> derived from <paramref name="catalog"/>, but with all exported
	/// parts assignable to type <paramref name="t"/> removed from the catalog.
	/// </summary>
	public static ComposableCatalog WithoutPartsOfType(this ComposableCatalog catalog, Type t)
	{
	return catalog.WithoutPartsOfTypes(SpecializedCollections.SingletonEnumerable(t));
	}

[sharwell]

If the situation still sucks, and part rejection is still an opaque and painful thing to figure out

Part rejection is easy to figure out now. You can get a feel for it by stepping through this block:

roslyn/src/Workspaces/CoreTestUtilities/MEF/ExportProviderCache.cs

Lines 211 to 238 in 9bca5a2

	foreach (var errorCollection in _configuration.CompositionErrors)
	{
	foreach (var error in errorCollection)
	{
	foreach (var part in error.Parts)
	{
	foreach (var (importBinding, exportBindings) in part.SatisfyingExports)
	{
	if (exportBindings.Count <= 1)
	{
	// Ignore composition errors for missing parts
	continue;
	}
	if (importBinding.ImportDefinition.Cardinality != ImportCardinality.ZeroOrMore)
	{
	// This failure occurs when a binding fails because multiple exports were
	// provided but only a single one (at most) is expected. This typically occurs
	// when a test ExportProvider is created with a mock implementation without
	// first removing a value provided by default.
	throw new InvalidOperationException(
	"Failed to construct the MEF catalog for testing. Multiple exports were found for a part for which only one export is expected:" + Environment.NewLine
	+ error.Message);
	}
	}
	}
	}
	}

[olegtk]

@AArnott

[jasonmalinowski]

Yeah I absolutely agree that part rejection can be a royal pain when you're trying to debug something, but at the same time it's a tool that exists and is often useful. In this particular PR we're fairly lucky in the API can easily be implemented to no-op if a dependent part exists but that solution isn't always usable. Sometimes trying to deal with a missing export is complicated, and in some sense AllowDefault might mean your composition is free from errors, but isn't in a remotely tested state.

It's also a bit of a layering violation if we're having to make changes to work around the lack of support for something for one host. We can do code to mutate VS for Mac's composition to say things like "we want to throw out parts that consume this known missing part", and it seems like that instead moves the declaration of "what might be missing" to the host that knows what it doesn't support. When the support comes online, then you can update the code in VS for Mac and not have to go to other repos to remove the workarounds.

👍 for trying to make this a bit more declarative so we can have both sides: part rejection when it's useful without losing diagnosability.

[AArnott]

Several points to add to the discussion:

1. Rejection in MEF is a first class feature and it's fine to depend on it.
2. Rejection is a multi-pass process during composition, and each time we compose and have to reject a part, it requires restarting the composition, which can take seconds on some machines. So adding rejection to the common case may slow down VS setup or cache-invalidated VS launches by several seconds.
3. The diagnostic log is indeed vital to many people's investigation, so adding 'by design' rejection will certainly add noise and complicate the investigation.
4. CPS used to depend heavily on rejection but when we switched from .NET MEF to VS MEF we entirely switched away from that. Our goal was different from yours though, but effectively we do just deal with conditional imports. In our case we have n exports so all of these conditional imports are collections.
5. AllowDefault=true is another first class feature of MEF and is usually a better choice when the export may not be present and you mean to support that scenario, as it doesn't slow down composition or generate errors in the log. But Rejection uniquely can take out entire transitive chains of exports if the leaf export is missing.

In summary

If AllowDefault=true is sufficient for you and all you have to do is a null check before using it, I would recommend you take that route.

[KirillOsenkov]

FYI @Therzok we may need to start filtering individual types from the composition.

[tmat]

@sharwell @jasonmalinowski Given @AArnott's recommendations, what's the verdict for Roslyn?
Applies to #42144 also.

[sharwell]

My recommendation is to continue with part rejection. It's the only way to keep IVT isolation and still continue to behave as though the partner team provided the original part.

Or in other words, we have a slightly more expensive setup operation (~one time) in exchange for a more accurate and efficient runtime configuration.

[KirillOsenkov]

I'm OK with this. Feel free to close the PR.