Update list-patterns.md #4790

alrz · 2021-05-28T14:15:26Z

Record recent LDM decisions and sync with the current impl.

Linking to test plan: dotnet/roslyn#51289

alrz · 2021-05-28T14:20:26Z

proposals/list-patterns.md

@@ -153,7 +148,7 @@ case {.., 1, _}: // expr.Length is >= 2 && expr[^2] is 1

 The order in which subpatterns are matched at runtime is unspecified, and a failed match may not attempt to match all subpatterns.

-> **Open question**: The pattern `{..}` tests for  `expr.Length >= 0`. Should we omit such test (assuming `Length` is always non-negative)?
+> **Open question**: By this definition, the pattern `{..}` tests for `expr.Length >= 0`. Should we omit such test, assuming `Length` is always non-negative? (answer [LDM 2021-05-26]: `{ .. }` will not emit a Length check)


📝 Notes were clear that this is about semantics and not an optimization.

alrz · 2021-05-28T14:24:44Z

@333fred @jcouv for review. thanks.

(I didn't touch enumerable sections but I think we should move it to its own document eventually)

alrz · 2021-05-28T14:29:42Z

proposals/list-patterns.md

+A *slice_pattern* with a subpattern is compatible with any type that is *countable* as well as *sliceable* — it has an accessible indexer that takes a `Range` argument or otherwise an accessible `Slice` method with two `int` parameters. If both are present, the former is preferred.
+A *slice_pattern* without a subpattern is compatible with any type that is compatible with a *list_pattern*.


📝 We shouldn't require Slice method if there is no subpattern. This'd be consistent with the decision about {..}.

alrz · 2021-05-28T14:30:20Z

proposals/list-patterns.md

- : '..' negated_pattern?
+ : '..' pattern?


Wasn't mentioned in LDM. But I think "all patterns" includes combinators. #Resolved

It does. I'm not familiar enough with the various levels of the pattern grammar, does negated_pattern not include combinators?

No it doesn't. I've started this as an unary operator, hence the precedence.

nit: Consider adding links to previous patterns specs:

https://github.com/dotnet/csharplang/blob/main/proposals/csharp-7.0/pattern-matching.md
https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/patterns.md
https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/patterns3.md

333fred · 2021-05-28T16:42:30Z

proposals/list-patterns.md

-A *slice_pattern* is compatible with any type that is *countable* as well as *sliceable* — it has an accessible indexer that takes a `Range` argument or otherwise an accessible `Slice` method that takes two `int` arguments. If both are present, the former is preferred. 
-A *slice_pattern* without a subpattern is also compatible with any type that is *enumerable*.
+A *slice_pattern* with a subpattern is compatible with any type that is *countable* as well as *sliceable* — it has an accessible indexer that takes a `Range` as an argument or otherwise an accessible `Slice` method with two `int` parameters. If both are present, the former is preferred.
+A *slice_pattern* without a subpattern is compatible with any type that is compatible with a *list_pattern*.


Might need to mention discard subpattern?

With this change, we have:

{..} works even without this[Range] or a proper Slice method.

{.._} requires this[Range]/Slice. The codegen, however, is permitted to omit the invocation (which we do). That is not limited to discards or the slice pattern, we skip an evaluation when the output is unused. ~~e.g. with an unused variable etc.~~

Sounds like we currently don't include unused variables here, but it's something that could be done as an optimization.

Here's an example to demonstrate it's not limited to discard: if (this is {IntProp: {}}){} IntProp is not called here.

The codegen, however, is permitted to omit the invocation (which we do).

This is a good distinction, but I do think it is good to make this not an optimization: it gets back to making sure that behavior of something like default(ImmutableArray<int>) is { .._ } is well-defined.

At the moment the behavior of default(ImmutableArray<int>) is { .._ } is an error. "unsupported type for slice pattern"

ImmutableArray doesn't expose a Slice method or range indexer.

Yeah, but additions are tracked by dotnet/runtime#22928.

My main concern is that when a type is not sliceable, we should be able to do {1, ..} etc. Not every list should be required to be sliceable only to be able to skip elements in the list pattern.

I'm fine with the behavior as spec'ed in this PR. I'd let .._ and ..var unused require a sliceable type. It's conceptually simpler.

For { ..var unused }: during binding we don't know that var unused is unused.
For { .._ }: @333fred , I can add a note to test plan to rediscuss if you think this should not require a sliceable type.

For { .._ }

Just like we still need an indexable type for {_} that we're not going to use, {.._} should require a sliceable type that we may not use. I think the part that we decide to not evaluate the method, is orthogonal to binding rules.

alrz · 2021-05-28T18:13:56Z

To be clear, I don't think we should do anything specific for ImmutableArray, it's not a known type in the compiler and I don't believe it should influence the design. However, it's a use case that we definitely want to cover in this area.

To that end, I've proposed to use a hypothetical bool Deconstruct (already championed in some form) so that we can safely match an ImmutableArray whether or not it's initialized. I'm not sure if we want to go down that road though. But checking IsDefault is too type specific IMO, as far as I know, that doesn't have any special meaning anywhere in the language or BCL in general.

jcouv

LGTM Thanks (iteration 2)

jcouv · 2021-05-28T18:18:11Z

I don't think we should do anything specific for ImmutableArray,

Agreed.
FYI, based on email discussion, I suspect there will be a formal proposal soon to add a null/default operator. But that'll be orthogonal to the list-patterns feature.

jcouv · 2021-05-28T18:49:58Z

@333fred I started thread with LDM mentioning the { .._ } question. Feel free to chime in. I think we can merge this PR if you're good with it (I am).

jcouv · 2021-05-28T22:44:21Z

Merged/squashed. Thanks @alrz

Update list-patterns.md

6f61854

alrz requested a review from a team as a code owner May 28, 2021 14:15

alrz commented May 28, 2021

View reviewed changes

Update list-patterns.md

b253865

alrz mentioned this pull request May 28, 2021

List patterns: binding and lowering dotnet/roslyn#53299

Merged

333fred reviewed May 28, 2021

View reviewed changes

jcouv self-assigned this May 28, 2021

jcouv approved these changes May 28, 2021

View reviewed changes

333fred approved these changes May 28, 2021

View reviewed changes

jcouv merged commit bcd1ace into dotnet:main May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update list-patterns.md #4790

Update list-patterns.md #4790

alrz commented May 28, 2021 •

edited by jcouv

Loading

alrz May 28, 2021

alrz commented May 28, 2021

alrz May 28, 2021 •

edited

Loading

alrz May 28, 2021 •

edited by jcouv

Loading

333fred May 28, 2021

alrz May 28, 2021

jcouv May 28, 2021

333fred May 28, 2021

alrz May 28, 2021 •

edited

Loading

alrz May 28, 2021

alrz May 28, 2021

333fred May 28, 2021

alrz May 28, 2021

333fred May 28, 2021

alrz May 28, 2021

jcouv May 28, 2021

alrz May 28, 2021

alrz commented May 28, 2021

jcouv left a comment

jcouv commented May 28, 2021

jcouv commented May 28, 2021

jcouv commented May 28, 2021

		A slice_pattern with a subpattern is compatible with any type that is countable as well as sliceable — it has an accessible indexer that takes a `Range` argument or otherwise an accessible `Slice` method with two `int` parameters. If both are present, the former is preferred.
		A slice_pattern without a subpattern is compatible with any type that is compatible with a list_pattern.

Update list-patterns.md #4790

Update list-patterns.md #4790

Conversation

alrz commented May 28, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz commented May 28, 2021

alrz May 28, 2021 • edited Loading

Choose a reason for hiding this comment

alrz May 28, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz commented May 28, 2021

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented May 28, 2021

jcouv commented May 28, 2021

jcouv commented May 28, 2021

alrz commented May 28, 2021 •

edited by jcouv

Loading

alrz May 28, 2021 •

edited

Loading

alrz May 28, 2021 •

edited by jcouv

Loading

alrz May 28, 2021 •

edited

Loading