Support basic end-to-end scenarios for converting string constants to UTF8 byte representation. #58849

AlekseyTs · 2022-01-13T22:12:00Z

Proposal - https://github.com/dotnet/csharplang/blob/main/proposals/utf8-string-literals.md
Relates to test plan #58848

…o UTF8 byte representation. Proposal - https://github.com/dotnet/csharplang/blob/main/proposals/utf8-string-literals.md

AlekseyTs · 2022-01-14T14:30:58Z

@dotnet/roslyn-compiler Please review

RikkiGibson

LGTM aside from some nits and questions

RikkiGibson · 2022-01-14T19:08:29Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

@@ -825,6 +827,17 @@ public bool IsConstantExpression
            }
        }

+        /// <summary>
+        /// Returns true if the conversion is an implicit Utf8 strings literal conversion.


Looks like string was meant to be used here instead of strings #Closed

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs

RikkiGibson · 2022-01-14T19:52:13Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1570,7 +1637,22 @@ internal Conversion GetCallerLineNumberConversion(TypeSymbol destination, ref Co

            TypeSymbol expectedAttributeType = corLibrary.GetSpecialType(SpecialType.System_Int32);
            BoundLiteral intMaxValueLiteral = new BoundLiteral(syntaxNode, ConstantValue.Create(int.MaxValue), expectedAttributeType);
-            return ClassifyStandardImplicitConversion(intMaxValueLiteral, expectedAttributeType, destination, ref useSiteInfo);
+
+            // Below is a dupliucation of relevant parts of ClassifyStandardImplicitConversion method.


nit: dupliucation -> duplication #Closed

RikkiGibson · 2022-01-14T19:58:20Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+
+            TypeSymbol destinationOriginalDefinition = destination.OriginalDefinition;
+            if (constantValue?.IsString == true && // PROTOTYPE(UTF8StringLiterals) : confirm if we actually want it to work with 'null' constant value.
+                sourceExpression.Type?.SpecialType == SpecialType.System_String &&


Are there known scenarios where constantValue?.IsString == true but sourceExpression.Type is not string? #Resolved

Are there known scenarios where constantValue?.IsString == true but sourceExpression.Type is not string?

I am not aware of any specific scenario like that. This doesn't mean they couldn't exist.

RikkiGibson · 2022-01-14T20:18:48Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -80,9 +80,10 @@ internal ConversionsBase WithNullability(bool includeNullability)
        /// Determines if the source expression is convertible to the destination type via
        /// any built-in or user-defined implicit conversion.
        /// </summary>
-        public Conversion ClassifyImplicitConversionFromExpression(BoundExpression sourceExpression, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        public Conversion ClassifyImplicitConversionFromExpression(BoundExpression sourceExpression, CSharpCompilation compilation, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)


Was it considered to make 'compilation' a field on `ConversionsBase` instead of adding a parameter? #Resolved

Was it considered to make 'compilation' a field on `ConversionsBase` instead of adding a parameter?

ConversionBase intentionally doesn't depend on any binding context. Classification of conversions between types doesn't depend on it. I tried to move all classification of conversion from expression to Conversions (it has Binder), unfortunately implementation of many methods is structured in such a way that it bundles both classifications (from type and from expression) together and conditionally looks at expression when it is passed in. That effort quickly became rather complicated because one of the goals was to avoid code duplication and minimize code changes. Then I tried the approach where compilation is passed along with expression and that became much easier to implement and I think it is much easier to review.

Makes sense. Thanks for the information.

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

RikkiGibson · 2022-01-14T21:10:33Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+}
+";
+
+        [ConditionalFact(typeof(CoreClrOnly))]


I didn't understand why this condition is specified on many of the tests. Is it not possible to execute code that uses the feature on desktop framework? #Resolved

I didn't understand why this condition is specified on many of the tests. Is it not possible to execute code that uses the feature on desktop framework?

Documentation says "Span" types are not available on desktop.

RikkiGibson · 2022-01-14T21:17:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+            var comp = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.DebugExe);
+
+            CompileAndVerify(comp, expectedOutput: @"ReadOnlySpan").VerifyDiagnostics();
+        }


Should there also be a test where a type defines both an implicit conversion from string and from e.g. ReadOnlySpan<byte>? Apologies if I missed it. #Closed

Should there also be a test where a type defines both an implicit conversion from string and from e.g. ReadOnlySpan<byte>?

Having an additional test doesn't hurt. However, UserDefinedImplicitConversions_01 demonstrates that a user defined conversion like that cannot be used as an implicit conversion.

RikkiGibson · 2022-01-14T21:18:20Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+}
+";
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.DebugExe);
+            comp.VerifyEmitDiagnostics(


Consider also leaving a prototype comment here to give a diagnostic which indicates the requirement of a constant string for the conversion to work. #Resolved

Consider also leaving a prototype comment here to give a diagnostic which indicates the requirement of a constant string for the conversion to work.

I think the error message here is accurate and sufficiently good.

cston · 2022-01-14T22:35:36Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+                (destination is ArrayTypeSymbol { IsSZArray: true, ElementType.SpecialType: SpecialType.System_Byte } || // byte[]
+                 ((destinationOriginalDefinition.Equals(compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions) ||             // Span<T>
+                   destinationOriginalDefinition.Equals(compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)) &&    // ReadOnlySpan<T>
+                   destinationOriginalDefinition.TypeKind == TypeKind.Struct && destinationOriginalDefinition.IsRefLikeType &&


destinationOriginalDefinition.TypeKind == TypeKind.Struct && destinationOriginalDefinition.IsRefLikeType &&

Consider moving these checks ahead of the calls to Equals(compilation.GetWellKnownType(...)) since these seem less expensive. #Resolved

cston · 2022-01-14T23:38:47Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+        }
+
+        [ConditionalFact(typeof(CoreClrOnly))]
+        public void UserDefinedExplictConversions_01()


Suggested change

public void UserDefinedExplictConversions_01()

public void UserDefinedExplicitConversions_01()

``` #Resolved

cston · 2022-01-14T23:39:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+        }
+
+        [ConditionalFact(typeof(CoreClrOnly))]
+        public void UserDefinedExplictConversions_02()


Suggested change

public void UserDefinedExplictConversions_02()

public void UserDefinedExplicitConversions_02()

``` #Resolved

cston · 2022-01-14T23:44:14Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+{
+    static void Main()
+    {
+        System.Console.WriteLine(Test(""s""));


Test(""s"")

Why do we prefer ReadOnlySpan<char> over byte[] rather than being ambiguous? #Resolved

Why do we prefer ReadOnlySpan<char> over byte[] rather than being ambiguous?

Conversion to ReadOnlySpan<char> is not a new conversion.

cston · 2022-01-14T23:49:51Z

src/Compilers/CSharp/Test/Semantic/Semantics/Utf8StringsLiteralsTests.cs

+            var comp = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.DebugExe);
+
+            CompileAndVerify(comp, expectedOutput: @"string").VerifyDiagnostics();
+        }


}

Consider including an example where overload resolution behavior has changed. For instance, will the following report "string" with C#10, and "byte[]" with C#11?

using System; class Program { static void Main() { var p = new Program(); Console.WriteLine(p.M("")); } public string M(byte[] b) => "byte[]"; } static class E { public static string M(this object o, string s) => "string"; } ``` #Resolved

AlekseyTs · 2022-01-20T19:22:03Z

I will follow up on the feedback marked as pending in another PR.

In reply to: 1017843992

Support basic end-to-end scenarios for converting string constatnts t…

622aa4d

…o UTF8 byte representation. Proposal - https://github.com/dotnet/csharplang/blob/main/proposals/utf8-string-literals.md

AlekseyTs added Area-Compilers Feature - Utf8StringLiterals labels Jan 13, 2022

AlekseyTs requested a review from a team as a code owner January 13, 2022 22:12

RikkiGibson self-assigned this Jan 14, 2022

Add language version check.

0574b02

RikkiGibson approved these changes Jan 14, 2022

View reviewed changes

cston reviewed Jan 14, 2022

View reviewed changes

cston approved these changes Jan 20, 2022

View reviewed changes

AlekseyTs merged commit 36def1e into dotnet:features/Utf8StringLiterals Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support basic end-to-end scenarios for converting string constants to UTF8 byte representation. #58849

Support basic end-to-end scenarios for converting string constants to UTF8 byte representation. #58849

AlekseyTs commented Jan 13, 2022 •

edited by jcouv

Loading

AlekseyTs commented Jan 14, 2022

RikkiGibson left a comment

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

RikkiGibson Jan 14, 2022

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2022

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 20, 2022 •

edited

Loading

	public void UserDefinedExplictConversions_01()
	public void UserDefinedExplicitConversions_01()
	``` #Resolved

Support basic end-to-end scenarios for converting string constants to UTF8 byte representation. #58849

Support basic end-to-end scenarios for converting string constants to UTF8 byte representation. #58849

Conversation

AlekseyTs commented Jan 13, 2022 • edited by jcouv Loading

AlekseyTs commented Jan 14, 2022

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

RikkiGibson Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

cston Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2022

Choose a reason for hiding this comment

cston Jan 14, 2022 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 20, 2022 • edited Loading

AlekseyTs commented Jan 13, 2022 •

edited by jcouv

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

RikkiGibson Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

cston Jan 14, 2022 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 20, 2022 •

edited

Loading