Need to retain type forward information for forwarded types

[TianqiZhang]

This is related to #103 and #342. Now that both of them are closed, there's still one missing piece.

Take Dictionary<TKey,TValue> as an example, the full assembly info should include information like in below 2-dimensional matrix:

assembly/frameworks	.Net Framework	.NET Core	.NET Standard
System.Collections	❌	✔️	❌
mscorlib	✔️	❔	❌
netstandard	❌	❌	✔️

I can somehow put together all information in this matrix except for the ❔. Now we are showing ❔ as ✔️ too, however it should be "forwarded".

I'm wondering if there's a good way to include this information in ECMAXML.

[joelmartinez]

@TianqiZhang as discussed in that other thread, the mscorlib in netcore-2.1 is currently explicitly listed in the list of assemblies that are part of that moniker, so that's part of the confusion. However, if it were to be moved into the dependencies folder, it would be used for resolving, and excluded from the list in the frameworks index file.

Would that be enough to properly figure out how to mark the info in that matrix? If an assembly is in the frameworks index file, then it's ✔️, but if it's only listed on the type/member AssemblyInfo, then that assembly would be forwarded?

[TianqiZhang]

@joelmartinez I see what you mean. @mairaw is there a special reason for mscorlib.dll to sit in netcore-2.1 folder in binaries repo?

It looks like we either manually move the facade assemblies to dependency folder, or we identify them when doing reflection. @joelmartinez when ECMA2Yaml generates the _moniker2assembly.json mapping, it excludes facades which doesn't have any real types defined:
https://github.com/docascode/ECMA2Yaml/blob/master/ECMA2Yaml/ECMA2Yaml/AssemblyLoader.cs
Can similar logic be added to mdoc?

[mairaw]

That's the drop I get from the PU. It includes facades as well.

[joelmartinez]

can we 100% guarantee that a façade assembly is defined by the fact that it only contains exported types? And if so, that mdoc would never want to include this assembly in the list of assemblies on the frameworks index file?

I don't know … that seems like one of those things that is just asking to be proved wrong :P

@dend, thoughts?

[TianqiZhang]

@joelmartinez I'd like to request mdoc to add a flag for these facade assemblies in frameworks index file, so that downstream can have the flexibility to handle them for different requirements.

Per my understanding, there are at least 3 places that used assembly info:

1. page apiscan metadata. Facades are currently there, and they should be. ✔️
2. assemblies list on top of the page. Facades are currently there, however they shouldn't be.❌
3. intellisense build. Facades are excluded for now, with an additional moniker2assembly mapping json that is generated in CI. We'd like to get rid of this json and directly use mdoc's output.🙋

If mdoc can flag out the facade assemblies, it will greatly help in No.2 and No.3 scenarios. cc @mairaw @dend

[joelmartinez]

@TianqiZhang understood. I would still like to get verification that a façade assembly is defined by only containing exported types and nothing else (perhaps @terrajobst could comment?).

Implementation Notes:

• The attribute would be added here
• This data structure (used in the previous point), would have to be enhanced to include the façade flag (a bool).

[TianqiZhang]

@joelmartinez here I found an article about facade assembly: https://github.com/dotnet/standard/blob/master/docs/history/evolution-of-design-time-assemblies.md
I think there's no strict technical definition of it, just a concept.

I gave it more thinking during the past weeks. Marking an assembly to be "facade" in framework index file is relatively easy to implement for us, however that's a bit "coarse-grained" because it can't handle the "partial facade" case, which means an assembly can both contain forwarded types and define its own type at the same time.

On the other hand, marking type forward info in <AssemblyInfo> tag in each type could be an alternative way. For example we could have something like this:

  <AssemblyInfo>
    <AssemblyName>mscorlib</AssemblyName>
    <AssemblyVersion>1.0.5000.0</AssemblyVersion>
    <AssemblyVersion>2.0.0.0</AssemblyVersion>
    <AssemblyVersion>2.0.5.0</AssemblyVersion>
    <AssemblyVersion>4.0.0.0</AssemblyVersion>
    <AssemblyVersion forwarded="true" frameworkAlternative="netcore-2.1;netcore-2.2">4.0.0.0</AssemblyVersion>
  </AssemblyInfo>

This won't make the ECMAXML much bigger, because type forwarding only happens on type level, so we don't need to touch <AssemblyInfo> nodes in members.

What do you think? //cc @mairaw @dend

[joelmartinez]

@TianqiZhang wouldn't it also need to tell what assembly it was forwarded to ?

[TianqiZhang]

@joelmartinez oh what I meant by forwarded="true" is that the current type is not actually in this assembly, it is forwarded from somewhere else. How about I change it to forwardedFrom and fill it with the assembly name where this type is really from? For example for type Dictionary<TKey,TValue> we can have something like this:

  <AssemblyInfo>
    <AssemblyName>mscorlib</AssemblyName>
    <AssemblyVersion>1.0.5000.0</AssemblyVersion>
    <AssemblyVersion>2.0.0.0</AssemblyVersion>
    <AssemblyVersion>2.0.5.0</AssemblyVersion>
    <AssemblyVersion>4.0.0.0</AssemblyVersion>
    <AssemblyVersion forwardedFrom="System.Collections" frameworkAlternative="netcore-2.1;netcore-2.2">4.0.0.0</AssemblyVersion>
  </AssemblyInfo>

[joelmartinez]

Ahh! ok, yes this is much clearer as far as communicating intent :)

Some implementation notes/thoughts

The trick here from an implementation perspective is going to be that, the way the Mono.Cecil API works (and by extension, mdoc's data structures), is that resolving the type (which happens early on in the process) passes through some behind-the-scenes classes, so by the time mdoc is enumerating through that type, it only sees the actual assembly that contains the type definition (so in the example case you've got here, mdoc only sees mscorlib where the definition exists, and doesn't know anything about System.Collections because that information was abstracted away during the resolving stage.

That being said, we have a mechanism that we might be able to use. I added a TypeExported event to the resolver (here) … we should be able to use that to perhaps build up a forward mapping that we might be able to use here.

[TianqiZhang]

aha, thanks for the heads up. A forward mapping seems handy. I took a brief look at the code, it seems that you already have the forwarding chain recorded in forwardedTypesTo in AssemblySet.cs.

So just another example for communicating intent (😁), can we add a bunch of <TypeForwardingChain> nodes like below under <Type>?

<Type Name="Dictionary&lt;TKey,TValue&gt;" FullName="System.Collections.Generic.Dictionary&lt;TKey,TValue&gt;">
  <TypeSignature Language="C#" Value="blablabla" />
  <TypeForwardingChain frameworkAlternative="netcore-2.1;netcore-2.2">
    <TypeForwarding From="mscorlib" To="System.Collections"/>
  </TypeForwardingChain>
</Type>

[TianqiZhang]

@joelmartinez any thoughts on this?

[joelmartinez]

yeah, something like that should definitely be possible … would definitely make it clearer.

[joelmartinez]

mdoc 5.7.4.10 (in this PR #432) will add a feature that will make this enhancement infinitely easier to implement. You can see the initial implementation here:
db8a85e#diff-cf20fb82aaef409362c2feac6390110aR23

Essentially, the FrameworkTypeEntry now has a data structure that lets you answer the following questions:

• What assemblies is this type included in
• What assemblies are forwards

This information is at the moniker/framework level. It still doesn't fully answer the questions at the member level though, so a bit more work will be necessary ... we will probably have to rethink how we track these assembly references, because a type/member might be in an assembly in one moniker, but forwarded to another in another. I'm tempted to say that we need to move this into the framework xml file, but those files are already so huge.

More planning TBD

[TianqiZhang]

@joelmartinez this is such great news! From the perspective of displaying the assemblies metadata and generating intellisense files, type level information is probably already enough, since "type-forwarding" only happens at type level anyway, there's no "member-forwarding" that we need to care about :P

[joelmartinez]

@TianqiZhang ahh, well that's great :D I wonder if we should get rid of assembly info at the member level entirely then?

[TianqiZhang]

@joelmartinez I think it's probably doable. Once we start to mark forwarded assemblies at type level in ECMAXML, I'll start to refactor the whole assembly-moniker-uid mapping logic in ECMA2Yaml, that might be a good chance to kill the dependency on member-level assembly xml node.

[joelmartinez]

This feature will be completed in PR #452