Only add a type forward if the nested type is visible outside of the assembly #275
Conversation
|
Hi @joperezr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
| @@ -495,7 +495,8 @@ private void AddTypeForward(Assembly assembly, INamedTypeDefinition seedType) | |||
| // but the runtime currently throws a TypeLoadException without explicit forwarders for the nested | |||
| // types. | |||
| foreach (var nestedType in seedType.NestedTypes.OrderBy(t => t.Name.Value)) | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
What did you think about my question about InternalsVisibleTo in the issue. I believe it's OK for GenFacades to ignore this case, but I would:
Assembly A [assembly: InternalsVisibleTo(C)]
public class X { internal class Y {} }Assembly B, generated by GenFacades -> A [assembly: TypeForwardedTo(typeof(X))]Assembly C, uses A via type forward from B class Z : X.Y { }
I think probably not, but we should deliberate doc that here. If it does need to support it, then I think we have to keep anything with internal visibility because the InternalsVisibleTo of target could change over time. Also, did you check what Roslyn compiler now emits in these cases? I am curious and think we should follow up with them if they still emit entries for private nested classes. Finally, please do an IL diff of Facades in corefx generated before and after this change and make sure that nothing unexpected is getting stripped. |
|
I also think that is a non common case, but if you think we want to support it I can do it. If not, where do you want me to document it? I haven't check yet what Roslyn compiler does nor have I done the IL diff. I'll do both tasks today and report back on the tracking issue, just wanted to start the PR first. |
|
Actually, I just realized a better fix that would make the whole internal vs. private thing moot. Instead of enumerating the nested types of the destination, we should enumerate the nested types of the contract and forward those. We won't generally have any private nested classes in contracts. |
|
+1 to Nick's last suggestion that we should use the contract as the filter. |
|
@joperezr, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
joperezr
force-pushed
the
RemoveNonVisibleExportedTypesGenFacades
branch
from
c87ddef to
c8abb93
Compare
|
@weshaggard @nguerrera Let me know if the last commits are what you envisioned. I already tested it in the corefx build and it is passing, and also I've checked the ILs of the assemblies produced and seems like it's doing the right thing. |
|
Ping! :) |
| } | ||
| } | ||
| else // Add all the nested types for ExportedTypes. | ||
| foreach (var nestedType in seedType.NestedTypes.OrderBy(t => t.Name.Value)) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
… are visible outside the assembly.
…d of the seedtype.
…th all of their nested types always
…e Nested types there and include them in the docId list as well.
joperezr
force-pushed
the
RemoveNonVisibleExportedTypesGenFacades
branch
from
e4afc4f to
208ca14
Compare
|
I already tested my last changes with corefx repo, and everything seems to work correctly except that I'm running into the known issue dotnet/corefx#3726 The rest of the tests pass just fine. |
| { | ||
| Trace.TraceError(" /preferSeedType:{0}={1}", docId.Substring("T:".Length), type.ContainingUnitNamespace.Unit.Name.Value); | ||
| Trace.TraceError(" /preferSeedType:{0}={1}", docId.Substring("T:".Length), GetContainingUnitNamespaceFromType(type)); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
…nFacades Only add a type forward if the nested type is visible outside of the assembly
|
Forgot to squash before merging. Sorry for the trouble! |
Only add a type forward if the nested type is visible outside of the assembly. This is the fix for issue #274