Auto-generate the reference assemblies and update potentially missing APIs in the ref #35474

ahsonkhan · 2019-02-21T01:11:29Z

The goal of this PR to establish a consistent baseline of the reference assembly using /t:GenerateReferenceSource on the ref assemblies along with the source.
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/updating-ref-source.md

Sorting alphabetically to be consistent.
Add dummy fields where needed.
Make sure source/ref assemblies public surface area matches.
Make sure we correctly expose the newly added public APIs.
Remove using directives and fully qualify the types.

Please review the specific area that you own and let me know if something is incorrect:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/issue-guide.md

The goal is to avoid any accidental breaking changes to the ref assemblies.

Partially addresses https://github.com/dotnet/corefx/issues/29737

cc @dotnet/dotnet-corefx, @danmosemsft, @karelz, @joshfree, @jkotas, @terrajobst, @joperezr, @ViktorHofer, @weshaggard

attributes

netcoreapp ref

instead

bartonjs · 2019-02-21T15:49:18Z

FWIW, since this isn't just a "ran a tool, everything is happy" I wonder if breaking it up into smaller PRs would be better (there's no particular interdependency across assembly boundaries).

danmoseley · 2019-02-21T17:08:43Z

Also related: https://github.com/dotnet/corefx/issues/33117 (some libraries missing ref assemblies)

danmoseley · 2019-02-21T17:09:00Z

Thansk for doing this @ahsonkhan

davidsh · 2019-02-21T17:33:37Z

src/System.Net.Http/ref/System.Net.Http.cs

-        public NameValueWithParametersHeaderValue(string name, string value) : base(default(string)) { }
+        protected NameValueWithParametersHeaderValue(System.Net.Http.Headers.NameValueWithParametersHeaderValue source) : base (default(System.Net.Http.Headers.NameValueHeaderValue)) { }
+        public NameValueWithParametersHeaderValue(string name) : base (default(System.Net.Http.Headers.NameValueHeaderValue)) { }
+        public NameValueWithParametersHeaderValue(string name, string value) : base (default(System.Net.Http.Headers.NameValueHeaderValue)) { }


These two overloads are wrong now. They should be still using 'string' passing into base().

The base-ctor call isn't actually part of the metadata; the generator just finds something on the base class that it can legally call to make the ref.cs compile.

davidsh · 2019-02-21T17:35:16Z

src/System.Net.HttpListener/ref/System.Net.HttpListener.cs

    }
    public partial class HttpListenerBasicIdentity : System.Security.Principal.GenericIdentity
    {
-        public HttpListenerBasicIdentity(string username, string password) : base(default(string)) { }
+        public HttpListenerBasicIdentity(string username, string password) : base (default(System.Security.Principal.GenericIdentity)) { }


This doesn't look right. Why is it changing from 'string'?

ericstj · 2019-02-21T17:35:19Z

Can you share some guidance on what type of testing I need to do for API compat that would highlight breaking changes that might have been introduced more cleanly?

Here are all the interesting APICompat runs that I would expect for a change like this.

Refs prior to change and after your change.
Refs from last release and after your change.
Refs for netstandard and after your change (should be part of the build as ApiCompat.proj).
Desktop and refs before your change (to establish a baseline) and after your change. Diff any new failures and make sure they are intentional.
References from packages before your your change and after.

I specifically targeted and focused on netcoreapp and then manually reverted the if/def changes for other TFMs.

In that case you need to be very careful about any additions to CS files as these could be claiming API in a netstandard ref for which we may no longer be building implementation. Hopefully we've turned most of these off, but I don't know for certain if that is the case.

How did you deal with baselines for intentionally excluded types?

Can you give me an example of such a type?

Have a look at this: https://github.com/dotnet/corefx/search?q=filename%3AMatchingRefApiCompatBaseline&unscoped_q=filename%3AMatchingRefApiCompatBaseline

Do you have any suggestions? Is this something we can run in CI whenever a new API is added?

GenAPI is configurable. For anything that requires manual intervention we could configure it to be excluded from GenAPI then handle it with manual partial source files. For example: types that need to be if-def'ed, exclude them from the main CS and put them in the manual partial source file. If we wanted to we could make it easy to generate the manual files by providing short cut targets that treat exclusions as inclusions and write to a user provided filename. For anything that's baselined (intentionally omitted) we could pass that both to APICompat (as it is today) and GenAPI by changing the format of baselines. I belive we could pass the same format of exclusion lists to both GenAPI and APICompat.

davidsh · 2019-02-21T17:36:19Z

src/System.Net.Mail/ref/System.Net.Mail.cs

+        public AlternateView(System.IO.Stream contentStream, string mediaType) : base (default(System.IO.Stream)) { }
+        public AlternateView(string fileName) : base (default(System.IO.Stream)) { }
+        public AlternateView(string fileName, System.Net.Mime.ContentType contentType) : base (default(System.IO.Stream)) { }
+        public AlternateView(string fileName, string mediaType) : base (default(System.IO.Stream)) { }


Why is the type changing from 'string' to 'System.IO.Stream' here? This now doesn't match the actual implementation.

davidsh · 2019-02-21T17:36:28Z

src/System.Net.Mail/ref/System.Net.Mail.cs

+        public Attachment(System.IO.Stream contentStream, string name, string mediaType) : base (default(System.IO.Stream)) { }
+        public Attachment(string fileName) : base (default(System.IO.Stream)) { }
+        public Attachment(string fileName, System.Net.Mime.ContentType contentType) : base (default(System.IO.Stream)) { }
+        public Attachment(string fileName, string mediaType) : base (default(System.IO.Stream)) { }


Why is the type changing from 'string' to 'System.IO.Stream' here? This now doesn't match the actual implementation.

davidsh · 2019-02-21T17:36:35Z

src/System.Net.Mail/ref/System.Net.Mail.cs

+        public LinkedResource(System.IO.Stream contentStream, string mediaType) : base (default(System.IO.Stream)) { }
+        public LinkedResource(string fileName) : base (default(System.IO.Stream)) { }
+        public LinkedResource(string fileName, System.Net.Mime.ContentType contentType) : base (default(System.IO.Stream)) { }
+        public LinkedResource(string fileName, string mediaType) : base (default(System.IO.Stream)) { }


Why is the type changing from 'string' to 'System.IO.Stream' here? This now doesn't match the actual implementation.

davidsh · 2019-02-21T17:48:02Z

src/System.Net.Requests/ref/System.Net.Requests.cs

    public partial class HttpWebRequest : System.Net.WebRequest, System.Runtime.Serialization.ISerializable
    {
-        internal HttpWebRequest() { }
+        public HttpWebRequest() { }


In .NET Framework, we have this:

//introduced for supporting design-time loading of System.Windows.dll [Obsolete("This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.", true)] [EditorBrowsable(EditorBrowsableState.Never)] public HttpWebRequest(){}

src/System.Net.Requests/ref/System.Net.Requests.cs

333fred · 2019-02-21T18:04:00Z

src/Microsoft.VisualBasic.Core/ref/Microsoft.VisualBasic.Core.cs

@@ -14,24 +14,26 @@ public enum CallType
        Method = 1,
        Set = 8,
    }
-    public sealed partial class Collection : System.Collections.ICollection, System.Collections.IList
+    public sealed partial class Collection : System.Collections.ICollection, System.Collections.IEnumerable, System.Collections.IList


System.Collections.IEnumerable [](start = 77, length = 30)

Why was this added?

Looks like the public documentation states that this was supposed to be implementing it, I'm assuming it was just missing from the original ref assembly?

In reply to: 259048143 [](ancestors = 259048143)

333fred · 2019-02-21T18:12:36Z

src/Microsoft.VisualBasic.Core/ref/Microsoft.VisualBasic.Core.cs

    {
        public Collection() { }
        public int Count { get { throw null; } }
+        public object this[int Index] { get { throw null; } }
+        [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(2))]


(2) [](start = 100, length = 3)

Nit: why was the System.AttributeTargets below fixed to use the actual enum value and this wasn't?

333fred · 2019-02-21T18:18:56Z

src/Microsoft.VisualBasic.Core/ref/Microsoft.VisualBasic.Core.cs

+        internal BooleanType() { }
+        public static bool FromObject(object Value) { throw null; }
+        public static bool FromString(string Value) { throw null; }
+    }
    public sealed partial class Conversions


This is now missing the EditorBrowsableState attribute.

333fred · 2019-02-21T18:21:26Z

src/Microsoft.VisualBasic.Core/ref/Microsoft.VisualBasic.Core.cs

@@ -645,20 +643,20 @@ public partial class TextFieldParser : System.IDisposable
        public string[] ReadFields() { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)]
        public string ReadLine() { throw null; }
-        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)]
+        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)]	


[](start = 109, length = 1)

Nit: trailing whitespace added.

vancem · 2019-02-21T18:44:56Z

I have looked over the DiagnosticSource types (which are a bit unusual in how they do their reference assemblies), as well as the EventCounter types, and they look OK.

davidsh · 2019-02-21T18:56:20Z

I reviewed all System.Net.* changes. I add some comments. Some of the changes look weird to me (changing types in 'ref' *.CS files) in the base() calls. But @bartonjs says it is just a tooling weirdness and doesn't actually change anything.

ahsonkhan · 2019-02-21T21:47:17Z

I wonder if breaking it up into smaller PRs would be better (there's no particular interdependency across assembly boundaries).

I saw value in a central update where common update patterns can be reviewed/highlighted since most changes were auto-generated. For example, what attributes must be retained, how base class definitions come up, dummy primitives/etc., are common changes across all ref assemblies and it would be redundant to review separately. I don't mind splitting it up but I don't think opening 50+ PR (per 1 file or area) would be reasonable. How would you suggest I break it up? Would 5 or so PRs with arbitrary break up points be preferred (say split across alphabetical boundary)?

Essentially, my question is, what's making reviewing a single PR, difficult? Is it just the size/number of files, or is that there is a lot of change that isn't relevant to each area-owner? If 20-25 file changes/1-2k line PRs is reasonable (rather than 100+ files), I can split it into ~5 PRs. If, however, we want per area, then that would be quite a few PRs (30+). If we want per-area PRs, I would love to hear suggestions on how to auto-create/submit PRs because I, personally, would prefer not to do this work manually for all the ref assemblies.

Alternatively, each area owner could run the tool and audit the changes themselves, but I would prefer to leverage the work already done here :)

danmoseley · 2019-02-22T03:45:39Z

Perhaps this was already mentioned (I didn't read all the comments) but did you do a diff of the "post compilation" result? Eg., with API reviewer. That would eliminate 100% of the reordering showing only any actual adds/removes.

I'm sure it is not perfect as each of those tools probably have idiosyncracies around attributes they show, etc.

GrabYourPitchforks · 2019-02-22T06:02:00Z

Agree with @danmosemsft. Seeing a diff with the API Reviewer tool would give us (the reviewers) confidence that things are good. Right now there's a significant amount of noise which is adding to the difficulty of performing the review.

ahsonkhan · 2019-02-22T08:48:08Z

Refs prior to change and after your change.

did you do a diff of the "post compilation" result? Eg., with API reviewer. That would eliminate 100% of the reordering showing only any actual adds/removes.

Seeing a diff with the API Reviewer tool would give us (the reviewers) confidence that things are good.

Thanks for the suggestion. Here is the API diff between master and this PR. It highlights the additions/removals more cleanly so should be easier to review. We need to decide if any of these changes we want to avoid making and revert:
https://gist.github.com/ahsonkhan/9172948674eb7fb8d98e84438509b9b0

davidsh · 2019-02-22T17:53:51Z

Thanks for the suggestion. Here is the API diff between master and this PR. It highlights the additions/removals more cleanly so should be easier to review. We need to decide if any of these changes we want to avoid making and revert:
https://gist.github.com/ahsonkhan/9172948674eb7fb8d98e84438509b9b0

System.Net.* changes look good.

danmoseley · 2019-02-22T18:20:32Z

Thanks @ahsonkhan that makes things much clearer - assuming the diff isn't suppressing anything like attributes then perhaps it's the only thing that really needs reviewing by area owners.

Where new API is showing up there, it potentially does not have unit tests because most of our test projects reference ref, not src. In a quick check I don't see obvious holes though. Eg the CallSiteOps class looks like it's tested in S.Linq.Expressions, however (at least some of those methods are).

jkotas · 2019-02-23T16:40:01Z

I think it would be best to split this into multiple PRs:

One big PR that just fixes the formatting and that is verified to be zero-diff in generated binaries. This PR does not need reviewing.
File issues on the problems identified to follow up. These can be done in smaller PRs per-area or done by the area owners if they are more difficult and need unit tests, etc.

jkotas · 2019-02-23T16:42:18Z

Essentially, my question is, what's making reviewing a single PR, difficult?

99% of the delta is just noise that does not need any reviewing. It is very hard to pick the 1% that matters and not miss anything.

ahsonkhan · 2019-02-25T10:55:20Z

One big PR that just fixes the formatting and that is verified to be zero-diff in generated binaries. This PR does not need reviewing.

#35557

jkotas · 2019-03-05T21:23:32Z

This has massive merge conflict now that the mechanical changes have been merged. @ahsonkhan Could you please open a new PR with the rest if you plan to do more work on this?

ahsonkhan added 30 commits February 15, 2019 00:38

Auto-generate the reference assemblies

012a461

Remove empty ref assemblies that are completely type forwarded

43a9012

Rename ref cs files to match the ref assembly names and add back

62234ce

attributes

Update CoreFx.Private.TestUtilities to add back attributes and comments

a70940b

Add back attributes to Microsoft.CSharp

e4dd41b

Add back type forwards for M.Win32.Registry.AccessControl

ac95280

Add back attributes to M.Win32.SystemEvents

d50f508

Add back obsolete and compiler generated attributes to System.CodeDom

963359e

Add attributes and ifdefs to S.Collections.Immutable.

3ab5085

Add type forwards to S.Collection.NonGenerics

dcefe24

Add obsolete attributes to S.Collections.Specialized

b3445a5

Add back the obsolete attribute to S.ComponentModel.Annotations

d3b3aff

Add back type forward and attributes to S.ComponentModel.Composition

b622217

Add back attributes to S.ComponentModel.EventBasedAsync

477dbb4

Add attribute to S.ComponentModel.Primitives.

8d5e658

Add back type forwards, attributes, and remove duplicate APIs

d25448b

Add type forwards and attributes to S.Data.Common

1bc559d

Add attributes to S.Data.Odbc

9adf5bc

Add typeforwards to S.Data.SqlClient and remove duplicates from

0a9bf6a

netcoreapp ref

Revert changes to S.Diag.DiagSource and modify DiagSourceActivity

4973729

instead

Add attributes to S.Diagnostics.Eventlog

ad2ef98

Add attributes to S.Diag.PerfCounters

6050147

Add back obsolete attributes to S.Diag.Process

bbdbcd4

More attributes to Diag.SackTrace/TraceSource/Tracing

fc4edcf

Add back attributes and ifdefs

a3dbcda

Add leftover attributes, comments, and merge netcoreapp ref file back

bf3b035

Update System.Runtime

41141a5

Update System.Runtime.Extensions

ff9b302

Fix build breaks for .NET Core App

d39560a

Add back CodeDom CompilerGeneratedAttribute except for one (Size)

8b08ef7

davidsh reviewed Feb 21, 2019

View reviewed changes

src/System.Net.Requests/ref/System.Net.Requests.cs Show resolved Hide resolved

333fred reviewed Feb 21, 2019

View reviewed changes

ahsonkhan mentioned this pull request Feb 25, 2019

Auto-generate the reference assemblies and separate type forwards #35557

Merged

jkotas closed this Mar 5, 2019

ahsonkhan mentioned this pull request Jun 12, 2019

Re-generate System.Collections.Immutable ref assembly and fix a param name #38264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-generate the reference assemblies and update potentially missing APIs in the ref #35474

Auto-generate the reference assemblies and update potentially missing APIs in the ref #35474

ahsonkhan commented Feb 21, 2019 •

edited

Loading

bartonjs commented Feb 21, 2019

danmoseley commented Feb 21, 2019

danmoseley commented Feb 21, 2019

davidsh Feb 21, 2019

bartonjs Feb 21, 2019

davidsh Feb 21, 2019

ericstj commented Feb 21, 2019

davidsh Feb 21, 2019

davidsh Feb 21, 2019

davidsh Feb 21, 2019

davidsh Feb 21, 2019

333fred Feb 21, 2019

333fred Feb 21, 2019

333fred Feb 21, 2019

333fred Feb 21, 2019

333fred Feb 21, 2019

vancem commented Feb 21, 2019

davidsh commented Feb 21, 2019

ahsonkhan commented Feb 21, 2019

danmoseley commented Feb 22, 2019

GrabYourPitchforks commented Feb 22, 2019

ahsonkhan commented Feb 22, 2019

davidsh commented Feb 22, 2019

danmoseley commented Feb 22, 2019

jkotas commented Feb 23, 2019

jkotas commented Feb 23, 2019 •

edited

Loading

ahsonkhan commented Feb 25, 2019

jkotas commented Mar 5, 2019

Auto-generate the reference assemblies and update potentially missing APIs in the ref #35474

Auto-generate the reference assemblies and update potentially missing APIs in the ref #35474

Conversation

ahsonkhan commented Feb 21, 2019 • edited Loading

bartonjs commented Feb 21, 2019

danmoseley commented Feb 21, 2019

danmoseley commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancem commented Feb 21, 2019

davidsh commented Feb 21, 2019

ahsonkhan commented Feb 21, 2019

danmoseley commented Feb 22, 2019

GrabYourPitchforks commented Feb 22, 2019

ahsonkhan commented Feb 22, 2019

davidsh commented Feb 22, 2019

danmoseley commented Feb 22, 2019

jkotas commented Feb 23, 2019

jkotas commented Feb 23, 2019 • edited Loading

ahsonkhan commented Feb 25, 2019

jkotas commented Mar 5, 2019

ahsonkhan commented Feb 21, 2019 •

edited

Loading

jkotas commented Feb 23, 2019 •

edited

Loading