From 60295318b876d2bdc0a478b5f00401ca8a7929ac Mon Sep 17 00:00:00 2001 From: Jan Jones Date: Fri, 13 Sep 2024 12:36:01 +0200 Subject: [PATCH 1/2] Fix null coalescing operator emit with span conversion --- .../LocalRewriter_NullCoalescingOperator.cs | 3 +- .../CSharp/Test/Emit3/FirstClassSpanTests.cs | 101 ++++++++++++++++++ .../Test/Utilities/CSharp/TestSources.cs | 4 + 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs index 2fca3bdf59728..8a74f7adce37d 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs @@ -87,9 +87,10 @@ private BoundExpression MakeNullCoalescingOperator( // if left conversion is intrinsic implicit (always succeeds) and results in a reference type // we can apply conversion before doing the null check that allows for a more efficient IL emit. + // span conversion is from a reference type (array/string) to a value type ([ReadOnly]Span) so it cannot participate here. Debug.Assert(rewrittenLeft.Type is { }); if (rewrittenLeft.Type.IsReferenceType && - BoundNode.GetConversion(leftConversion, leftPlaceholder) is { IsImplicit: true, IsUserDefined: false }) + BoundNode.GetConversion(leftConversion, leftPlaceholder) is { IsImplicit: true, IsUserDefined: false, IsSpan: false }) { rewrittenLeft = ApplyConversionIfNotIdentity(leftConversion, leftPlaceholder, rewrittenLeft); diff --git a/src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs b/src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs index 32345f45a80ea..debb390179dbd 100644 --- a/src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs +++ b/src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs @@ -6694,6 +6694,107 @@ ReadOnlySpan M() Diagnostic(ErrorCode.ERR_EscapeVariable, "x").WithArguments("x").WithLocation(7, 16)); } + [Theory, MemberData(nameof(LangVersions))] + public void Conversion_Array_Span_Coalesce(LanguageVersion langVersion) + { + var source = """ + using System; + class C + { + void M1(int[] a) + { + M2(a ?? Span.Empty); + } + void M2(Span s) => throw null; + } + """; + var comp = CreateCompilationWithSpanAndMemoryExtensions(source, + parseOptions: TestOptions.Regular.WithLanguageVersion(langVersion)); + var verifier = CompileAndVerify(comp); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("C.M1", """ + { + // Code size 25 (0x19) + .maxstack 2 + .locals init (int[] V_0) + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: stloc.0 + IL_0003: ldloc.0 + IL_0004: brtrue.s IL_000d + IL_0006: call "System.Span System.Span.Empty.get" + IL_000b: br.s IL_0013 + IL_000d: ldloc.0 + IL_000e: call "System.Span System.Span.op_Implicit(int[])" + IL_0013: call "void C.M2(System.Span)" + IL_0018: ret + } + """); + } + + [Fact] + public void Conversion_string_ReadOnlySpan_Coalesce() + { + var source = """ + using System; + class C + { + void M1(string s) + { + M2(s ?? ReadOnlySpan.Empty); + } + void M2(ReadOnlySpan s) => throw null; + } + """; + var comp = CreateCompilationWithSpanAndMemoryExtensions(source, parseOptions: TestOptions.Regular13); + var verifier = CompileAndVerify(comp); + verifier.VerifyDiagnostics(); + var owner = ExecutionConditionUtil.IsCoreClr ? "string" : "System.ReadOnlySpan"; + verifier.VerifyIL("C.M1", $$""" + { + // Code size 25 (0x19) + .maxstack 2 + .locals init (string V_0) + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: stloc.0 + IL_0003: ldloc.0 + IL_0004: brtrue.s IL_000d + IL_0006: call "System.ReadOnlySpan System.ReadOnlySpan.Empty.get" + IL_000b: br.s IL_0013 + IL_000d: ldloc.0 + IL_000e: call "System.ReadOnlySpan {{owner}}.op_Implicit(string)" + IL_0013: call "void C.M2(System.ReadOnlySpan)" + IL_0018: ret + } + """); + + var expectedIl = """ + { + // Code size 25 (0x19) + .maxstack 2 + .locals init (string V_0) + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: stloc.0 + IL_0003: ldloc.0 + IL_0004: brtrue.s IL_000d + IL_0006: call "System.ReadOnlySpan System.ReadOnlySpan.Empty.get" + IL_000b: br.s IL_0013 + IL_000d: ldloc.0 + IL_000e: call "System.ReadOnlySpan System.MemoryExtensions.AsSpan(string)" + IL_0013: call "void C.M2(System.ReadOnlySpan)" + IL_0018: ret + } + """; + + comp = CreateCompilationWithSpanAndMemoryExtensions(source, parseOptions: TestOptions.RegularNext); + CompileAndVerify(comp).VerifyDiagnostics().VerifyIL("C.M1", expectedIl); + + comp = CreateCompilationWithSpanAndMemoryExtensions(source); + CompileAndVerify(comp).VerifyDiagnostics().VerifyIL("C.M1", expectedIl); + } + [Fact] public void OverloadResolution_SpanVsIEnumerable() { diff --git a/src/Compilers/Test/Utilities/CSharp/TestSources.cs b/src/Compilers/Test/Utilities/CSharp/TestSources.cs index 7af0695589dde..89ef6fd52daa5 100644 --- a/src/Compilers/Test/Utilities/CSharp/TestSources.cs +++ b/src/Compilers/Test/Utilities/CSharp/TestSources.cs @@ -45,6 +45,8 @@ public Span(T[] arr, int start, int length) this.Length = length; } + public static Span Empty => default; + public void CopyTo(Span other) { Array.Copy(arr, start, other.arr, other.start, Length); @@ -130,6 +132,8 @@ public ReadOnlySpan(T[] arr, int start, int length) this.Length = length; } + public static ReadOnlySpan Empty => default; + public void CopyTo(Span other) { Array.Copy(arr, start, other.arr, other.start, Length); From b5356537643f6b02495269cb7a7dd16e9884ee4e Mon Sep 17 00:00:00 2001 From: Jan Jones Date: Thu, 19 Sep 2024 11:39:54 +0200 Subject: [PATCH 2/2] Simplify check --- .../LocalRewriter/LocalRewriter_NullCoalescingOperator.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs index 8a74f7adce37d..0bced6dfb78a1 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingOperator.cs @@ -87,10 +87,9 @@ private BoundExpression MakeNullCoalescingOperator( // if left conversion is intrinsic implicit (always succeeds) and results in a reference type // we can apply conversion before doing the null check that allows for a more efficient IL emit. - // span conversion is from a reference type (array/string) to a value type ([ReadOnly]Span) so it cannot participate here. Debug.Assert(rewrittenLeft.Type is { }); if (rewrittenLeft.Type.IsReferenceType && - BoundNode.GetConversion(leftConversion, leftPlaceholder) is { IsImplicit: true, IsUserDefined: false, IsSpan: false }) + BoundNode.GetConversion(leftConversion, leftPlaceholder) is { Kind: ConversionKind.Identity or ConversionKind.ImplicitReference }) { rewrittenLeft = ApplyConversionIfNotIdentity(leftConversion, leftPlaceholder, rewrittenLeft);