Improve GenApi to produce more consistent reference source

[Anipik]

While working on https://github.com/dotnet/runtime/issues/26187 I am finding a lot of issues where the current version of genapi is not correctly working. This issue is to track all those issues and hopefully resolve them in future. I will add more to the issues.

• [ ] Adding a root extensibility point.
• [ ] Not Removing Attributes. The list of attributes removed is EditorBrowsable, MaybeNull, Obsolete, CompilerGeneratedAttribute, DebuggerStepThroughAttribute, StructLayoutAttribute, ToolboxItemAttribute, ComVisibleAttribute
• [ ] Generation correct genapi for full facades
• [ ] Nullablity is also not being intrepreted correctly.
• [ ] System.decimal -> to decimal for net5.0
• [ ] Handle if def for different tfms
• [ ] ctor(type) : base (default(type)) instead of ctor(type) : base (default(string)), system.array -> int
• [ ] 100d constant shouldnt get converted to 100
• [ ] default value getting removed param=default(type) => param
• [ ] Extensions methods are not generated correctly , this keyword is skipped
• [ ] Full Facades with project references are not producing anything eg xml projects //type forwards types bit getting picked

Diff:- https://github.com/dotnet/runtime/compare/master...Anipik:UpdateRef

cc @ericstj @safern @ViktorHofer

[ericstj]

What do you mean about root extensibility point?

Attributes should not be removed. Especially not obsolete those only work at compile time. If you are noticing inconsistencies those should be addressed.

I believe the fullfacades issue was due to a condition that still depends on ReferenceFromRuntime which @ViktorHofer removed.

https://github.com/dotnet/runtime/blob/8b92fcdd1e4e80cb3eacf9c1cc9e4c9bbc2dcb03/src/libraries/Directory.Build.targets#L168

I believe there is a bug related to nullability and interfaces. https://github.com/dotnet/arcade/issues/4722

[Anipik]

What do you mean about root extensibility point?

Able to generate a reference source for all the projects at the same time.

I believe the fullfacades issue was due to a condition that still depends on ReferenceFromRuntime which @ViktorHofer removed.

I have a fix, just need to put up a pr for that.

If you are noticing inconsistencies those should be addressed.

There are couple of them, i will try to elaborate as i proceed with the issue

[ericstj]

Try setting ProjectReferenceBuildTargets when building https://github.com/dotnet/runtime/blob/master/src/libraries/src.proj That or something like it should be enough to run GenerateReferenceSource in all src projects.

[Anipik]

yeah i wrote a small target similar to runAPiCOmpat or build. :P

[ericstj]

Not Removing Attributes. The list of attributes removed is EditorBrowsable, MaybeNull, Obsolete, CompilerGeneratedAttribute, DebuggerStepThroughAttribute, StructLayoutAttribute, ToolboxItemAttribute, ComVisibleAttribute

For things that are being removed from GenAPI we should consider moving them from https://github.com/dotnet/runtime/blob/master/eng/DefaultGenApiDocIds.txt to https://github.com/dotnet/runtime/blob/master/eng/ApiCompatExcludeAttributes.txt

The former is meant to indicate Attributes which we should exclude from Ref and exclude from ApiCompat comparisons (with ref, and previous platforms). IOW attributes which don't represent public surface nor design-time behavior. The latter is meant to indicate attributes only excluded from ApiCompat. These are attributes which don't represent public surface but are interesting for design-time behavior.

Handle if def for different tfms

We can't support this, we need those to be handled via exclusions and manual files, or even eliminating the old builds if we no longer need to build them.

100d constant shouldnt get converted to 100

Looks like we don't suffix the d if ToString doesn't contain a decimal .. https://github.com/dotnet/arcade/blob/d6dd7a000fe9908e4caae5eb9aa30ecc9cb917ff/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Attributes.cs#L281

default value getting removed param=default(type) => param

This looks like GenAPI was choosing a different base constructor to call

Extensions methods are not generated correctly , this keyword is skipped

I didn't see this, but we can debug in GenAPI to see what's going wrong.

[safern]

Nullablity is also not being intrepreted correctly.

I'd like to understand what do you mean by this? There is a couple of bugs open in arcade that affect the nullability generation, but nullability is interpreted correctly overall, maybe you hit one of those bugs?

[ericstj]

I discussed with @Anipik that I think he's hitting the explicit interface bug.

[safern]

Yeah, probably he's hitting it when generating PNSE assemby I guess?

What we could do as a workaround is to disable nullability for PNSE assemblies just as we do for netstandard and netfx.

[ericstj]

No @Anipik is working on making GenerateReferenceSource clean and is cataloging/fixing all the things preventing that.

[safern]

I see, but for PNSE the nullable annotations doesn't really matter and explicit interface are treated as non-public by the compiler so the annotations we put in ref assemblies are useless.

[ericstj]

This has nothing to do with PNSE: we simply can’t cleanly run GenerateRefenceSorce right now and this is one of the issues blocking that.

[akoeplinger]

We also just hit an issue with nullability in a PNSE generated source for System.Net.NetworkInformation in https://github.com/dotnet/runtime/pull/38928:

/Users/admin/dev/runtime/artifacts/obj/System.Net.NetworkInformation/net5.0-Browser-Release/System.Net.NetworkInformation.notsupported.cs(367,78):
error CS8618: Non-nullable field 'None' is uninitialized. Consider declaring the field as nullable. [/Users/admin/dev/runtime/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj]

from this code:

    public partial class PhysicalAddress
    {
        public static readonly System.Net.NetworkInformation.PhysicalAddress None;

I assume this would tie in with https://github.com/dotnet/arcade/issues/4616

[ericstj]

It's coincidentally related to #4616 in that it's possible the fix for that might fix this. What are we doing in the ref assembly? It looks like it has same issue: https://github.com/dotnet/runtime/blob/8145efbab6a37876e614902a44aa91ceab8613d9/src/libraries/System.Net.NetworkInformation/ref/System.Net.NetworkInformation.cs#L366 Why no error there?

[ericstj]

I see, we suppress it: https://github.com/dotnet/runtime/blob/0d1fd3d9d649b6aeae93b4f22b5776aa1d51dbc3/eng/referenceAssemblies.props#L11

[safern]

Yeah this is not a nullability error, this is basically matching the nullability declared on the ref but when you have a non-nullable field that is not initialized you get that warning.

[dagood]

Another nullability issue I think: https://github.com/dotnet/arcade/issues/6718. public static [NotNullIf...]T As<T>(...), not handling [return: ...] properly.

[deeprobin]

Another bug would be that nints are treated as IntPtr although the primitives must be used. The same applies to nuint and UIntPtr.

[deeprobin]

I would also suggest that the CI validates this referencesource in the future. I.e. that the order of the members, attributes, etc. are correct - Thus we avoid in the future larger diffs when generating the developer must sort out his changes separately.