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

Revamp proposal for list patterns on enumerables #9165

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 22, 2025

Someone with edit rights, please edit the proposal link in #9005 to https://github.com/dotnet/csharplang/blob/main/proposals/list-patterns-on-enumerables-2.md and edit the summary to:

Extends list patterns to be able to be used with enumerables that are not countable or indexable. `items.Where(...) is [var singleItem]`, or `is []`, or `is [p1, .., p2]`.

The pattern will be evaluated without multiple enumeration. The slice pattern `..` is supported, but only without a subpattern.

cc: @jcouv

@jnm2 jnm2 requested a review from a team as a code owner February 22, 2025 03:42
@jnm2
Copy link
Contributor Author

jnm2 commented Feb 22, 2025

@alrz If you're available, your feedback is welcome (I heard you had an impl at one point)

@alrz
Copy link
Member

alrz commented Mar 1, 2025

Can I ask in what ways this is indented to differ from the original? The prototype lives at dotnet/roslyn#66553 based on the initial design (except for the buffer type contract explained in the description).

I've recorded open questions as PROTOTYPE comments there but I don't know how it maps to this updated version now.

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 1, 2025

@alrz It differs in two ways. 1) It recommends disallowing subpatterns on the slice pattern, even if the enumerable is sliceable. 2) It uses the enumerator directly like the foreach statement does, rather than a helper type.

Comment on lines +31 to +33
#### No multiple enumeration

Enumerables cannot be assumed to represent a materialized collection. An enumerable may represent an in-memory collection or a generated sequence, but it may also represent a remote query or an iterator method. As such, an enumerable may return different results on each enumeration, and enumeration may have side effects. Multiple enumeration is considered both a performance smell and a correctness issue in cases where the enumerable is not known to be a materialized collection or a trivially generated, guaranteed-stable sequence. The .NET SDK and other popular tools produce warnings for multiple enumeration of the same enumerable.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs consideration: is the no-multiple-enumeration guarantee even possible when switching on the enumerable?

Copy link
Contributor Author

@jnm2 jnm2 Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I also don't think any of these scenarios would necessarily absolutely be expected to reduce multiple enumeration compared to each other:

enumerable is [1] or [2, 3]
enumerable is [1] || enumerable is [2, 3]
enumerable.SequenceEqual([1]) || enumerable.SequenceEqual([2, 3])

The existing analyzers that warn about multiple enumeration in the last case can be augmented to see it happening in the prior two cases as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler may be able to guarantee no multiple enumeration though. And then maybe is [1] or [1, 2] may not have to enumerate multiple times, and maybe this is worth guaranteeing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enumerable is [1] || enumerable is [2, 3]

I think that must result in multiple enumerations because the || is not pattern matching. Basically it's two independent pattern matching expressions each of which has single enumeration of enumerable.

enumerable is [1] or [2, 3]

This though is a case where we'd want to avoid multiple enumerations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler may be able to guarantee no multiple enumeration though.

Think that would be important for us to support list patterns on IEnumerable<T>. Basically within a single pattern expression or switch there should be only a single enumeration of the enumerable. Once you break out of the pattern, with say an ||, then all bets are off.

@333fred
Copy link
Member

333fred commented Mar 2, 2025

@alrz It differs in two ways. 1) It recommends disallowing subpatterns on the slice pattern, even if the enumerable is sliceable. 2) It uses the enumerator directly like the foreach statement does, rather than a helper type.

1 feels like it should just be added as an open question. 2 seems like a negative change, we moved to a helper type because the ldm had expressed some passing concern on codegen size. Neither of these feels like it should be a distinct proposal document; if anything, let's keep history intact by proposing updates to the existing document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants