Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding AllowRejection to formalize rejection #178

Open
KirillOsenkov opened this issue Mar 4, 2020 · 5 comments
Open

Consider adding AllowRejection to formalize rejection #178

KirillOsenkov opened this issue Mar 4, 2020 · 5 comments

Comments

@KirillOsenkov
Copy link
Member

KirillOsenkov commented Mar 4, 2020

Specifying AllowDefault on an import allows us to still instantiate the part being composed if that import is not found.

It would be useful to express the current semantics of rejection if AllowDefault is missing, but without causing an error in the composition. This would formalize the implicit expectation that the part will get rejected if the import is missing.

The big problem with rejection is it can have unforeseen consequences as it's transitive. Also having errors in the composition is not great.

The Roslyn team has a usage scenario that goes like this:

We have [Export] components with required imports, knowing that in some scenarios the import will not be available and the linked export will silently also not be part of the catalog. We are using this to simulate a behavior where a MEF export in package A behaves as though it was provided by package B, where package A is always available but package B is optional. By implementing the export in package A with a required import from package B, the export becomes fully-conditioned on B.

Currently this scenario causes by-design errors in the composition. So hosts that rely on composition errors for diagnostics and investigation of real failures now get to either filter the by-design errors or manually filter the types that will get rejected in their composition.

I'm guessing one thing we can do is add

[Import(AllowRejection = true)]

or

[Import(RejectIfMissing = true)]

or

[Import(CausesRejectionIfMissing = true)]

and don't report an error in the composition if the part is rejected.

See some discussion here:
dotnet/roslyn#42121

@KirillOsenkov KirillOsenkov changed the title Consider adding AllowsRejection to formalize rejection Consider adding AllowRejection to formalize rejection Mar 4, 2020
@AArnott
Copy link
Member

AArnott commented Mar 4, 2020

VS-MEF doesn't define its own attributes, so we can't do any of your suggestions.

I'm also concerned that folks might expect a behavioral change by setting something like AllowRejection=true when in fact rejection is always the behavior when a MEF part's imports can't be satisfied.

We could perhaps define a new attribute called [RejectionExpected] or something like that, that might be placed on the [Import] member or perhaps on the MEF part itself. I believe the only effect you want this to have is to designate it differently in the MEF composition log.

We would still need to record in the log that the part was rejected, since folks who want to know why it's missing would still need to find the evidence, so all we're talking about (I think) is adding an annotation to that log entry stating that rejection of this part might be expected.

@KirillOsenkov
Copy link
Member Author

Yes I've realized that one would need a new attribute and would need a reference to Microsoft.VisualStudio.Composition.dll to reference this attribute.

[RejectionExpected] I like it. Yes, it'd be just to suppress the error in the composition.

@AArnott
Copy link
Member

AArnott commented Mar 5, 2020

We'll leave this active for now, but the extensibility team is booked through half of 16.7 already, so I don't see this happening any time soon. If you're willing to send the PR yourself, I can write a mini-spec for what I'd like to see and am willing to review it.

CC: @tinaschrepfer

@AArnott
Copy link
Member

AArnott commented Sep 15, 2022

I am curious about the use case of attributing the import vs. the rejected part. Do we need both?

An attributed part would say "If I'm rejected for any reason, it's probably ok."
An attributed import would say "If this import leads to rejection of the overall part, it's probably ok."

So attributing the import would allow for flagging cases where the part is rejected for an unexpected reason. It's more precise, for better or worse.

@AArnott
Copy link
Member

AArnott commented Sep 15, 2022

I just confirmed that VS-MEF exposes both part and import metadata in each rejection diagnostic found in CompositionConfiguration.CompositionErrors. So you could actually do this yourself with no VS-MEF changes.

Part metadata level:
[PartMetadata("Rejection", "Allowed")]

Import level:
[ImportMetadata("Rejection", "Allowed")]

Then when crawling the composition errors, you can look at the rejected parts, and for each one, look at its metadata:
error.Parts.foreach.Definition.Metadata
And you can look at each of its imports' metadata:
error.Parts.foreach.SatisfyingExports.foreach.Key.ImportDefinition.Metadata
Further, you can look at what each of the imports managed to match up with, thereby figuring out which imports led to rejection and make a determination as to why it was rejected, and whether the metadata makes it ok.

Yes, we could build an attribute and a new API in the CompositionErrors to do this for you. But you could do it yourself without any vs-mef changes.

AArnott added a commit to AArnott/vs-mef that referenced this issue Nov 7, 2022
Adopt .NET 7 SDK and NuGet central package versioning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants