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

[API Proposal]: MemoryExtensions.ContainsAny{Except} #86528

Closed
MihaZupan opened this issue May 19, 2023 · 8 comments · Fixed by #88394
Closed

[API Proposal]: MemoryExtensions.ContainsAny{Except} #86528

MihaZupan opened this issue May 19, 2023 · 8 comments · Fixed by #88394
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented May 19, 2023

Background and motivation

Similar to how we have span.IndexOf(T) and span.Contains(T), I'm proposing a contains variant of IndexOfAny{Except}.
These methods would be semantically equivalent to IndexOfAny{Except} >= 0.

The motivation behind the API is similar to the original Contains (#25228, #33785 (comment)):

  • we can avoid some costs if we know that the caller doesn't care about the exact position of a match
  • ContainsAny/!ContainsAny is more readable than various combinations of IndexOfAny + >= 0 < 0 != -1 ...
Example performance numbers for IndexOfAny vs ContainsAny for a set of Ascii values
Method Length MatchAtStart Mean
IndexOfAny 1 False 1.642 ns
ContainsAny 1 False 1.206 ns
IndexOfAny 1 True 1.828 ns
ContainsAny 1 True 1.341 ns
IndexOfAny 7 False 7.455 ns
ContainsAny 7 False 5.057 ns
IndexOfAny 7 True 1.873 ns
ContainsAny 7 True 1.377 ns
IndexOfAny 8 False 2.207 ns
ContainsAny 8 False 2.064 ns
IndexOfAny 8 True 3.267 ns
ContainsAny 8 True 2.459 ns
IndexOfAny 16 False 2.189 ns
ContainsAny 16 False 2.033 ns
IndexOfAny 16 True 3.256 ns
ContainsAny 16 True 2.465 ns
IndexOfAny 32 False 2.908 ns
ContainsAny 32 False 2.613 ns
IndexOfAny 32 True 3.909 ns
ContainsAny 32 True 2.283 ns
IndexOfAny 64 False 3.445 ns
ContainsAny 64 False 3.155 ns
IndexOfAny 64 True 3.261 ns
ContainsAny 64 True 1.942 ns

API Proposal

namespace System;

public static class MemoryExtensions
{
    // Existing
    public static int IndexOf<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?;
    public static bool Contains<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?;
    public static int IndexOfAny<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static int IndexOfAnyExcept<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static int IndexOfAny(this ReadOnlySpan<char> span, SearchValues<string> values);

    // Proposed
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAny(this ReadOnlySpan<char> span, SearchValues<string> values);
}

API Usage

-if (text.IndexOfAny(s_invalidChars) >= 0)
+if (text.ContainsAny(s_invalidChars))

-if (text.IndexOfAnyExcept(s_allowedChars) >= 0)
+if (text.ContainsAnyExcept(s_allowedChars))

Alternative Designs

IndexOfAny has many overloads and variants. Should any of them also have ContainsAny variants?

static bool ContainsAnyExcept(this ReadOnlySpan<T> span, T value);
static bool ContainsAny{Except}(this ReadOnlySpan<T> span, T value0, T value1);
static bool ContainsAny{Except}(this ReadOnlySpan<T> span, T value0, T value1, value2);
static bool ContainsAny{Except}(this ReadOnlySpan<T> span, ReadOnlySpan<T> values);
static bool ContainsAny{Except}InRange(this ReadOnlySpan<T> span, T lowInclusive, T highInclusive);
static bool ContainsAny{Except}WhiteSpace(this ReadOnlySpan<T> span);
static bool ContainsNewLine(ReadOnlySpan<char> value) =>
-   value.IndexOfAny('\r', '\n') >= 0;
+   value.ContainsAny('\r', '\n');

Risks

More methods to think about - ContainsAny/ContainsAnyExcept would be a new set of overloads on a common type.

@MihaZupan MihaZupan added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory labels May 19, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 19, 2023
@ghost
Copy link

ghost commented May 19, 2023

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Similar to how we have span.IndexOf(T) and span.Contains(T), I'm proposing a contains variant of IndexOfAny{Except}.
These methods would be semantically equivalent to IndexOfAny{Except} >= 0.

The motivation behind the API is similar to the original Contains (#25228, #33785 (comment)):

  • we can avoid some costs if we know that the caller doesn't care about the exact position of a match
  • ContainsAny/!ContainsAny is more readable than various combinations of IndexOfAny + >= 0 < 0 != -1 ...
Example performance numbers for IndexOfAny vs ContainsAny for a set of Ascii values
Method Length MatchAtStart Mean
IndexOfAny 1 False 1.642 ns
ContainsAny 1 False 1.206 ns
IndexOfAny 1 True 1.828 ns
ContainsAny 1 True 1.341 ns
IndexOfAny 7 False 7.455 ns
ContainsAny 7 False 5.057 ns
IndexOfAny 7 True 1.873 ns
ContainsAny 7 True 1.377 ns
IndexOfAny 8 False 2.207 ns
ContainsAny 8 False 2.064 ns
IndexOfAny 8 True 3.267 ns
ContainsAny 8 True 2.459 ns
IndexOfAny 16 False 2.189 ns
ContainsAny 16 False 2.033 ns
IndexOfAny 16 True 3.256 ns
ContainsAny 16 True 2.465 ns
IndexOfAny 32 False 2.908 ns
ContainsAny 32 False 2.613 ns
IndexOfAny 32 True 3.909 ns
ContainsAny 32 True 2.283 ns
IndexOfAny 64 False 3.445 ns
ContainsAny 64 False 3.155 ns
IndexOfAny 64 True 3.261 ns
ContainsAny 64 True 1.942 ns

API Proposal

namespace System;

public static class MemoryExtensions
{
    // Existing
    public static int IndexOf<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?;
    public static bool Contains<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?;
    public static int IndexOfAny<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static int IndexOfAnyExcept<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static int IndexOfAny(this ReadOnlySpan<char> span, SearchValues<string> values);

    // Proposed
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAny(this ReadOnlySpan<char> span, SearchValues<string> values);
}

API Usage

-if (text.IndexOfAny(s_invalidChars) >= 0)
+if (text.ContainsAny(s_invalidChars))

-if (text.IndexOfAnyExcept(s_allowedChars) >= 0)
+if (text.ContainsAnyExcept(s_allowedChars))

Alternative Designs

IndexOfAny has many overloads and variants. Should any of them also have ContainsAny variants?

static bool ContainsAny{Except}(this ReadOnlySpan<T> span, T value0, T value1);
static bool ContainsAny{Except}(this ReadOnlySpan<T> span, T value0, T value1, value2);
static bool ContainsAny{Except}(this ReadOnlySpan<T> span, ReadOnlySpan<T> values);
static bool ContainsAny{Except}InRange(this ReadOnlySpan<T> span, T min, T max);
static bool ContainsNewLine(ReadOnlySpan<char> value) =>
-   value.IndexOfAny('\r', '\n') >= 0;
+   value.ContainsAny('\r', '\n');

Risks

More methods to think about - ContainsAny/ContainsAnyExcept would be a new set of overloads on a common type.

Author: MihaZupan
Assignees: -
Labels:

api-suggestion, area-System.Memory

Milestone: -

@stephentoub
Copy link
Member

Which of the places we're currently using SearchValues with IndexOfAny{Except} would be replaceable/improved with the proposed methods?

@MihaZupan
Copy link
Member Author

In System.Net:

Other:

AspNetCore (some places would likely switch from IndexOfAny to ContainsAny on the fast + IndexOfAny on the slow path):

@stephentoub
Copy link
Member

Thanks. So a reasonable percentage of our current IndexOfAny use with SearchValues could now use ContainsAny. Would we be able to share most of the code in the SearchValues implementations?

@MihaZupan
Copy link
Member Author

MihaZupan commented May 22, 2023

By abusing generics some more, yes: MihaZupan@2dec12f#diff-1616491faf2fad45e66a3301f2ed62e37e1c6454605ee94e2210913d58b32988
(that is if we wanted a specialized implementation at all vs. just the convenience of the API)

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels May 26, 2023
@stephentoub stephentoub added this to the 8.0.0 milestone Jun 6, 2023
@bartonjs
Copy link
Member

bartonjs commented Jun 8, 2023

Video

  • We need to overload across ReadOnlySpan and Span, since extension methods don't apply to implicit conversions.
  • We decided to go ahead and square the circle and approve a Contains version of everything that has an IndexOfAny
namespace System;

public static partial class MemoryExtensions
{
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAny(this ReadOnlySpan<char> span, SearchValues<string> values);

    public static bool ContainsAny<T>(this Span<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAnyExcept<T>(this Span<T> span, SearchValues<T> values) where T : IEquatable<T>?;
    public static bool ContainsAny(this Span<char> span, SearchValues<string> values);

    // If any overloads of IndexOfAny{Except} are missing, consider them approved, too.
    // NOTE: Generic constraints are missing in these methods, because they were not reflected in the proposal.
    // The constraints should match the IndexOfAny versions
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, T value);
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, T value0, T value1);
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, T value0, T value1, T value2);
    public static bool ContainsAny<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> values);
    public static bool ContainsAnyInRange<T>(this ReadOnlySpan<T> span, T lowInclusive, T highInclusive);
    public static bool ContainsAnyWhiteSpace(this ReadOnlySpan<char> span);
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, T value0, T value1);
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, T value0, T value1, T value2);
    public static bool ContainsAnyExcept<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> values);
    public static bool ContainsAnyExceptInRange<T>(this ReadOnlySpan<T> span, T lowInclusive, T highInclusive);
    public static bool ContainsAnyExceptWhiteSpace(this ReadOnlySpan<char> span);

    public static bool ContainsAnyExcept<T>(this Span<T> span, T value);
    public static bool ContainsAny<T>(this Span<T> span, T value0, T value1);
    public static bool ContainsAny<T>(this Span<T> span, T value0, T value1, T value2);
    public static bool ContainsAny<T>(this Span<T> span, ReadOnlySpan<T> values);
    public static bool ContainsAnyInRange<T>(this Span<T> span, T lowInclusive, T highInclusive);
    public static bool ContainsAnyExcept<T>(this Span<T> span, T value0, T value1);
    public static bool ContainsAnyExcept<T>(this Span<T> span, T value0, T value1, T value2);
    public static bool ContainsAnyExcept<T>(this Span<T> span, ReadOnlySpan<T> values);
    public static bool ContainsAnyExceptInRange<T>(this Span<T> span, T lowInclusive, T highInclusive);

}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 8, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 23, 2023
@tannergooding
Copy link
Member

@stephentoub, @MihaZupan; same for this. Is this critical for .NET 8 or is it fine if it slips to .NET 9?

@MihaZupan
Copy link
Member Author

MihaZupan commented Jul 24, 2023

This one was effectively done with #87621 (one overload depends on #85573).
If #88394 doesn't make 8.0, I'll update the description of #85573 to include ContainsAny, and close this one.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants