Support Pattern Matching ReadOnlySpan<char> on constant string by YairHalberstadt · Pull Request #44388 · dotnet/roslyn

YairHalberstadt · 2020-05-19T18:54:01Z

Switching ReadOnlySpan on a constant string is championed at dotnet/csharplang#1881 and marked as AnyTime.

It seems inconsistent to be able to switch on ReadOnlySpan but not use it as a pattern, so I've added support for that too.

The approach is to generate a call to MemoryExtensions.SequenceEqual(span, pattern.AsSpan()).

This delegates to the same underlying method as string.Equals does on .Net Core so should be very efficient.

For large switches, we use the same optimization as string, namely generate a hash function to switch on before doing the equality check. If required members for the hash function aren't there we fall back to a series of if else equality checks.

I've also added support for warning on e.g. span is "foo" and "bar".

Open Questions

Should this also support pattern matching Span<char> on a constant string?
A) Yes

Relates to test plan: #59191

jaredpar · 2020-05-19T19:18:41Z

We're going to need to create a feature branch for this, or possible just create features/compiler-next. This can't go into master as we're fairly locked down on the set of features we're delivering for C# 9 right now (not just compiler implementation that is our constraint, it's constraints on other teams as well).

Will discuss with team today if we want to use a feature branch or compiler-next for this.

… are impossible to match ,or match all values

…Halberstadt/roslyn into feature/switch-on-span-char

Happypig375 · 2020-05-22T09:36:46Z

So excited to see progress on my own suggestion! My computer is too old and slow to handle a repository as large as Roslyn, taking at least half an hour to even build, so I am excited to see someone else helping out!

Happypig375 · 2020-08-15T06:54:25Z

@YairHalberstadt Span<char> won't be supported?

YairHalberstadt · 2020-08-15T17:17:03Z

@Happypig375
That's up to the language team to decide.

RikkiGibson · 2020-08-15T21:51:56Z

Is there a proposal or speclet for the feature implemented by this PR?

Happypig375 · 2020-08-16T03:50:27Z

@RikkiGibson The proposal is dotnet/csharplang#1881.

YairHalberstadt · 2020-10-09T09:24:35Z

I've now added support for Span<char>

jaredpar · 2020-10-23T21:56:55Z

                         WellKnownType.System_Runtime_CompilerServices_NativeIntegerAttribute,
-                         WellKnownType.System_Runtime_CompilerServices_IsExternalInit
+                         WellKnownType.System_Runtime_CompilerServices_IsExternalInit,
+                         WellKnownType.System_MemoryExtensions


I'm a little wary of using MemoryExtensions here. This is adding a new type dependency to the compiler and one where we have some dependency on how the members are implemented. Why not just use the operator== on the Span<T> and ReadOnlySpan<T> types instead. That maintains our existing type dependency and we already depend on the implementation of several of the type members.

If the runtime exposes these APIs to us from a class we already take a type dependency on, that might be preferable, but I'm not sure operator == on Span would do what we want. For patterns I think we want to compare the values within the span to the characters of the string constant, but this operator just checks equality of the length and underlying reference.

https://github.com/dotnet/runtime/blob/d603356dfbf09616e55bd99ed840aa20468a9e57/src/libraries/System.Private.CoreLib/src/System/Span.cs#L346-L347

jaredpar · 2020-10-23T22:32:46Z

+        /// This is a layer on top of the Compilation version that generates a diagnostic if the special
+        /// member isn't found.
+        /// </summary>
+        internal Symbol GetWellKnownTypeMember(WellKnownMember member, DiagnosticBag diagnostics, SyntaxNode syntax)


Tempted to call this TryGetWellKnownTypeMember too.

This is for consistency with the already existing GetSpecialTypeMember and TryGetSpecialTypeMember

jaredpar · 2020-10-23T22:33:21Z

+            where TSymbol : Symbol
+        {
+            symbol = (TSymbol)compilation.GetWellKnownTypeMember(wellKnownMember);
+            if ((object)symbol == null)


Suggested change

if ((object)symbol == null)

if (symbol is null)

jaredpar · 2020-10-23T22:42:00Z

I think we need to get a spec together for this change. It's pretty small but we need to spec the behavior. I think the LDM notes from 10/7 are a good starting point. https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-10-07.md#readonlyspanchar-patterns

Particularly I think the interesting points we need to spec out and agree on are:

How is null is handled. The PR handles it as an error which I tend to agree with but need to spec that out. The LDM notes say "constant string" and null is a valid constant string so need to get crisp on that.
How the comparison is achieved. The PR takes the approach of using MemoryExtensions. Think should weigh that against just using the equality operators on the Span<T> types themselves. Again though this is somethnig we should spec out in detail.

…-on-span-char

cston · 2022-01-28T22:07:23Z

+        /// This is a layer on top of the Compilation version that generates a diagnostic if the well known
+        /// member isn't found.
+        /// </summary>
+        internal Symbol? GetWellKnownTypeMember(WellKnownMember member, BindingDiagnosticBag diagnostics, SyntaxNode syntax)


GetWellKnownTypeMember

Is this different in behavior from the GetWellKnownTypeMember() overload at line 1645? #Pending

cston · 2022-01-28T22:10:52Z

+                : null;
+        }
+
+        internal static bool TryGetWellKnownTypeMember<TSymbol>(CSharpCompilation compilation, WellKnownMember wellKnownMember, SyntaxNode syntax, BindingDiagnosticBag diagnostics, [MaybeNullWhen(false)] out TSymbol symbol)


TryGetWellKnownTypeMember

Could this method be replaced with one of the existing GetWellKnownTypeMember() overloads, for instance the overload at line 1664? #Pending

cston · 2022-01-28T22:11:31Z

        /// </summary>
        internal Symbol GetSpecialTypeMember(SpecialMember member, BindingDiagnosticBag diagnostics, SyntaxNode syntax)
        {
-            Symbol memberSymbol;


Symbol memberSymbol

Consider reverting the changes to this method if not needed. #Pending

cston · 2022-01-28T22:38:11Z

+                    ? WellKnownMember.System_ReadOnlySpan_T__get_Length
+                    : WellKnownMember.System_Span_T__get_Length) as MethodSymbol;
+                var spanCharLengthMethod = spanTLengthMethod?.AsMember((NamedTypeSymbol)keyType);
+                if (spanCharLengthMethod != null && !spanCharLengthMethod.HasUseSiteError)


spanCharLengthMethod != null && !spanCharLengthMethod.HasUseSiteError

It seems surprising that we allow Span<T>.Length to be missing. (It looks like we allow missing String.Length for the existing string switch cases, and emit different code when comparing against "", although I'm not sure why.) I think it would be better to require Span<T>.Length and ReadOnlySpan<T>.Length in Binder where we check for other well-known members needed for the span cases, and remove the conditional code here. #Pending

cston · 2022-01-28T23:45:41Z

+                LabelSymbol start = F.GenerateLabel("start");
+
+
+                //  This method should be kept consistent with SynthesizedStringSwitchHashMethod.ConstructStringHash


ConstructStringHash

ComputeStringHash? #Pending

cston · 2022-01-28T23:59:29Z

+                    if (MessageID.IDS_FeatureSpanCharConstantPattern.CheckFeatureAvailability(diagnostics, Compilation, node.Location))
+                    {
+                        // report missing member and use site diagnostics
+                        if (inputType.IsReadOnlySpanChar())


inputType.IsReadOnlySpanChar()

Consider using conditional expression within the GetWellKnownTypeMember() call rather than if { } else { }, for simplicity and consistency with other similar uses of these members. #Pending

RikkiGibson · 2022-01-28T20:11:29Z

            if (wasExpression)
            {
+                var convertedType = convertedExpression.Type ?? inputType;
+                if (convertedType.SpecialType == SpecialType.System_String && inputType.IsSpanOrReadOnlySpanChar())


It wasn't obvious to me why this was the right change to make, versus doing a conversion from string to ROS<char> in ConvertPatternExpression().

RikkiGibson · 2022-01-28T20:36:44Z


-            Cci.IReference stringEqualityMethodRef = _module.Translate(equalityMethod, syntaxNode, _diagnostics);
+            Cci.IMethodReference stringEqualityMethodRef = null;
+            Cci.IMethodReference lengthRef = null;


consider using 'MethodRef' instead of 'Ref' for this and below locals for clarity/consistency with 'stringEqualityMethodRef' #Pending

RikkiGibson · 2022-01-28T20:37:55Z

+                var sequenceEqualsTMethod = _module.Compilation.GetWellKnownTypeMember(isReadOnlySpan
+                    ? WellKnownMember.System_MemoryExtensions__SequenceEqual_ReadOnlySpan_T
+                    : WellKnownMember.System_MemoryExtensions__SequenceEqual_Span_T) as MethodSymbol;
+                var sequenceEqualsCharMethod = sequenceEqualsTMethod?.Construct(_module.Compilation.GetSpecialType(SpecialType.System_Char));


I didn't understand why we defend against 'null' here and for other 'GetWellKnownTypeMember' methods when we are supposed to issue error diagnostics during binding when these helpers are not present. #Pending

RikkiGibson · 2022-01-28T20:44:19Z

+                        if (isSpanOrReadOnlySpan)
+                        {
+                            // The caller ensures that the key is not byref, and is not a stack local
+                            if (keyArg.Local is { } local)


Does this do the right thing in scenarios where the input span is a field of a ref struct? #Pending

It looks like LocalRewriter.PatternLocalRewriter.ShareTempsAndEvaluateInput() ensures the value for the switch is either a parameter or a local. Added SwitchSpanChar_RefStructField() test for this case.

RikkiGibson · 2022-01-28T21:43:09Z

        }
    }
+
+    internal sealed partial class SynthesizedSpanSwitchHashMethod : SynthesizedGlobalMethodSymbol


I don't like that we made 'SynthesizedSpanSwitchHashMethod' and 'SynthesizedStringSwitchHashMethod' partial. It looks like that's just so the constructor can go over here. However, I don't think it's your responsibility to fix it.

RikkiGibson · 2022-01-29T00:52:30Z

+}";
+            CreateCompilationWithSpanAndMemoryExtensions(source, parseOptions: TestOptions.RegularPreview)
+                .VerifyDiagnostics(
+                    // (5,49): error CS0150: A constant value is expected


It seems like it might be better to have an error here about the unconvertibility of null to type Span<char>.

RikkiGibson · 2022-01-29T00:53:32Z

+using System;
+class C
+{
+    static bool M(Span<char> chars) => chars is null;


We should ensure we test a scenario where a named const string with a null value is used in a pattern. #Pending

RikkiGibson · 2022-01-29T01:00:24Z

+                    1,
+                    (byte)SignatureTypeCode.GenericMethodParameter, (byte)0,
+
+                // System_MemoryExtensions__AsSpanString


Should this be 'AsSpan_String'? #Pending

RikkiGibson · 2022-01-29T01:09:02Z

                         WellKnownType.System_Runtime_CompilerServices_NativeIntegerAttribute,
-                         WellKnownType.System_Runtime_CompilerServices_IsExternalInit
+                         WellKnownType.System_Runtime_CompilerServices_IsExternalInit,
+                         WellKnownType.System_MemoryExtensions


If the runtime exposes these APIs to us from a class we already take a type dependency on, that might be preferable, but I'm not sure operator == on Span would do what we want. For patterns I think we want to compare the values within the span to the characters of the string constant, but this operator just checks equality of the length and underlying reference.

https://github.com/dotnet/runtime/blob/d603356dfbf09616e55bd99ed840aa20468a9e57/src/libraries/System.Private.CoreLib/src/System/Span.cs#L346-L347

cston · 2022-01-31T22:28:27Z

        }

+        [Fact]
+        public void PatternMatchReadonlySpanCharOnConstantString()


Suggested change

public void PatternMatchReadonlySpanCharOnConstantString()

public void PatternMatchReadOnlySpanCharOnConstantString()

``` #Pending

cston · 2022-01-31T23:15:14Z

+class C
+{
+    static void Main()
+    {


{

Consider adding Test("");. #Pending

cston · 2022-01-31T23:16:39Z

+class C
+{
+    static void Main()
+    {


{

Consider adding Test("");. #Pending

cston · 2022-01-31T23:20:03Z

+                    // (5,49): error CS8652: The feature 'pattern matching ReadOnly/Span<char> on constant string' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.
+                    //     static bool M(Span<char> chars) => chars is "";
+                    Diagnostic(ErrorCode.ERR_FeatureInPreview, @"""""").WithArguments("pattern matching ReadOnly/Span<char> on constant string").WithLocation(5, 49));
+        }


}

Consider testing with TestOptions.Regular10 instead (and rename method). #Pending

cston · 2022-01-31T23:20:14Z

+                    // (5,57): error CS8652: The feature 'pattern matching ReadOnly/Span<char> on constant string' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.
+                    //     static bool M(ReadOnlySpan<char> chars) => chars is "";
+                    Diagnostic(ErrorCode.ERR_FeatureInPreview, @"""""").WithArguments("pattern matching ReadOnly/Span<char> on constant string").WithLocation(5, 57));
+        }


}

Consider testing with TestOptions.Regular10 instead (and rename method). #Pending

cston · 2022-01-31T23:41:53Z

+class C
+{
+    static void Main()
+    {


{

Consider adding Test("".ToArray()); #Pending

cston · 2022-02-01T00:35:28Z

+        /// This has a different diagnostic to <see cref="SwitchReadOnlySpanCharOnNull"/> because there is a valid implicit conversion from null to Span via Array,
+        /// but not to the ReadOnlySpan defined in <see cref="TestSources.Span"/> which has two implicit operators from reference types to ReadOnlySpan (for string and array).
+        /// When using the BCL, the diagnostic will be this one for both.
+        /// </summary>


We should test with the BCL types rather than TestSources, to avoid surprises (due to different implicit conversions, etc.) when compiling projects that are targeting the BCL.

Please consider changing the new tests in this file that depend on definitions of System.Span<T> and System.MemoryExtensions to compile against CoreCLR, with [ConditionalFact].

[ConditionalFact(typeof(CoreClrOnly))] public void SwitchSpanCharOnNull() { var source = "..."; CreateCompilation( source, targetFramework: TargetFramework.NetCoreApp, parseOptions: TestOptions.RegularPreview); // ... } ``` #Pending

cston · 2022-02-01T00:36:31Z

    }
 }";
+
+        public const string MemoryExtensions = @"


MemoryExtensions

Consider removing if tests compile against BCL types instead. #Pending

cston · 2022-02-02T18:17:46Z

@YairHalberstadt, sorry for the delay reviewing this PR. We've completed a couple of passes through the latest commit and expect to be able to review any subsequent commits promptly.

For planning purposes, do you have a timeframe in mind when you expect to address the remaining feedback? If it might be more than a few days, we could merge this PR into a feature branch and assign someone else to address remaining items in separate PRs to that branch. Let us know how you'd like to proceed, thanks.

YairHalberstadt · 2022-02-02T18:20:48Z

@cston thanks for the review

I don't think I'll be able to address the feedback in the next couple of weeks unfortunately.

cston · 2022-02-08T17:59:14Z

We'll address the remaining comments in a separate PR to the feature branch.

cston

LGTM, assuming remaining issues are addressed in a follow-up PR.

RikkiGibson

LGTM with the assumption that pending comments will be addressed in a follow-up PR

cston · 2022-02-08T21:40:44Z

Thanks @YairHalberstadt!

Support Pattern Matching ReadOnlySpan<char> on constant string

4465eb2

YairHalberstadt requested a review from a team as a code owner May 19, 2020 18:54

YairHalberstadt added 2 commits May 20, 2020 07:07

Update well known member and type tests

2cc1782

Add errors and warning for readonlyspan constant string patterns that…

f2c6f99

… are impossible to match ,or match all values

This comment was marked as resolved.

Sign in to view

YairHalberstadt closed this May 20, 2020

YairHalberstadt reopened this May 20, 2020

YairHalberstadt added 3 commits May 21, 2020 14:30

Merge branch 'master' into feature/switch-on-span-char

889fab6

Merge branch 'feature/switch-on-span-char' of https://github.com/Yair…

bd281e9

…Halberstadt/roslyn into feature/switch-on-span-char

Fix test

1627fec

jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 18, 2020

This comment was marked as resolved.

Sign in to view

jaredpar added this to the Compiler.Next milestone Aug 12, 2020

jaredpar self-assigned this Aug 12, 2020

YairHalberstadt added 2 commits October 8, 2020 21:13

merged from master

4b032ed

Add support for Span<T>

9ad9c31

RikkiGibson self-assigned this Oct 23, 2020

jaredpar reviewed Oct 23, 2020

View reviewed changes

RikkiGibson added 2 commits October 23, 2020 15:46

Merge branch 'master' of github.com:dotnet/roslyn into feature/switch…

3dc070e

…-on-span-char

Fix xlf

4bf35b5

This comment was marked as resolved.

Sign in to view

Fix formatting

be2ba8b

cston reviewed Jan 28, 2022

View reviewed changes

RikkiGibson reviewed Jan 29, 2022

View reviewed changes

cston reviewed Jan 31, 2022

View reviewed changes

cston reviewed Feb 1, 2022

View reviewed changes

cston mentioned this pull request Feb 1, 2022

Test plan for pattern matching on ReadOnlySpan<char> #59191

Closed

40 tasks

cston changed the base branch from main to features/patterns-span-char February 2, 2022 20:07

cston approved these changes Feb 8, 2022

View reviewed changes

RikkiGibson approved these changes Feb 8, 2022

View reviewed changes

cston merged commit d36edf5 into dotnet:features/patterns-span-char Feb 8, 2022

cston mentioned this pull request Feb 10, 2022

Address feedback from previous pattern matching ReadOnlySpan<char> change #59451

Merged

		LabelSymbol start = F.GenerateLabel("start");


		// This method should be kept consistent with SynthesizedStringSwitchHashMethod.ConstructStringHash

	public void PatternMatchReadonlySpanCharOnConstantString()
	public void PatternMatchReadOnlySpanCharOnConstantString()
	``` #Pending

Conversation

YairHalberstadt commented May 19, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open Questions

Uh oh!

jaredpar commented May 19, 2020

Uh oh!

This comment was marked as resolved.

Happypig375 commented May 22, 2020

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Happypig375 commented Aug 15, 2020

Uh oh!

YairHalberstadt commented Aug 15, 2020

Uh oh!

RikkiGibson commented Aug 15, 2020

Uh oh!

Happypig375 commented Aug 16, 2020

Uh oh!

YairHalberstadt commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Oct 23, 2020

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jan 28, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jan 28, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jan 28, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YairHalberstadt commented May 19, 2020 •

edited by jcouv

Loading

YairHalberstadt commented Oct 9, 2020 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

cston Jan 28, 2022 •

edited

Loading

RikkiGibson Jan 28, 2022 •

edited by cston

Loading

RikkiGibson Jan 28, 2022 •

edited by cston

Loading

RikkiGibson Jan 28, 2022 •

edited by cston

Loading

RikkiGibson Jan 29, 2022 •

edited by cston

Loading

RikkiGibson Jan 29, 2022 •

edited by cston

Loading

cston Jan 31, 2022 •

edited

Loading

cston Jan 31, 2022 •

edited

Loading

cston Jan 31, 2022 •

edited

Loading

cston Jan 31, 2022 •

edited

Loading

cston Jan 31, 2022 •

edited

Loading

cston Jan 31, 2022 •

edited

Loading

cston Feb 1, 2022 •

edited

Loading

cston Feb 1, 2022 •

edited

Loading