Delete src/SourceBuild/patches/roslyn/0001-ambiguous-call-site.patch

[ViktorHofer]

Testing

[ViktorHofer]

@jozkee can you please help with the failure in the sdk-source-build leg? Is the ambiguous call intentional?

/vmr/src/roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationHelpers.cs(74,27): error CS0121: The call is ambiguous between the following methods or properties: 'string.Join(string?, ReadOnlySpan<object?>)' and 'string.Join(string?, ReadOnlySpan<string?>)' [/vmr/src/roslyn/src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj::TargetFramework=net9.0]

cc @jeffhandley

I wonder why this only affects source-build and not the msft build.

[jozkee]

You can fix it by changing the src to string.Join(".", (ReadOnlySpan<string?>)[.. names]) that seems like the best target between ReadOnlySpan<string?> and ReadOnlySpan<object?>.

It is not clear to me why the collection expression [.. names] gets ambiguous here cc @dotnet/roslyn. names is a List<string>, the compiler can't decide between using ReadOnlySpan<string?> and ReadOnlySpan<object?>, prior to the new params span APIs, it was resolving to string[].

Nevertheless, for non-params overloads, this always resolves to ReadOnlySpan<object>

List<string> names = new();
Join([names]);

static string Join(string separator, ReadOnlySpan<string> value) => "string";
static string Join(string separator, ReadOnlySpan<object> value) => "object";

It can't convert to ReadOnlySpan<string> at all, try commenting-out the ReadOnlySpan<object> method and you will get an error. "Argument 2: cannot convert from 'collection expressions' to 'System.ReadOnlySpan."

[ViktorHofer]

In case it helps this is with Microsoft (R) Visual C# Compiler version 4.11.0-2.24255.2 (062ad3db) (roslyn) and 9.0.0-preview.5.24256.1 (Microsoft.NETCore.App targeting pack)

[jozkee]

Seems like you still need to keep the patch because roslyn hasn't bumped their dotnet version and haven't stepped into this error.

[ViktorHofer]

Just realized that this only affects source-build because in that build configuration, the library in question targets net9.0 while the msft build targets net8.0. That TFM difference is intentional.

Seems like you still need to keep the patch because roslyn hasn't bumped their dotnet version and haven't stepped into this error.

Yes that's an option. I'm more concerned about a potential breaking change for our customers that we don't know about yet.

[333fred]

I'm more concerned about a potential breaking change for our customers that we don't know about yet.

This will be solved by first-class spans. @jjonescz, another test case for the matrix.

[cston]

@333fred,

This will be solved by first-class spans.

If the argument is a collection expression, the call may be ambiguous, even with first-class span support, because better conversion for collection expressions currently has specific rules for span types and only falls back to implicit conversions for non-span types.

Better conversion for collection expression conversions might need an additional rule to prefer System.ReadOnlySpan<E₁> over System.ReadOnlySpan<E₂> if an implicit conversion exists from E₁ to E₂.

[333fred]

I expect it will work like arrays do in this scenario, because ROS<string> is a more specific type than ROS<object>. If that isn't the case, that's concerning, because the point of first-class spans is to solve cases like this where spans behave differently than arrays. https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYAMBYAUBgRj1UwAJUCAWAbj2IIE4AKAIkB4NwEH2WBKW3AGQCWAZ2AAeCpgB8pKAEMAtgFNhpALyylAdya88AYQB0AKQD2gqKxYxSAbUOH5y4QF09uYulL7SAb3oAbOQAzOQEZGYWTJKkwkoADnJgcsCmYDaSti6kAG5yADYQStzqMiySLHwUQaihMZGWMXGJyanppKYARgBWSgDGwFm5BUUlamVdvQOVeAC+pEA==

[cston]

I expect it will work like arrays do in this scenario, because ROS<string> is a more specific type than ROS<object>.

The better conversion rules for collection expression conversions do not include a variance rule for ReadOnlySpan<T> (that was probably an oversight), and the rules do not fall back to implicit conversion when the two types are span types, so this case will not be handled by the implicit conversions for first-class spans.

The fix may be to add a rule to better conversion for collection expressions for ReadOnlySpan<T> variance, or perhaps remove the collection expression specific rules entirely and rely on first-class spans when compiling with -langversion:13 or higher.

[cston]

The fix may be to add a rule to better conversion for collection expressions for ReadOnlySpan<T> variance, or perhaps remove the collection expression specific rules entirely and rely on first-class spans when compiling with -langversion:13 or higher.

Actually, removing the collection expression rules (and relying on first-class spans) would be a breaking change since we currently prefer ReadOnlySpan<T> over Span<T> for collection expressions while first-class spans prefers Span<T>.

[jjonescz]

Tracked by dotnet/roslyn#73857

[ViktorHofer]

dotnet/roslyn#73656 now hits the failure as well and I expect the roslyn team to fix / work around this directly in the roslyn code base to unblock the Arcade dependency flow. Therefore closing this PR.