Correcting Mismatch between ref and src #39741

Anipik · 2020-07-21T23:00:10Z

working towards #26187
Changing src to match ref.

In order to change ref we need api approval.
Some small genapi related re ordering of members.

cc @buyaa-n @safern @tannergooding @carlossanlop @dotnet/ncl @tarekgh

Dotnet-GitSync-Bot · 2020-07-21T23:00:13Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-07-21T23:00:16Z

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

ericstj · 2020-07-22T00:46:50Z

src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs

@@ -226,9 +226,9 @@ public static bool TryParse([NotNullWhen(true)] string? ipString, [NotNullWhen(t
            return (address != null);
        }

-        public static bool TryParse(ReadOnlySpan<char> ipSpan, [NotNullWhen(true)] out IPAddress? address)
+        public static bool TryParse(ReadOnlySpan<char> ipString, [NotNullWhen(true)] out IPAddress? address)


@dotnet/ncl this looks like less desirable than what was previously here. If you intended to change the reference assembly prameter name, take that through API review and file a breaking change issue.

Original API proposal: #22918
public static bool TryParse(ReadOnlySpan<char> ipChars, out IPAddress address);

According to the original issue, it should be ipChars. The code is quite old (merged 3 years ago on Aug 22, 2017). @karelz is parameter name change breaking change? If not, should we align it with the original API proposal?

The publicly documented API is ipString https://docs.microsoft.com/en-us/dotnet/api/system.net.ipaddress.tryparse?view=netcore-3.1#System_Net_IPAddress_TryParse_System_ReadOnlySpan_System_Char__System_Net_IPAddress__

It's also what we've shipped since 2.1 dotnet/corefx@0029b32#diff-08121b9ae82947c9c21c6bb7c18202a4R223

is parameter name change breaking change?

It's source breaking when source uses named parameters. EG:

ReadOnlySpan<char> ip = "1.1.1.1"; IPAddress res; IPAddress.TryParse(ipString: ip, out res);

If you were to correct the ref, this source would break.

Technically changing implementation is also observable, through reflection, but we typically prioritize avoiding source breaking changes on these issues.

@dotnet/ncl please weigh in on what you'd like to do here.

cc: @karelz @scalablecory @wfurt

We should keep the same name we had in previous release 3.1 IMO ... at this stage of 5.0, I would avoid discussions about changing param name.
How was the discrepancy introduced?

In the original PR, just overlooked when doing review I guess.

@karelz we're weighing which way is less breaking (fixing ref vs fixing src). Currently we're thinking fixing ref is less breaking, and we still have a chance to do that. Are you suggesting that instead we don't make any changes and keep ref and src out of sync?

OK, the original PR dotnet/corefx#23224 introduced the inconsistency: ref with ipString, but implementation with ipSpan (while approved API asked for ipChars).
I agree that changing the implementation to match the ref name makes most sense at this point -- it is the least breaking.

I don't think it is worth to make the breaking change of parameter name in ref in 5.0 (and likely never).

(our replies crossed and I didn't see your summary in #39741 (comment))

Honestly, it probably does not matter which way we go. I am fine to go with recommendation from API review folks. ipSpan is not ideal ... if API review group is fine with such arg name, we can use it. Otherwise, let's stick to ipString.
The risk of breaking customers is either way fairly small IMO.

ericstj · 2020-07-22T00:53:59Z

src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackFrame.cs

@@ -66,10 +66,10 @@ public StackFrame()
        /// <summary>
        /// Constructs a StackFrame corresponding to the active stack frame.
        /// </summary>
-        public StackFrame(bool needFileInfo)
+        public StackFrame(bool fNeedFileInfo)


This is another potentially undesirable change, however preserves compat. cc @tommcdon

src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackFrame.cs

src/libraries/System.Resources.Writer/ref/System.Resources.Writer.cs

src/libraries/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs

src/libraries/System.IO.MemoryMappedFiles/ref/System.IO.MemoryMappedFiles.cs

src/libraries/System.Resources.Writer/ref/System.Resources.Writer.cs

Anipik · 2020-07-23T16:45:21Z

@ericstj @safern @ViktorHofer can u approve this one ?

ViktorHofer · 2020-07-23T16:49:54Z

I defer to @ericstj or @safern to approve as I'm not an expert in detecting breaking changes :)

...raries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Windows.cs

safern · 2020-07-23T18:59:34Z

@ericstj I guess we will need to file breaking change issues for all parameter name changes in implementation given it is a reflection breaking change?

ericstj · 2020-07-23T19:08:45Z

given it is a reflection breaking change?

Possibly, though as I mentioned we considered changing the ref more breaking than "fixing" the src to match the ref. I'd prefer we git area-owners to chime in on what's best for their own API and then based on that decide if they need to document a break.

ericstj · 2020-07-23T19:30:38Z

Actually thinking through this a bit more. It looks to me like largely the runtime side of these has better naming.

The reference side, though more breaking, is less likely to block someone. If they happened to be using named parameters they'll see a compile error with an easy fix.

Compare that to the runtime side which may result in different behavior or exception. At this point a developer may be blocked if they don't have the ability to recompile the code.

I think I'm going to flip on this and reccomend we update refs to match src and just file the breaking change notice for refs. cc @terrajobst @stephentoub in case I'm missing something.

ericstj

LGTM

Let's make sure to file a breaking change notice mentioning all the places where the parameter names change on references, noting it breaks source that referred to these using named-parameters.

Anipik · 2020-07-30T21:52:40Z

Created the breaking change notice dotnet/docs#19763

* minor formatting * ref changes

Dotnet-GitSync-Bot added area-Infrastructure-libraries new-api-needs-documentation labels Jul 21, 2020

Anipik changed the title ~~[Donot Review] Correcting Mismatch between ref and src~~ Correcting Mismatch between ref and src Jul 22, 2020

Anipik requested review from stephentoub, ericstj and ViktorHofer July 22, 2020 00:27

ericstj reviewed Jul 22, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackFrame.cs Outdated Show resolved Hide resolved

ericstj reviewed Jul 22, 2020

View reviewed changes

src/libraries/System.Resources.Writer/ref/System.Resources.Writer.cs Outdated Show resolved Hide resolved

jaredpar mentioned this pull request Jul 22, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

buyaa-n reviewed Jul 22, 2020

View reviewed changes

src/libraries/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs Outdated Show resolved Hide resolved

safern reviewed Jul 22, 2020

View reviewed changes

src/libraries/System.IO.MemoryMappedFiles/ref/System.IO.MemoryMappedFiles.cs Outdated Show resolved Hide resolved

safern reviewed Jul 22, 2020

View reviewed changes

src/libraries/System.Resources.Writer/ref/System.Resources.Writer.cs Show resolved Hide resolved

safern reviewed Jul 23, 2020

View reviewed changes

...raries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Windows.cs Outdated Show resolved Hide resolved

Anipik added 2 commits July 29, 2020 19:38

minor formatting

2dae232

ref changes

8193ba8

ericstj approved these changes Jul 30, 2020

View reviewed changes

ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 30, 2020

Anipik mentioned this pull request Jul 30, 2020

Mismatches for parameter names between reference and implementation assembly dotnet/docs#19763

Closed

Anipik merged commit 621ed57 into dotnet:master Jul 30, 2020

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020

Correcting Mismatch between ref and src (dotnet#39741)

b173282

* minor formatting * ref changes

karelz added this to the 5.0.0 milestone Aug 18, 2020

Anipik deleted the ref branch August 25, 2020 21:51

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correcting Mismatch between ref and src #39741

Correcting Mismatch between ref and src #39741

Anipik commented Jul 21, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Jul 21, 2020

ghost commented Jul 21, 2020

ericstj Jul 22, 2020

ManickaP Jul 22, 2020

ericstj Jul 22, 2020

ericstj Jul 23, 2020

ManickaP Jul 28, 2020

karelz Jul 28, 2020

ManickaP Jul 28, 2020

ericstj Jul 28, 2020

karelz Jul 28, 2020 •

edited

Loading

karelz Jul 28, 2020

ericstj Jul 22, 2020

tommcdon Jul 23, 2020

Anipik commented Jul 23, 2020

ViktorHofer commented Jul 23, 2020 •

edited

Loading

safern commented Jul 23, 2020

ericstj commented Jul 23, 2020

ericstj commented Jul 23, 2020 •

edited

Loading

ericstj left a comment

Anipik commented Jul 30, 2020

Correcting Mismatch between ref and src #39741

Correcting Mismatch between ref and src #39741

Conversation

Anipik commented Jul 21, 2020 • edited Loading

Dotnet-GitSync-Bot commented Jul 21, 2020

ghost commented Jul 21, 2020

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

karelz Jul 28, 2020 • edited Loading

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

Anipik commented Jul 23, 2020

ViktorHofer commented Jul 23, 2020 • edited Loading

safern commented Jul 23, 2020

ericstj commented Jul 23, 2020

ericstj commented Jul 23, 2020 • edited Loading

ericstj left a comment

Choose a reason for hiding this comment

Anipik commented Jul 30, 2020

Anipik commented Jul 21, 2020 •

edited

Loading

karelz Jul 28, 2020 •

edited

Loading

ViktorHofer commented Jul 23, 2020 •

edited

Loading

ericstj commented Jul 23, 2020 •

edited

Loading