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

Mark proxy assembly with IgnoresAccessChecksToAttribute to allow implementing non public interfaces #1814

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

hazzik
Copy link
Member

@hazzik hazzik commented Jul 20, 2018

Some serious black magic here. This is supported at least in .NET 4.6+ and .NET Core 2.0. In .NET Standard fallback to the old method.

No new tests are needed. VerifyProxyForClassWithInternalInterface and VerifyFieldInterceptorProxy of StaticProxyFactoryFixture already check this.

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

The only reference I find about this attribute being supported outside of CoreRT is a rejected PR: dotnet/roslyn#20870. Is there any better reference about it?

And why not adding some tests, like an entity using an internal interface in DomainModel project?

Update: ok, there are already tests with internal interface.

.Distinct();
foreach (var a in assemblyNamesToIgnoreAccessCheck)
ProxyBuilderHelper.GenerateInstanceOfIgnoresAccessChecksToAttribute(assemblyBuilder, a);
#else
Copy link
Member

Choose a reason for hiding this comment

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

No need to change FieldInterceptorProxyBuilder: its interface list is hard-coded and never contain any non-public one. This proxy does not need to re-implement interfaces from the proxied class, it just inherits it and only intercept base class properties. It does not need to intercept other methods because its state is still in the inherited class, contrary to the lazy entity loading proxy which delegate its state to one of its member.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, then we need to remove interfaces.RemoveWhere(i => !i.IsVisible) from here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I had overlooked this one.

@fredericDelaporte
Copy link
Member

I am quite reluctant to take a dependency on such "black magic" tricks. If some later update removes the support of this, NHibernate would be no more usable with entities implementing an internal interface. In current master state, it is usable, but with the risk that if a proxified entity ends up in a method using the internal interface, this internal interface may not work on the appropriate state. (It depends on whether the interface implementation access the entity state through overridden methods or not.)

@hazzik
Copy link
Member Author

hazzik commented Jul 20, 2018

The only reference I find about this attribute being supported outside of CoreRT is a rejected PR: dotnet/roslyn#20870. Is there any better reference about it?

Unfortunately no, there is no any references about this attribute over the internet. I was accidentally found the information of this attribute. I noted that it could be great to use it here. But later when I finally decided to implement this trick I spend several hours trying to find where I saw it as I forgot the attribute name. (Just found that it was in my email subscription to some thread.)

As I understand the mentioned PR is not related anyhow to the problem of low level documentation. The PR was about adding the support of the attribute on the language level (similar to InternalsVisibleToAttribute) unbarring the usage of internal members from unrelated assemblies. So it was turned-down because, unlike InternalsVisibleTo attribute which requires you to "trust" other assemblies with "keys" to the internals, this attribute can lock-pick any assembly. So, they decided it would be better to require from user a conscious decision to break a law and use a brute force (a reflection) to break into the assembly internals.

If some later update removes the support of this

I highly doubt this. The attribute has already spread it's tentacles into the codebases: several projects (System.Reflection.DispatchProxy, vs-mef, dnSpy) are using it; and several projects (Microsoft Orleans, Castle.Core (Just noticed that for Castle.Core the same comment triggered this consideration)) are considering using it. Closest one to what we need are DisapatchProxy and Castle.Core. (Castle.Core has a nice summary)

In current master state ...

I would like to fail hard than allow latent bugs sneak into the user apps.

The complimentary to this we need to make assembly names static (preferably call all of them "DynamicProxyGenAssembly2" to be inline with other proxy frameworks) to allow users to add InternalsVisibleTo attribute to permit proxies to use the internals.

@fredericDelaporte
Copy link
Member

Ok then.

@hazzik hazzik force-pushed the internal-interfaces branch from 8e60922 to 78c0b36 Compare July 23, 2018 02:23
@hazzik
Copy link
Member Author

hazzik commented Jul 23, 2018

Rebased to resolve conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants