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

Updating program to .NET 6 Preview 7 SDK causes warnings about interpolated string handlers in previously working code #55345

Closed
jkoritzinsky opened this issue Aug 2, 2021 · 7 comments · Fixed by #55380
Assignees
Milestone

Comments

@jkoritzinsky
Copy link
Member

Version Used: 4.0.0-3.21376.22 (SDK version 6.0.100-preview.7.21377.7)

Steps to Reproduce:

Write the following program with any SDK prior to the .NET 6 Preview 7 SDK (using any 3.x.x Roslyn version):

<Project>
	<PropertyGroup>
		<OutputType>Exe</OutputType>
		<LangVersion>8.0</LangVersion> <!-- Can be anything less than 10.0 and not Preview -->
	</PropertyGroup>
</Project>
using System;
using System.Text;

StringBuilder builder = new();
builder.Append($"Hello World, {args.Length}");
Console.WriteLine(builder);

Rationale for having the LangVersion element: It was initially for ensuring that new language features could be used in dotnet/runtime and dotnet/runtimelab early, but the line was never removed after C# 8.0 was released.

This builds correctly in Preview 6.

Expected Behavior:

This builds in Preview 7 using the same method overloads as Preview 6, or a diagnostic with more information to know exactly what changed with the upgrade.

Alternatively, a breaking change notice could be posted in partnership with the .NET Libraries team.

Actual Behavior:

This fails to build in Preview 7, with the following diagnostic since Roslyn picks the new overload that uses the new 'interpolated string handlers' feature:

error CS8400: Feature 'interpolated string handlers' is not available in C# 8.0. Please use language version 10.0 or greater.

@jkoritzinsky jkoritzinsky added the Feature - Interpolated String Improvements Interpolated string improvements label Aug 2, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 2, 2021
@jkoritzinsky
Copy link
Member Author

This was hit by @NikolaMilosavljevic in dotnet/runtime and by myself in dotnet/runtimelab while working with Preview 7 builds installed or configured for the repo to use.

@jkoritzinsky
Copy link
Member Author

cc: @333fred @stephentoub

@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 2, 2021
@jaredpar jaredpar added this to the 17.0 milestone Aug 2, 2021
@333fred
Copy link
Member

333fred commented Aug 2, 2021

In order to fix this, we'd have to change the behavior of overload resolution in lower langversions. Possible, but something we generally try to avoid. @jaredpar, what are your thoughts on doing that?

@stephentoub
Copy link
Member

stephentoub commented Aug 2, 2021

In order to fix this, we'd have to change the behavior of overload resolution in lower langversions.

Why is it binding to the handler overload if the language version being used doesn't support lowering interpolated strings to a handler?

@333fred
Copy link
Member

333fred commented Aug 2, 2021

In order to fix this, we'd have to change the behavior of overload resolution in lower langversions.

Why is it binding to the builder overload if the language version being used doesn't support lowering interpolated strings to a builder?

Because currently, language version does not affect binding behavior here (just like it doesn't affect most binding behavior in the compiler). We bind in one fashion, then issue a diagnostic if a conversion is used in the wrong language version. To fix this, we'd have to change binding behavior based on language version. There is precedent, but it's generally something we try to do sparingly because it complicates test matrices. I can see this as being potentially high-impact enough to warrant such a change, however.

@stephentoub
Copy link
Member

Seems like without doing something special-here, anyone using LangVersion < C# 10 with .NET 6 and using an interpolated string with StringBuilder.Append, StringBuilder.AppendLine, or Debug.Assert (or any other APIs that add handler-based overloads for existing string-based ones) will now fail to build.

@333fred
Copy link
Member

333fred commented Aug 2, 2021

anyone using LangVersion < C# 10 with .NET 6

This is the crux of the issue. Given that the feature was specifically designed to ensure that these new overloads are preferred over existing APIs, you can only experience this break after upgrading your version of something (be that the runtime or some other library), and there's an easy workaround (cast to string explicitly), we're reticent to make a binding behavior change based on language version. LDM will be back in session on August 23rd or 25th, and we can discuss whether we should make a behavior change with Mads and the whole group then.

333fred added a commit to 333fred/roslyn that referenced this issue Aug 3, 2021
Closes dotnet#55345. Note that we will still know about the conversion, meaning that if a library introduces an interpolated string handler overload and has another overload with a conversion that is not "better" than any other conversion, such as an overload between `Span<char>` and `CustomHandler`, that code will be considered ambiguous and not compile. I think that is fine though, as 99% of these cases will be overloads between `string` and `CustomHandler`, not `Span<char>` and `CustomHandler`.
333fred added a commit that referenced this issue Aug 3, 2021
Closes #55345. Note that we will still know about the conversion, meaning that if a library introduces an interpolated string handler overload and has another overload with a conversion that is not "better" than any other conversion, such as an overload between `Span<char>` and `CustomHandler`, that code will be considered ambiguous and not compile. I think that is fine though, as 99% of these cases will be overloads between `string` and `CustomHandler`, not `Span<char>` and `CustomHandler`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants