Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize ref definitions #180

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Synchronize ref definitions #180

merged 3 commits into from
Sep 12, 2019

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Sep 6, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

Wraith2 commented Sep 7, 2019

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
Copy link
Contributor Author

Wraith2 commented Sep 7, 2019

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
Copy link
Contributor

David-Engel commented Sep 9, 2019

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:

  1. When would we want the ref definition to not match the implementation and why?
  2. 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
Copy link
Contributor Author

Wraith2 commented Sep 9, 2019

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,

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) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come throw null; was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Sep 11, 2019

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

Wraith2 commented Sep 11, 2019

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.

Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cheenamalhotra cheenamalhotra changed the title synchronize ref definitions Synchronize ref definitions Sep 12, 2019
@cheenamalhotra cheenamalhotra added this to the 1.1.0-preview1 milestone Sep 12, 2019
@cheenamalhotra cheenamalhotra merged commit 53d2128 into dotnet:master Sep 12, 2019
@cheenamalhotra 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
@cheenamalhotra
Copy link
Member

cheenamalhotra commented Sep 12, 2019

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

@Wraith2 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📍 Push to Commit Feed This label will be added to PRs whose NuGet Package will be available from CI Public Feed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants