Synchronize ref definitions #180

Wraith2 · 2019-09-06T22:01:34Z

This PR syncs the definitions of netcore netstandard and netfx ref projects where possible. This is mainly by reordering the types into but some attributes have been added to netcore where they were present in netfx since this isn't an interface change and enhances mainly designer experiences. It's now possible to diff the ref definitions and get a pretty clear picture what the differences in public surface are between the different versions.

David-Engel · 2019-09-07T00:12:02Z

I started looking at the changes and there are quite a few additions to the netcore ref file beyond the designer attributes.

SqlCommand.NotificationAutoEnlist
SqlConnectionStringBuilder.AsynchronousProcessing
- (Quite a few under here actually, that have not been brought to netcore.)
  I've run out of time to go through the rest. It's hard to tell what's been added through all the re-ordering. I'll try to continue on Monday.

Wraith2 · 2019-09-07T09:13:01Z

removed from SqlConnectionStringBuilder.ConnectionReset, ContextConnection, NetworkLibrary, TransparentNetworkIPResolution. removed SqlCommand.NotificationAutoEnlist.
I might see if I can do something with reflection if I can find the non-ref binaries.

Wraith2 · 2019-09-07T19:53:14Z

I write a program which dumps the definitions from a binary and then did some manual comparisons. interesting results overall.

net462-ref.txt
netcore-impl.txt
netcore-ref.txt
net462-impl.txt

David-Engel · 2019-09-09T21:39:10Z

The comparisons are definitely interesting. I'm having to do a lot of research for this PR. Learning new things is not bad. Some questions in my mind are:

When would we want the ref definition to not match the implementation and why?
What exactly do the various designer attributes do? (I'm new to these so reading a lot but the MS docs tend to be sparse.) Is it safe to add them or is there anything that needs to be verified about methods for which we are adding those attributes?

We don't have any tests around the ref definition, so verifying this PR is all manual. It might be easier if this were split into different PRs where we can easily accept a round of reformatting/white space changes, a round of only sorting changes (no lines added or changed), and then a round of actual changes where we can ask questions about specific changes. Like why has Microsoft.Data.SqlClient. been prepended to a reference? (I'd prefer to remove it from netfx if it's just for syncing.) Or why has throw null; been added or removed? Or how is it that SqlDataReader.Close() did not already exist in ref and no one noticed until now? (I guess everyone is just wrapping it in a using or calling Dispose().)

Wraith2 · 2019-09-09T22:26:20Z

When would we want the ref definition to not match the implementation and why?

From my understanding they should never. They can differ with varying consequences, least damaging is the metadata. The only purpose of the ref assemblies is to be bound against by compilers when the exact TFM+RID version has not or can not be determined. If something exists in ref but not impl then an attempt to use it will cause a runtime failure. If something exists in impl but not ref then it may or may not be visible to the compiler which could cause editor/intellisense visibility strangeness or compiler errors complaining about missing types.

The attributes I took over are all the ones that didn't need any additional references. The Security attributes are irrelevant on netcore, security is stubbed. The rest were componentmodel designer metadata which influences the propertygrid and other visual studio designer experiences, so not needed but having them present will make things feel more natural to people who use visual designers. HostProtectionAttribute was the only one that I didn't attempt to sync, that doesn't exist and won't on netcore because the reliability infrastructure it's part of is not in that runtime.

Like why has Microsoft.Data.SqlClient. been prepended to a reference?

Any type which has a language keyword can use that language keyword. Anything which does not should use a fully qualified name. My assumption for this is that it prevents there being any possibly namespace mistakes by virtue of there being no using directives at all, everything must be explicit about the exact type. The ref assemblies can be tool generated in corefx which is why everything is strictly in alphanumeric order.

Or why has throw null; been added or removed?

if an attempt is made to use the ref assembly as an impl is made it should fail as soon as possible. throwing null is the pattern used throughout the other ref assemblies for this so if it was missing I added it.

Or how is it that SqlDataReader.Close() did not already exist in ref and no one noticed until now? (I guess everyone is just wrapping it in a using or calling Dispose().)

That's what I assume. Since Close is inherited from DbDataReader it will have been present in the intellisense from the base type so even if you looked for it you'd find it and the callvirt would mean you called the right one. Not a damaging oversight. It does make it clear that the existing ref in corefx wasn't tool generated.

I can rework from the beginning but I'm not sure it'll be less confusing to do so. The process logically involves diffing the files and making repeated rounds of modifications to limit the diff output. interim points in the process have no relative value,

David-Engel · 2019-09-09T23:40:35Z

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

        public event Microsoft.Data.SqlClient.SqlInfoMessageEventHandler InfoMessage { add { } remove { } }
        protected override System.Data.Common.DbTransaction BeginDbTransaction(System.Data.IsolationLevel isolationLevel) { throw null; }
        public new Microsoft.Data.SqlClient.SqlTransaction BeginTransaction() { throw null; }
        public new Microsoft.Data.SqlClient.SqlTransaction BeginTransaction(System.Data.IsolationLevel iso) { throw null; }
        public Microsoft.Data.SqlClient.SqlTransaction BeginTransaction(System.Data.IsolationLevel iso, string transactionName) { throw null; }
        public Microsoft.Data.SqlClient.SqlTransaction BeginTransaction(string transactionName) { throw null; }
        public override void ChangeDatabase(string database) { }
+        public static void ChangePassword(string connectionString, Microsoft.Data.SqlClient.SqlCredential credential, System.Security.SecureString newSecurePassword) { }


How come throw null; was removed?

Not sure. There were a bunch of other methods near it that didn't throw either so I've added it to a few. I'm going to see if can track down the exact rules for this since my knowledge is based on seeing ref edits and review feedback on my own.

I am a bit confused, too, why some include it and some don't. So I'm curious what you find out. I don't feel like it's a blocker, though, given my understand of ref.

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

Wraith2 · 2019-09-11T22:56:30Z

SqlFacetAttribute is in the ref but not impl. Do you want me to copy the definition from netfx into netcore? It's a surface area compatibility class since it can't possibly be useful in netcore.

David-Engel · 2019-09-11T23:08:39Z

SqlFacetAttribute is in the ref but not impl. Do you want me to copy the definition from netfx into netcore? It's a surface area compatibility class since it can't possibly be useful in netcore.

I don't see an impl in netfx. I think SqlFacetAttribute probably was meant to be removed from ref entirely when we resolved the Microsoft.SqlServer.Server namespace issue. We removed that namespace entirely and moved SqlDataRecord + dependent classes into Microsoft.Data.SqlClient.Server since SqlDataRecord seemed to be the main class used by users (we can easily bring others in if we are wrong about usage but it's hard to remove stuff).

David-Engel · 2019-09-11T23:23:31Z

@Wraith2 I think this is good to merge as-is. If you want to add the SqlFacetAttribute change to this PR, let me know. Otherwise, I can approve it.

Wraith2 · 2019-09-11T23:34:52Z

If you're happy with the current state I'd say merge it. I can come back to null throwing and the SqlFacetAttribute later in a dedicated PR if they need changes.

David-Engel

@cheenamalhotra This LGTM. Tagging you for a second set of eyes.

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs

cheenamalhotra · 2019-09-12T20:56:03Z

NuGet package available for this PR here: 1.1.0-build.19255.1-53d2128
(Refer here for details)

Wraith2 force-pushed the reformat branch from fd8cee5 to 5914817 Compare September 6, 2019 22:03

David-Engel requested changes Sep 9, 2019

View reviewed changes

synchronize ref definitions

dbb70d0

Wraith2 force-pushed the reformat branch from 66ef1a1 to dbb70d0 Compare September 11, 2019 22:52

David-Engel requested a review from cheenamalhotra September 11, 2019 23:49

David-Engel approved these changes Sep 11, 2019

View reviewed changes

cheenamalhotra reviewed Sep 12, 2019

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs Outdated Show resolved Hide resolved

Wraith2 added 2 commits September 12, 2019 18:08

autoformat document

d9f36ab

autoformat document

63de3ad

cheenamalhotra changed the title ~~synchronize ref definitions~~ Synchronize ref definitions Sep 12, 2019

cheenamalhotra added this to the 1.1.0-preview1 milestone Sep 12, 2019

cheenamalhotra approved these changes Sep 12, 2019

View reviewed changes

cheenamalhotra merged commit 53d2128 into dotnet:master Sep 12, 2019

cheenamalhotra added the 📍 Push to Commit Feed This label will be added to PRs whose NuGet Package will be available from CI Public Feed. label Sep 12, 2019

Wraith2 deleted the reformat branch September 13, 2019 22:02

yukiwongky pushed a commit to yukiwongky/SqlClient that referenced this pull request Nov 5, 2019

Synchronize ref definitions (dotnet#180)

913c1ae

initram mentioned this pull request Sep 22, 2020

System.Data.SqlClient clarify SqlDataReader get char #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize ref definitions #180

Synchronize ref definitions #180

Wraith2 commented Sep 6, 2019

David-Engel commented Sep 7, 2019

Wraith2 commented Sep 7, 2019

Wraith2 commented Sep 7, 2019

David-Engel commented Sep 9, 2019 •

edited

Loading

Wraith2 commented Sep 9, 2019 •

edited

Loading

David-Engel Sep 9, 2019

Wraith2 Sep 11, 2019

David-Engel Sep 11, 2019

Wraith2 commented Sep 11, 2019

David-Engel commented Sep 11, 2019

David-Engel commented Sep 11, 2019

Wraith2 commented Sep 11, 2019

David-Engel left a comment

cheenamalhotra commented Sep 12, 2019 •

edited

Loading

Synchronize ref definitions #180

Synchronize ref definitions #180

Conversation

Wraith2 commented Sep 6, 2019

David-Engel commented Sep 7, 2019

Wraith2 commented Sep 7, 2019

Wraith2 commented Sep 7, 2019

David-Engel commented Sep 9, 2019 • edited Loading

Wraith2 commented Sep 9, 2019 • edited Loading

David-Engel Sep 9, 2019

Choose a reason for hiding this comment

Wraith2 Sep 11, 2019

Choose a reason for hiding this comment

David-Engel Sep 11, 2019

Choose a reason for hiding this comment

Wraith2 commented Sep 11, 2019

David-Engel commented Sep 11, 2019

David-Engel commented Sep 11, 2019

Wraith2 commented Sep 11, 2019

David-Engel left a comment

Choose a reason for hiding this comment

cheenamalhotra commented Sep 12, 2019 • edited Loading

David-Engel commented Sep 9, 2019 •

edited

Loading

Wraith2 commented Sep 9, 2019 •

edited

Loading

cheenamalhotra commented Sep 12, 2019 •

edited

Loading