Fix support for multiple internal metadata view interfaces defined from multiple assemblies #93
Conversation
…assemblies This repros #91
This worksaround the CLR limitation that IgnoresAccessChecksToAttribute attributes are only observed once then cached, preventing any later-added attributes from being noticed. Fixes #91
We generate a new dynamic assembly for each unique set of skip visiblity checks now, which is the minimum we can get away with given the CLR limitations.
|
CC: @davidwrighton |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
- Coverage 85.79% 85.78% -0.02%
==========================================
Files 67 67
Lines 5598 5613 +15
Branches 869 871 +2
==========================================
+ Hits 4803 4815 +12
- Misses 558 559 +1
- Partials 237 239 +2
Continue to review full report at Codecov.
|
| var result = ImmutableHashSet<AssemblyName>.Empty | ||
| .Add(typeInfo.Assembly.GetName()); | ||
|
|
||
| // If this type has a base type defined in another assembly that is also internal |
There was a problem hiding this comment.
Did you mean to leave the commented code here?
There was a problem hiding this comment.
Yes, since it really feels like it should be here, and I want it to remind me to ask the CLR folks why I don't need visibility into transitive assemblies when I only ask to skip visibility checks for the first assembly.
There was a problem hiding this comment.
Because what's happening there is that access checks are only done on the metadata as written. For instance, A derives from B derives from C. As written, A only knows about B, so the access check from A is only to see if it can access B. For access to C, the only access check is that B is allowed to access C. These rules are straightforward as long as no techniques are used to skirt around access checks, but when you do, you can see surprises like this.
There was a problem hiding this comment.
OK, thanks. If A defines a member that my proxy implements, evidently that doesn't require C's visibility into A. I guess if A defines a member with a Type that is internal to A, at that point C couldn't implement it without an explicit skip visibility check for A. Yes?
| // because the CLR will not honor any additions to that set once the first generated type is closed. | ||
| // So maintain a dictionary to point at dynamic modules based on the set of skip visiblity check assemblies they were generated with. | ||
| var skipVisibilityCheckAssemblies = SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(viewType); | ||
| if (!TransparentProxyModuleBuilderByVisibilityCheck.TryGetValue(skipVisibilityCheckAssemblies, out ModuleBuilder moduleBuilder)) |
There was a problem hiding this comment.
If the type is public, do you still need to do this? (i.e when skipVisibilityCheckAssemblies is ImmutableHashSet<AssemblyName>.Empty
There was a problem hiding this comment.
When the set is empty, we still need a dynamic assembly to generate into. Our lookup dictionary needs an entry for the empty set as well so we don't end up special casing empty sets -- we execute the same code regardless of the size of the set.
Embed PDBs for all local builds This ensures that if you build a dll and copy it somewhere, then build again, you haven't lost the ability to debug the original dll because it carries the pdb with it wherever it goes. We do not embed the pdb in the official builds however because the official builds will archive the pdb so it isn't lost, and it would unnecessarily bloat the assembly.
This fixes our metadata view code gen so that when implementing internal interfaces, we generate a new dynamic assembly each time a request comes in that requires a unique set of skip visiblity check attributes. When a request comes in that coincides with the set of skip check attributes of a dynamic assembly we've already opened, we reuse that one to keep the dynamic assembly count to a minimum.
Fixes #91