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.

Sign up for GitHub

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

Jump to bottom

Add support for internal metadata view interfaces #10

Merged
merged 5 commits into from
May 22, 2017
Merged

Add support for internal metadata view interfaces #10

merged 5 commits into from
May 22, 2017

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented May 20, 2017

This gives VS MEF the ability to generate metadata view dynamic types that derive from internal interfaces. .NET MEF and NuGet MEF don't have this capability, but it is achievable using the CLR's assembly attribute that tells it to disregard visibility checks.

Closes #9

@AArnott AArnott self-assigned this May 20, 2017
@AArnott AArnott requested review from davkean and AlexEyler May 20, 2017 14:49
@msftclas
Copy link

@AArnott,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@AArnott
Copy link
Member Author

AArnott commented May 20, 2017

@davkean Did you want this before 15.6?

@davkean
Copy link
Member

davkean commented May 22, 2017

@AArnott No hurry, 15.6 is fine. Thanks for doing this!

AlexEyler
AlexEyler approved these changes May 22, 2017
View reviewed changes
src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs
private TypeInfo EmitMagicAttribute()
{
var tb = this.moduleBuilder.DefineType(
"System.Runtime.CompilerServices.IgnoresAccessChecksToAttribute",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there docs for this attribute? I haven't heard of it before, but it definitely seems to come in handy here. I'm assuming that this attribute is supported in .NET 4.5, right?

Copy link
Member Author

@AArnott AArnott May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not documented on MSDN. It is in CoreCLR and .NET Framework, although I'm not sure which version started supporting it. If vs-mef runs on a version that doesn't support it, it will just be ignored and we'll "regress" to behavior before this PR. This PR won't make anything worse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a bug to get the attribute documented: Bug 438899: Document IgnoresAccessChecksToAttribute

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET 4.6 added support for this attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - that should be fine in VS since we require .NET 4.6. I was initially worried about the situations where we launch devenv /updateConfiguration when only .NET 4.5 is installed - this can happen if we're pending reboot to install .NET 4.6. However, I just remember this was "fixed" in 15.2, though, by avoiding composing MEF in the cases where .NET 4.6 is not installed (DevDiv PR 66331). The next launch of VS will do the composition in that case.

@AArnott AArnott merged commit 2047839 into master May 22, 2017
@AArnott AArnott deleted the dev/andarno/internalMetadataViews branch May 22, 2017 18:44
@KirillOsenkov
Copy link
Member

Dang, this adds 22 more test failures on Mono :(
I will investigate.

@AArnott
Copy link
Member Author

AArnott commented May 25, 2017

Odd, unless all those tests are the new ones I added for the feature. I didn't expect any regressions.

@KirillOsenkov
Copy link
Member

Not sure if this stack will provoke any insights:

System.IndexOutOfRangeException : Index was outside the bounds of the array.
  at (wrapper stelemref) System.Object:virt_stelemref_sealed_class (intptr,object)
  at System.Reflection.Emit.ModuleBuilder.AddType (System.Reflection.Emit.TypeBuilder tb) [0x0004f] in <bf0067fdc1d74a58b735fd97c769f75e>:0 
  at System.Reflection.Emit.ModuleBuilder.DefineType (System.String name, System.Reflection.TypeAttributes attr, System.Type parent, System.Type[] interfaces, System.Reflection.Emit.PackingSize packingSize, System.Int32 typesize) [0x0003f] in <bf0067fdc1d74a58b735fd97c769f75e>:0 
  at System.Reflection.Emit.ModuleBuilder.DefineType (System.String name, System.Reflection.TypeAttributes attr, System.Type parent, System.Type[] interfaces) [0x00000] in <bf0067fdc1d74a58b735fd97c769f75e>:0 
  at System.ComponentModel.Composition.MetadataViewGenerator.GenerateInterfaceViewProxyType (System.Type viewType) [0x00030] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.MetadataViewGenerator.GenerateView (System.Type viewType) [0x0003e] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.MetadataViewProvider.GetMetadataView[TMetadataView] (System.Collections.Generic.IDictionary`2[TKey,TValue] metadata) [0x0003d] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.AttributedModelServices.GetMetadataView[TMetadataView] (System.Collections.Generic.IDictionary`2[TKey,TValue] metadata) [0x0000b] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ExportFactoryCreator.CreateStronglyTypedExportFactoryOfTM[T,M] (System.ComponentModel.Composition.Primitives.Export export) [0x0005e] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ExportFactoryCreator+<>c__DisplayClass4_0.<CreateStronglyTypedExportFactoryFactory>b__0 (System.ComponentModel.Composition.Primitives.Export e) [0x00000] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ImportingItem.CastSingleExportToImportType (System.Type type, System.ComponentModel.Composition.Primitives.Export export) [0x00018] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ImportingItem.CastExportsToSingleImportType (System.ComponentModel.Composition.Primitives.Export[] exports) [0x00023] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ImportingItem.CastExportsToImportType (System.ComponentModel.Composition.Primitives.Export[] exports) [0x00016] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ReflectionComposablePart.SetImport (System.ComponentModel.Composition.ReflectionModel.ImportingItem item, System.ComponentModel.Composition.Primitives.Export[] exports) [0x00000] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.ReflectionModel.ReflectionComposablePart.SetImport (System.ComponentModel.Composition.Primitives.ImportDefinition definition, System.Collections.Generic.IEnumerable`1[T] exports) [0x00047] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.ImportEngine+PartManager.TrySetImport (System.ComponentModel.Composition.Primitives.ImportDefinition import, System.Collections.Generic.IEnumerable`1[T] exports) [0x00006] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.ImportEngine.TrySatisfyImportSubset (System.ComponentModel.Composition.Hosting.ImportEngine+PartManager partManager, System.Collections.Generic.IEnumerable`1[T] imports, System.ComponentModel.Composition.Hosting.AtomicComposition atomicComposition) [0x00065] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.ImportEngine.TrySatisfyImportsStateMachine (System.ComponentModel.Composition.Hosting.ImportEngine+PartManager partManager, System.ComponentModel.Composition.Primitives.ComposablePart part) [0x000c6] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.ImportEngine.TrySatisfyImports (System.ComponentModel.Composition.Hosting.ImportEngine+PartManager partManager, System.ComponentModel.Composition.Primitives.ComposablePart part, System.Boolean shouldTrackImports) [0x00041] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.ImportEngine.SatisfyImportsOnce (System.ComponentModel.Composition.Primitives.ComposablePart part) [0x00030] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.Hosting.CompositionContainer.SatisfyImportsOnce (System.ComponentModel.Composition.Primitives.ComposablePart part) [0x00057] in <22e18bf634fa421199510c92b8041f78>:0 
  at System.ComponentModel.Composition.AttributedModelServices.SatisfyImportsOnce (System.ComponentModel.Composition.ICompositionService compositionService, System.Object attributedPart) [0x0001d] in <22e18bf634fa421199510c92b8041f78>:0 
  at Microsoft.VisualStudio.Composition.Tests.MefV1ExportProviderAdapterTests.SatisfyImportsOnceWithExportFactoryAndTMetadata (Microsoft.VisualStudio.Composition.Tests.IContainer container) [0x0000e] in <aeb12ddba7f04385b7e260054ef982e6>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <bf0067fdc1d74a58b735fd97c769f75e>:0

@AArnott
Copy link
Member Author

AArnott commented May 25, 2017

That is a MEFv1 failure (i.o.w. mono's MEF implementation has a bug). It looks like mono MEF can't handle RefEmit of a type that implements an internal interface in another assembly very well. As in, RefEmit crashes badly.

@AArnott AArnott added this to the 15.5 milestone Dec 22, 2017
@AArnott AArnott mentioned this pull request May 5, 2018
Closed
@stakx stakx mentioned this pull request Jul 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexEyler AlexEyler AlexEyler approved these changes

@davkean davkean Awaiting requested review from davkean

Assignees

@AArnott AArnott

Labels
cla-not-required
Projects
None yet
Milestone
15.5
Development

Successfully merging this pull request may close these issues.
5 participants
@AArnott @msftclas @davkean @KirillOsenkov @AlexEyler