Auto-generate the reference assemblies to normalize them #39681
Conversation
explicit interface methods.
| @@ -113,7 +113,7 @@ public partial class ConcurrentDictionary<TKey, TValue> : System.Collections.Gen | |||
| void System.Collections.Generic.IDictionary<TKey, TValue>.Add(TKey key, TValue value) { } | |||
| bool System.Collections.Generic.IDictionary<TKey, TValue>.Remove(TKey key) { throw null; } | |||
| void System.Collections.ICollection.CopyTo(System.Array array, int index) { } | |||
| void System.Collections.IDictionary.Add(object key, object? value) { } | |||
| void System.Collections.IDictionary.Add(object key, object value) { } | |||
There was a problem hiding this comment.
@safern, is this expected?
It don't think it actually matters in such an explicit interface case, because no one will call via this signature, but... it's a little strange to be undoing it.
There was a problem hiding this comment.
I mentioned this in the original PR comment. I talked to @safern about this, and his change to GenAPI (to make it nullable aware, which I used here - dotnet/arcade#2679) removes the nullablility annotations on explicit interface implementations.
There was a problem hiding this comment.
The reason why it removes it is because we’re no longer preserving nullable metadata on non-public members, since explicit interfaces are non-public the compiler drops them, so we don’t have a way of knowing it’s nullability in GenAPI. I don’t love the idea but i can’t think of a way to preserve its nullability but by directly using the information in the interface declaration itself.
There was a problem hiding this comment.
Just to highlight explicitly, this change does cause a compiler warning which we are disabling: CS8617
cc @AlekseyTs, @gafter
|
I'm curious, what process do you use to generate this... do you have a script that iterates through all of the project directories and builds with /t:GenerateReferenceSource, or is there a top-level build command for doing that? |
| public const float PositiveInfinity = (float)1.0 / (float)0.0; | ||
| public const float NegativeInfinity = (float)-1.0 / (float)0.0; | ||
| public const float NaN = (float)0.0 / (float)0.0; | ||
| public const float Epsilon = 1.4E-45f; |
There was a problem hiding this comment.
@tannergooding - for some reason the auto-gen changes the value to 1.401298E-45f.
Similarly, the other values (MaxValue/MinValue) show up incorrectly as well. I kept the values that already exist but not sure what the intention is here:
public const float MaxValue = 3.40282347E+38f;
public const float MinValue = -3.40282347E+38f;
There was a problem hiding this comment.
I'm guessing because the improved algorithm identifies the shorter forms as bit-equivalent in the floating point representation, and more desirable as they're shorter.
There was a problem hiding this comment.
The current ref isn't shorter in all cases, and what GenAPI is proposing isn't either.
Current (and what's in master/this PR):
public const float Epsilon = 1.4E-45f;
public const float MaxValue = 3.40282346638528859E+38f;What GenAPI was suggesting changing this to:
public const float Epsilon = 1.401298E-45f; // this is a longer form
public const float MaxValue = 3.40282347E+38f; // this is a shorter formThere was a problem hiding this comment.
Ah, @tannergooding would have the explain that.
src/System.Collections.Immutable/ref/System.Collections.Immutable.cs
Outdated
Show resolved
Hide resolved
Basically yes, but its not a script (mainly because I am not super familiar with the how msbuild targets work, particularly in corefx). I wrote a C# tool that does this (enumerate through all ref directories and run |
|
Failing CI leg seems unrelated - corefx-ci (Windows x64_Debug): |
|
@stephentoub, do you think its worth porting this change to the release/3.0 branch to minimize diffs in the future (if someone was to modify the ref)? I don't see a great reason other than general clean up. |
I don't think so. We generally see very few, if not zero, changes to refs as part of servicing, and there's enough churn here that it's not clear how low the risk is of an unintentional break. But, I'll let @danmosemsft comment if he disagrees. If he wants to take it, that's fine. |
|
Is there any way this would significantly impact customers using 3.0 if we didn't? |
If the change has the effects it hopes to: There's no impact to customers either way. (Hence, I'd say we don't want it, we never touch the ref.cs files in servicing) |
OK. I had a quick eyeball for renamed parameter or added API and I didn't see any. |
readonly on private fields).
|
Updating System.Runtime ref as well (filed an issue for the one outlier: dotnet/arcade#3368) related to ValueTuple that had to be manually reverted back. |
| @@ -33,6 +33,7 @@ public partial struct Single | |||
| // We need to add this into the manual ref assembly to preserve it because the | |||
| // implementation doesn't have any reference field, hence GenApi will not emit it. | |||
| private object _dummy; | |||
| // Placing the value type field in the manual ref as well to avoid the error CS0282: There is no defined ordering between fields in multiple declarations of partial struct 'TypedReference'. | |||
| private int _dummyPrimitive; | |||
There was a problem hiding this comment.
Since the value type field does get generated by GenAPI (for the IntPtr fields), wanted to add an explicit comment to highlight why we want to keep that in the manual file instead.
|
Hello @ahsonkhan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
| public const FileAttribute vbSystem = FileAttribute.System; | ||
| public const MsgBoxStyle vbSystemModal = MsgBoxStyle.SystemModal; | ||
| public const Microsoft.VisualBasic.VariantType vbObject = Microsoft.VisualBasic.VariantType.Object; | ||
| public const int vbObjectError = -2147221504; |
| [System.ObsoleteAttribute("This member has been deprecated. Please use FilePutObject to write Object types, or coerce FileNumber and RecordNumber to Integer for writing non-Object types. http://go.microsoft.com/fwlink/?linkid=14202")] | ||
| public static void FilePut(object FileNumber, object Value, object RecordNumber/* = -1*/) { } |
There was a problem hiding this comment.
Note, we are losing this comment. Maybe it would be worth moving this to a .Manual file (possibly outside the PR).
ghost
merged commit 
…fx#39681) * Update Microsoft.VisualBasic.Core ref * Update System.Collections.Concurrent ref * Update System.Collections.Immutable ref * Update System.Collections ref * Update System.ComponentModel.Composition ref * Update System.Data.Common ref * Update System.Data.OleDb ref * Update System.Data.SqlClient ref for netcoreapp * Update System.Diagnostics.DiagnosticSource ref and DiagnosticSourceActivity.cs * Update System.IO.Packaging ref for netcoreapp * Update System.IO.Pipelines ref * Update System.Linq ref * Update System.Net.Http ref * Update System.Net.Security ref * Update System.Numerics.Tensors ref * Update System.Reflection.Emit for netcore * Update System.Security.AccessControl ref * Update System.Security.Permissions ref * Update System.ServiceModel.Syndication ref * Update System.Text.Encodings.Web ref * Update System.Threading.Channels ref for netcoreapp * Update System.Utf8String.Experimental ref * Update System.Utf8String.Experimental csproj to reference new file. * Upat eSystem.Runtime ref and Manual ref * Update Microsoft.Bcl.AsyncInterfaces ref * Disable warning CS8617 for missing nullability annotations within explicit interface methods. * Revert System.Runtime.cs back to what's in master. * Update System.Runtime ref (single change - System.Char -> char). * Revert System.Collections.Immutable back to what's in master (keep readonly on private fields). * Update System.Runtime ref and add comment in Manual ref file for TypedReference. Commit migrated from dotnet/corefx@bdd0814
…fx#39681) * Update Microsoft.VisualBasic.Core ref * Update System.Collections.Concurrent ref * Update System.Collections.Immutable ref * Update System.Collections ref * Update System.ComponentModel.Composition ref * Update System.Data.Common ref * Update System.Data.OleDb ref * Update System.Data.SqlClient ref for netcoreapp * Update System.Diagnostics.DiagnosticSource ref and DiagnosticSourceActivity.cs * Update System.IO.Packaging ref for netcoreapp * Update System.IO.Pipelines ref * Update System.Linq ref * Update System.Net.Http ref * Update System.Net.Security ref * Update System.Numerics.Tensors ref * Update System.Reflection.Emit for netcore * Update System.Security.AccessControl ref * Update System.Security.Permissions ref * Update System.ServiceModel.Syndication ref * Update System.Text.Encodings.Web ref * Update System.Threading.Channels ref for netcoreapp * Update System.Utf8String.Experimental ref * Update System.Utf8String.Experimental csproj to reference new file. * Upat eSystem.Runtime ref and Manual ref * Update Microsoft.Bcl.AsyncInterfaces ref * Disable warning CS8617 for missing nullability annotations within explicit interface methods. * Revert System.Runtime.cs back to what's in master. * Update System.Runtime ref (single change - System.Char -> char). * Revert System.Collections.Immutable back to what's in master (keep readonly on private fields). * Update System.Runtime ref and add comment in Manual ref file for TypedReference. Commit migrated from dotnet/corefx@bdd0814
Auto-generated the reference assemblies using
/t:GenerateReferenceSourceon all projects that already contained a/ref/directory.https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/updating-ref-source.md
Utf8Stringref specifically).System.Runtimeas is for now other than a cosmetic change.Partially addresses https://github.com/dotnet/corefx/issues/29737
There is no additional/removal of APIs in this change. The diff between this PR and master is empty.