Port System.ServiceModel new docs

[carlossanlop]

@HongGit @mconnew @directhex @gewarren PTAL

[mconnew]

Something has gone very wrong with the documentation generator here. The class TransportDuplexSessionChannel is internal, but documentation has been generated for it. There's an error about a cross reference to System.ServiceModel.Channels.IAsyncDuplexSession not being found, but that's an internal interface implemented on an internal class.

It looks like there's some bugs in the generator picking up api's it shouldn't.

[mconnew]

I think I see the problem, looking at the preview page, it lists the location of that class as being System.Private.ServiceModel.dll in v4.10.3. It might have been declared as public in that assembly in that version, but that assembly should never have been scanned for public api's as the nuget package isn't intended to be directly referenced. It has an empty ref folder so that if it is referenced, you don't get to compile against any api's.

[carlossanlop]

@mconnew the last preview that was ingested into dotnet-api-docs was preview6, and the TransportDuplexSessionChannel type did not get modified there: #10114

So it means we have been incorrectly scanning that DLL you mention since way before.

As you said, it must be getting detected as public in System.Private.ServiceModel.dll, because that's how we show it here:

dotnet-api-docs/xml/System.ServiceModel.Channels/TransportDuplexSessionChannel.xml

Line 2 in d6e3337

<TypeSignature Language="C#" Value="public abstract class TransportDuplexSessionChannel : System.ServiceModel.Channels.TransportOutputChannel, System.ServiceModel.Channels.ISessionChannel&lt;System.ServiceModel.Channels.IAsyncDuplexSession&gt;, System.ServiceModel.Channels.ISessionChannel&lt;System.ServiceModel.Channels.IDuplexSession&gt;" />

@gewarren what can we do to remove anything that came from that DLL?

[carlossanlop]

Are we incorrectly including that DLL in the transport packages you use to update the CI here, @gewarren?

[mconnew]

I checked, it is public in the private package for 4.10 as seen here. That type got moved to the System.ServiceModel.NetFramingBase package as part of removing the old private package, and was made internal as part of the process.

[gewarren]

The tool that grabs the NuGet packages looks for any packages published by aspnet or dotnetframework that start with System, Microsoft.Bcl, Microsoft.Extensions, or Microsoft.Win32. I'll add a special case to exclude System.Private.ServiceModel and rerun the CI, so those APIs should go away.

[carlossanlop]

Thank you @gewarren!

Let's wait for the CI update so I can rebase this PR and we determine if there are still any comments that need porting.

[carlossanlop]

CI update: #10164

[carlossanlop]

@mconnew the CI update has been merged and this PR has been rebased. Would you mind taking another look?

[gewarren]

??

[carlossanlop]

@mconnew can you suggest a more meaningful return value here?

[carlossanlop]

Ping @mconnew

[gewarren]

??

[carlossanlop]

@mconnew can you suggest a more meaningful return value here?

[carlossanlop]

Ping @mconnew

[mconnew]

There's still a few problems. SyncMethodInvoker, TaskMethodInvoker, ServiceChannelProxy, and OutputChannel are public in our implementation assembly, but are purposefully absent from our ref assembly.

I don't fully understand the schema for these files so can't tell if it's correct or not, but ClientBase<T> only implements IAsyncDisposable on .NET6+.

I also see another problem, looking at the CLientBase<T> details for System.IAsyncDisposable.DisposeAsync, under AssemblyInfo, it's listing System.Private.ServiceModel, assembly version 4.10.3.0. That api wasn't in 4.10.3, but if it was, the assembly info should be pointing to System.ServiceModel.Primitives because that's where the ref assembly (with matching name).

[gewarren]

There's still a few problems. SyncMethodInvoker, TaskMethodInvoker, ServiceChannelProxy, and OutputChannel are public in our implementation assembly, but are purposefully absent from our ref assembly.

There's a special case for System.ServiceModel.Primitives where we use the reference assemblies instead of the implementation assemblies - but the behavior you're describing seems to be the opposite of that. Should I remove this special behavior? If I recall correctly, it was necessary for the ingestion logic to avoid hitting an error.

I don't fully understand the schema for these files so can't tell if it's correct or not, but ClientBase<T> only implements IAsyncDisposable on .NET6+.

It currently shows as only applicable to .NET 8+ (and .NET Core 1.0/1.1 and .NET Framework) - https://review.learn.microsoft.com/en-us/dotnet/api/system.servicemodel.clientbase-1?view=net-8.0&branch=pr-en-us-10157#applies-to.

I also see another problem, looking at the CLientBase<T> details for System.IAsyncDisposable.DisposeAsync, under AssemblyInfo, it's listing System.Private.ServiceModel, assembly version 4.10.3.0. That api wasn't in 4.10.3, but if it was, the assembly info should be pointing to System.ServiceModel.Primitives because that's where the ref assembly (with matching name).

I don't think that info is displayed anywhere, so this might just be some leftover info that wasn't cleaned up when System.Private.ServiceModel was removed.

[carlossanlop]

Should I remove this special behavior?

@mconnew this is a question for you. Do you mind answering it so we can advance this PR?

[mconnew]

@gewarren, the reference assemblies are what is supposed to be used. The situation used to be that the implementation was in the System.Private.ServiceModel package in a lib folder. We created a ref folder with a dummy empty file (otherwise nuget doesn't see the folder or removes it, something bad happened anyway) so that if someone added the package, the compiler wouldn't see any of the implementation. Then in the System.ServiceModel.* packages (e.g. Primitives) we had a ref folder which had all the api's. In the lib folder, we had a type forwarding assembly which just redirected the runtime to the System.Private.ServiceModel.dll implementation to find the types. It just had type forwarding attributes and no implementation.

Now we've got rid of System.Private.Servicmodel. How things are right now is slightly different from how they will be in a few months. Currently every package except Primitives only has an implementation assembly which will then double for being the reference assembly. For Primitives we have a ref and a lib folder. The ref folder contains the reference assembly and has all the public types. In the lib folder, we have the implementation assembly which has extra types which are public. Some are unintentional (but it never mattered before), some are intentional so that the other packages can use them. It avoids having to use InternalsVisibleTo. SyncMethodInvoker and TaskMethodInvoker are unintentionally public but do not appear in the reference assembly.

I'm currently working on changes where NetTcp and Http will also have reference assemblies. Because we already did the work to make sure nothing was unintentionally public, the public api surface of the reference assembly and the implementation assembly will be the same for .NET. We're bringing back netstandard2.0 which will have some api's excluded as they aren't available on .NET Framework. And we'll have type forwarding facades for .NET Framework.

[gewarren]

@mconnew What do you need me to do to resolve the blocker for this PR? Is that remove SyncMethodInvoker, TaskMethodInvoker, ServiceChannelProxy, and OutputChannel?

[mconnew]

@gewarren, these are just a few types which I noticed. Basically you should be scanning for types in the ref folders first and only looking in the lib folder if there isn't a ref folder. We keep getting inquiries about various types or methods which are documented but aren't available because of this issue. We just had one inquiring about a method on SecurityBindingElement which isn't exposed but is documented as such. A few days ago we had someone inquiring about the enum type InstanceContextMode which isn't exposed by the ref assembly but is documented as available too. I don't want to play whack a mole with types and methods, the correct assembly should be scanned.

[gewarren]

@gewarren, these are just a few types which I noticed. Basically you should be scanning for types in the ref folders first and only looking in the lib folder if there isn't a ref folder.

@huangmin-ms Is this 👆 something that mdoc is capable of?

[huangmin-ms]

@gewarren, these are just a few types which I noticed. Basically you should be scanning for types in the ref folders first and only looking in the lib folder if there isn't a ref folder.

@huangmin-ms Is this 👆 something that mdoc is capable of?

@gewarren Thanks for reporting. We already support this and I can also see the configuration is correct.

https://apidrop.visualstudio.com/_git/binaries?path=/dotnet/net-8.0/net-8.0.csv&version=GBmaster&line=230&lineEnd=230&lineStartColumn=1&lineEndColumn=79&lineStyle=plain&_a=contents

pac230,[tfm=net8.0;includeXml=False;libpath=ref]System.ServiceModel.Primitives,8.0.0

However this is impacted by the source link change, we will fix it in https://ceapex.visualstudio.com/Engineering/_workitems/edit/996190

[huangmin-ms]

@gewarren, these are just a few types which I noticed. Basically you should be scanning for types in the ref folders first and only looking in the lib folder if there isn't a ref folder.

@huangmin-ms Is this 👆 something that mdoc is capable of?

@gewarren The fix is done. Please help to review the latest CI update: #10385 Thanks.

[gewarren]

@mconnew Just to clarify, you want the docs to use the reference assemblies for all System.ServiceModel.* packages?

[mconnew]

@gewarren, not exactly. Not all packages have reference assemblies. Currently only Primitives has a reference assembly, but that will be changing at some time in the future. Does the docs tooling require knowing ahead of time whether to look at a ref assembly of the lib?

[gewarren]

@gewarren, not exactly. Not all packages have reference assemblies. Currently only Primitives has a reference assembly, but that will be changing at some time in the future. Does the docs tooling require knowing ahead of time whether to look at a ref assembly of the lib?

Yes. Currently only System.ServiceModel.Primitives is special cased that way.

[mconnew]

That works for now, and when we release new package versions with ref assemblies that should be scanned instead, I'll let you know.

[carlossanlop]

What should I do about this PR?

[carlossanlop]

Closing. Will resubmit after the RC1 update.