this is caused by the Rune changes from dotnet/runtime#120145

@jkotas @T-Gro does this need a fix in fsharp?

this is caused by the Rune changes from dotnet/runtime#120145

@tarekgh @Joy-less

The code in the F# compiler can be annotated to choose the right overload, but the code has nothing "special" about it - similar calling patterns will for sure exist at other F# programs.

Meaning this will break user code equally well. It is a a source-breaking change for languages that are in the business of any non-trivial type inference.

Thinking out loud:
Are the Rune overloads meant to be used as a primary choice here?
If not, wouldn't they sit better as an extension only for users who opt-in into opening a namespace?

    type String with

        member inline x.StartsWithOrdinal value =
            x.StartsWith(value, StringComparison.Ordinal)

        member inline x.EndsWithOrdinal value =
            x.EndsWith(value, StringComparison.Ordinal)

        member inline x.EndsWithOrdinalIgnoreCase value =
            x.EndsWith(value, StringComparison.OrdinalIgnoreCase)

@akoeplinger : Orthogonal to the message above, can I make the code change for fsharp compiler's own usage here in this PR directly ,, to stop blocking dotnet/runtime forward flow?

Since this is main, it is for the NET11 period already, right?

yes it is for net11 and you can make the change here but as you said it'll break user code as well so I'm not sure if it's the right course of action.

The only other quick option is to revert the PR that introduced these APIs.

There are likely going to be more build breaks in F# code in other repos once the new APIs starts propagating through the system (e.g. in test code that does not build in dotnet/dotnet). I am not able to predict how bad it is going to be.

@T-Gro Your call whether to fix the F# compiler here and allow the new APIs to propagate, or whether you would like to see the new APIs reverted until this is discussed.

this is caused by the Rune changes from dotnet/runtime#120145

@tarekgh @Joy-less

Hello, I am not understanding what has been broken here. Have I done something wrong in my pull request? Why doesn't it work in F#?

@tarekgh @Joy-less

Hello, I am not understanding what has been broken here. Have I done something wrong in my pull request? Why doesn't it work in F#?

The reason is that w.r.t. to type inference (no need to manually annotate types of values, return values, arguments etc.), the change of going from 1 method overload with the same amount of parameters to 2+ method overloads is a source-breaking-change.

@T-Gro Your call whether to fix the F# compiler here and allow the new APIs to propagate, or whether you would like to see the new APIs reverted until this is discussed.

I am afraid that if we let it in now, there will be no reason to push the discussion further and decrease the chances of changing the design.

I would personally be in favor of:

• revert
• propose/discuss introducing this as an opt-in extension

One reason is the source-breaking-change for inferred argument types, the other would be from a code organization philosophy - extensions are a tool to avoid putting hundreds of methods into a few "master classes".

the other would be from a code organization philosophy - extensions are a tool to avoid putting hundreds of methods into a few "master classes".

We prefer instance methods over extension methods when designing new APIs. We are perfectly fine with a lot of methods on a class. This is mentioned in .NET Framework design guidelines (you can checkout e-book in ms library):

AVOID frivolously defining extension methods, especially on types outside your library or framework.
If you do own source code of a type, consider using regular instance methods instead

Introducing this as an extension would be controversial.

Does the issue is with StartsWith and EndsWith only? if this is the case, we can revert only these two APIs till we discuss and finalize what we should do. I am not a fan of reverting everything here. Also, have we had cases like this before? If so, what did we do to solve it? changing the API or fix the F# code? This is unfortunate to have this restriction.

The reason is that w.r.t. to type inference (no need to manually annotate types of values, return values, arguments etc.), the change of going from 1 method overload with the same amount of parameters to 2+ method overloads is a source-breaking-change.

I am wondering if this can happen most of the time by adding overloads with the same number of parameters. I don't think we should be restricting adding the right form of overloads because of that.

CC @bartonjs @stephentoub

It's certainly a thing we can discuss...

Off the top of my head:

• We definitely care about runtime breaking changes.
• We care about a lot, but not all, C# source-breaking changes.
  • The changes we don't care about tend to involve typeless code, e.g. var and default.
    • null, when a valid value, we do care about, and we would generally avoid adding an overload that would break passing null.
  • e.g. overloading SomeReader.TryReadNumber(out int) with SomeReader.TryReadNumber(out BigInteger) would fail for a caller with reader.TryReadNumber(out var value)
• If it's a case that we'd be OK with breaking, we're not OK with the break if it'll break "everyone"
• We care about all .NET languages.
  • But we don't always care about them all equally.

So it'll sort of come down to "is this going to break 'everyone' who uses F#?". If it requires two...four...twenty things to align, then we might say to break them. If OverloadResolutionPriorityAttribute can solve this case, we'd say use that. If "everyone" who uses F# will be broken by this, we might decide either to do extension methods, or cut the additions entirely.

wouldn't they sit better as an extension ...

Jan already quoted one of the guidelines, but the gist is "nothing should be an extension method unless there's a good reason". (e.g. "One of the arguments is defined in a later assembly", "It's a method on a generic type that's popular with value types and it won't be used for most <T>", "it's an instance method on a generic type, but only makes sense for select <T>", etc.)

"We think the method is important, but it's too-breaking for F#, and there's no other way" might be a reason. But we definitely don't start there.

@T-Gro to unblock this PR, could you please fix this in the F# side and open issue to track discussing this further? I can open the issue if you like too.

So it'll sort of come down to "is this going to break 'everyone' who uses F#?

You can see the added APIs list here dotnet/runtime#27912 (comment). Considering the changes are in String class, the change may break many users, but it depends on what API they are calling. I guess many F# users can be affected but source change should be simple and few (depending on the code).

Now, to unblock this PR, what do you think? Revert the changes in the runtime till we decide? or fix the F# failure here? We'll discuss how to fix this either way later.

I don't really have enough information/knowledge to make a recommendation.

I see 4 errors reported. If they're the only four, that looks like "maybe not a giant break". If they're just the first four, and things stopped building, then maybe it's a giant break.

The first three look like some form of local function definition. Is that common? Is that canonically written? I don't use F#, so I don't know.

The fourth one looks like it's in (what in C# would be) a lambda-like context. Is that code canonical? If so, maybe it's going to be a massive break. Or maybe this is the only line of code in the whole world combining tryFindIndexBack with string.StartsWith.

The only reasonable advice I can give is that seeing what change would be required to just push this through is useful data (if it's small, maybe we take the break-risk / if it's large, maybe we do something else / if it's medium we get to go "hmmm" a lot).

My understanding of the four failures is that they occur because the code uses StartsWith and EndsWith, for which we recently added overloads with the same number of parameters. If there are users calling other APIs (e.g., IndexOf) where we’ve also added overloads with the same parameter count as existing ones, I would expect similar failures there as well. I’m not an expert in F#, so @T-Gro may be able to provide more insight.

The list of APIs we added is here dotnet/runtime#27912 (comment)

In general, this source breaking change happens when:

• Method call was done with a value which has not yet been inferred to a specific type.
• There used to be only 1 overload to choose from before (with the exact same amount of parameters after applying other, known, arguments), and now there are more.

Indeed, this might have affected more APIs from the same PR.

The change is not source breaking for F# if the value's type has been already inferred to be string. If that is the case, F# compiler will know which overload to pick.

That being said, not manually annotating types is the idiomatic programming style for F#(i.e. not something hypothetical):

        member inline x.StartsWithOrdinal value =
            x.StartsWith(value, StringComparison.Ordinal)

You mentioned OverloadResolutionPriorityAttribute which makes me curious - would the goal be to annotate runtime APIs for the "preferred" overload in additions like this, i.e. is that something we could expect to be lived across the BCL? The compiler does not support it now because there was no big reason for it, but uniquely resolving method overloads in type-inference scenarios (e.g. between string vs Rune, between array vs ROS etc.) is a strong enough reason to consider it.

(the big "if" comes around the usage of it)

Just to make it super clear - my position for sure isn't saying no to Rune-accepting methods.

But unless Runes are meant to become "the new char", I still see value in detaching them from the most common type in .NET and only showing/exposing them to users who seek them. Just like Regex search not being directly defined on System.String either.

If I am wrong and Runes are meant to be used by most .NET programmers, let's just accept this and F# programs will get a compiler diagnostic forcing them to choose once they update their framework.

But unless Runes are meant to become "the new char", I still see value in detaching them from the most common type in .NET and only showing/exposing them to users who seek them. Just like Regex search not being directly defined on System.String either.

We’re adding these Rune APIs due to the growing need for them. They are not intended to replace char, but they should be treated with the same level of importance and consistency as char.

Just to make it super clear - my position for sure isn't saying no to Rune-accepting methods.

But unless Runes are meant to become "the new char", I still see value in detaching them from the most common type in .NET and only showing/exposing them to users who seek them. Just like Regex search not being directly defined on System.String either.

If I am wrong and Runes are meant to be used by most .NET programmers, let's just accept this and F# programs will get a compiler diagnostic forcing them to choose once they update their framework.

Please note that the changes are not solely related to Rune - the errors blocking this PR would actually exist even if no Rune overloads were added, because the previous PR also added string.StartsWith(char, StringComparison) etc that allows you to do the following:

Console.WriteLine("HELLO".StartsWith('h', StringComparison.OrdinalIgnoreCase)); // new overload that returns True

Previously, you could only pass a string:

Console.WriteLine("HELLO".StartsWith("h", StringComparison.OrdinalIgnoreCase)); // True

This addition makes it consistent with methods like string.Contains(char, StringComparison) which already exist:

Console.WriteLine("HELLO".Contains('h', StringComparison.OrdinalIgnoreCase)); // True

.. the previous PR also added ...

Yes, that is the same category here.

they should be treated with the same level of importance and consistency

If the goal was to treat it on-par witch char and flow it trough many levels of APIs, I wonder if there isn't a missing generalization, just there used to be a missing one for "generic math".

If the type system could express the semantic intention of "all search APIs accept a 'T which is a string OR char OR Rune", it would generalize well, work with type inference and would not cause a source-breaking change for F# programs.

I do realize this might be a non-goal, but it would also reduce the need for multiplying overloads across method calls that end up calling the same underlying helper.

This is blocking the codeflow for too long.

@T-Gro Could you please add commit to this PR to make the FSharp compiler build to unblock this?

If the goal was to treat it on-par witch char and flow it trough many levels of APIs, I wonder if there isn't a missing generalization, just there used to be a missing one for "generic math".

I do not think so. It is super common to overload APIs like this without any generalizations. From API design guidelines:

Overloading is one of the most important techniques for improving the usability, productivity, and readability of reusable libraries. Overloading on the number of parameters makes it possible to provide simpler versions of constructors and methods. Overloading on the parameter type makes it possible to use the same member name for members performing identical operations on a selected set of different types.