Remove the use of IsPartialFacadeAssembly in refererences

[ericstj]

This flag requires assembly-re-writing to replace type-defs in reference
assemblies with type forwards, when the same type exists in references.

We've pretty much exclusively used this on .NETFramework, since it's
not friendly to source build (CCI isn't part of source build).
.NETFramework isn't going to be changing so it doesn't save us much
to generate these typeforwards in the build.

We also used these to make sure the netstandard surface area was
compatible with .NETFramework facades (we'd use the pre-rewritten
reference assemblies for contract), but this need goes away now that we
have package validation based APICompat that compares netstandard refs
to net4x facades.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This flag requires assembly-re-writing to replace type-defs in reference
assemblies with type forwards, when the same type exists in references.

We've pretty much exclusively used this on .NETFramework, since it's
not friendly to source build (CCI isn't part of source build).
.NETFramework isn't going to be changing so it doesn't save us much
to generate these typeforwards in the build.

We also used these to make sure the netstandard surface area was
compatible with .NETFramework facades (we'd use the pre-rewritten
reference assemblies for contract), but this need goes away now that we
have package validation based APICompat that compares netstandard refs
to net4x facades.

Author:	ericstj
Assignees:	-
Labels:	area-Infrastructure-libraries, new-api-needs-documentation
Milestone:	-

[ViktorHofer]

Everywhere else you are conditioning on TargetFramework directly. For consistency you want to do this here (and in L12&L13) as well as there is only one .NET Framework and one .NET Standard tfm. In cases where there are multiple, this would of course make sense.

[ericstj]

I was trying to follow as consistent of a pattern as I could across the change but also wanted to honor the style of the project I was modifying if there is a clear pattern established.

Did you have a suggestion here? Were you saying that I should prefer a TargetFramework comparison when we only needed to apply the condition to a single value?

[ViktorHofer]

Right. Usually we condition on TargetFramwork directly if there is only a single tfm per universe and only use the inferred properties (TFI, TFV) when conditioning on multiple tfms of a universe. In this case this is a big NIT. Feel free to just ignore my feedback :)

[ericstj]

I've actually discussed with folks before (I think it was @zsd4yr) about style-cop on projects/targets. It'd be nice if we could a) define a set of unambiguous style guidelines b) codify enforcement of those.

[ViktorHofer]

For my better understanding, let's think this through.

Instead of rewriting of APIs exposed which exist on .NET Framework to type forwards, we now check in a fixed state of type forwards. We don't expect .NET Framework APIs to ever change again and if they do we should be easily able to react to that.
Most of the assemblies on .NET Framework are full facades and only very few (i.e. ACLs) are partial facades. If we would expose a new API in a .NET Framework assembly and that type already exists in the framework, would the dev need to manually add the type forward? If yes, should we document that requirement?

How do the source assemblies generate the type forwards (for full and the set of partial facades)? The IsPartialFacadeAssembly property is still in use there which makes me wonder if the same thing happens again for the source ones.

And side question, why do we even need the ref assemblies for .NET Framework in packages? Is that because otherwise potential .NET Standard refs in the ref folder would win?

[ericstj]

Instead of rewriting of APIs exposed which exist on .NET Framework to type forwards, we now check in a fixed state of type forwards. We don't expect .NET Framework APIs to ever change again and if they do we should be easily able to react to that.
Most of the assemblies on .NET Framework are full facades and only very few (i.e. ACLs) are partial facades.

Correct.

If we would expose a new API in a .NET Framework assembly and that type already exists in the framework, would the dev need to manually add the type forward? If yes, should we document that requirement?

I think we can do better about documenting refs here: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md
I'll add a section here around .NETFramework. The scenario you describe would be caught by package-validation-based API compat.

How do the source assemblies generate the type forwards (for full and the set of partial facades)? The IsPartialFacadeAssembly property is still in use there which makes me wonder if the same thing happens again for the source ones.

We use the reference assemblies to decide which types to forward in implementation. Partial facades consider the set of types defined and forwarded in the reference assembly and make sure that type forwards are generated when the type is not defined in source. Because source has a "contract" to copy from it doesn't rewrite anything.

The ref-facade infra was really a special case where it used an intermediate compiled assembly as the contract as a convenience to avoid this sort of if-def'ing. -- That's much less of an issue now that the number of assemblies we ship these types of packages for has gone down from 100s to 10s.

We also benefited from the ref-facade infra for APICompat as it gave us an intermediate assembly to use to check on what API we expected to be in .NETFramework. This coverage will now be provided more naturally through package-based API compat which compares the compatible netstandard assembly with the netframework facade.

And side question, why do we even need the ref assemblies for .NET Framework in packages? Is that because otherwise potential .NET Standard refs in the ref folder would win?

In most cases we don't need them in the package and don't include them in the package. Reasons we have to include them that remain today: the reference has different surface area (eg: type definition in ref, forward in src), or as-you-mention that is true for some compatible framework and a reference is needed to replace that. For most, if not all, of these they should be excluded from the package. I mentioned this to @Anipik as he's doing the pkgproj conversion we can actually do this cleanup if we diff the refs as part of that work.

[ViktorHofer]

Merged to test my arcade changes on top of main. Thanks a lot Eric

[ericstj]

@dseefeld this was the issue blocking source-build of the all-configurations leg. If you get around to retrying that it may be unblocked now.

[Anipik]

lgtm