Skip to content

Conversation

@Joy-less
Copy link
Contributor

Supercedes #111815.

This time the methods are fully implemented and ready to go, and the ref assemblies are added to match the implementations.

Copilot AI review requested due to automatic review settings June 30, 2025 17:58
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 30, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 30, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a set of new API overloads to support System.Text.Rune across various string and text manipulation APIs, addressing methods such as Equals, Contains, IndexOf/LastIndexOf, Replace, Trim, Append, Insert, and Write. The changes span multiple libraries including System.Runtime and System.Private.CoreLib to fully integrate rune-based operations.

  • Added overloads for string comparison methods accepting System.Text.Rune.
  • Introduced new methods on StringBuilder, TextWriter, and TextInfo to handle rune conversion and manipulation.
  • Updated internal string splitting and trimming routines to support rune separators and trim characters.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/System.Runtime/ref/System.Runtime.cs Added new overloads for string API methods accepting System.Text.Rune and StringComparison.
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs Added Append/Insert/Replace methods with System.Text.Rune and Rune retrieval overloads.
src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs Updated Equals implementations and added new overloads for comparing Runes with StringComparison.
src/libraries/System.Private.CoreLib/src/System/String.Searching.cs Provided new overloads of Contains, IndexOf, and LastIndexOf for System.Text.Rune with careful iteration over surrogates.
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs Added Replace, Split, and Trim overloads using System.Text.Rune for enhanced string manipulation.
src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs Extended StartsWith and EndsWith to support System.Text.Rune comparisons.
src/libraries/System.Private.CoreLib/src/System/IO/TextWriter.cs Integrated Write and WriteLine overloads that accept System.Text.Rune.
src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs Implemented ToLower and ToUpper for System.Text.Rune conversions.
src/libraries/System.Private.CoreLib/src/System/Char.cs Added an overload for Equals accepting a StringComparison using modern array initialization.
Comments suppressed due to low confidence (1)

src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:2850

  • In GetRuneAt(int index), consider throwing a more specific exception type (such as ArgumentOutOfRangeException) rather than a generic Exception for improved clarity and consistency with API design.
            throw new Exception($"Unable to get rune at {index}.");

@jeffhandley
Copy link
Member

@stephentoub - Since this has been open as long as it has, I wanted to check with you that we should still pursue adding the Rune overloads. Any concerns or hesitations?

@Joy-less
Copy link
Contributor Author

Joy-less commented Sep 5, 2025

@jeffhandley I have made some changes to my PR:

  • Fixed TextInfo.ToLower(Rune) and TextInfo.ToUpper(Rune) for runes which use two chars. Very niche but affects some characters like 𐐨 (\uD801\uDC28) -> 𐐀 (\uD801\uDC00).
  • Used the ordinal char.Equals(char) implementation for char.Equals(char, StringComparison) when StringComparison is StringComparison.Ordinal.

I think #111170 is a relevant pull request to this one.

Also, the original creator of the API proposals for both this pull request and the above pull request has this on their profile:

On other projects, not checking GitHub notifications - ping via Teams if urgent.

@tarekgh
Copy link
Member

tarekgh commented Sep 5, 2025

@Joy-less I'll get into this PR in the next few days. Thanks!

@Joy-less
Copy link
Contributor Author

@Joy-less I'll get into this PR in the next few days. Thanks!

@tarekgh Any update?

@tarekgh
Copy link
Member

tarekgh commented Sep 25, 2025

Looks I accidentality merged with unrealted main changes. I'll try to fix that.

@tarekgh tarekgh removed the linkable-framework Issues associated with delivering a linker friendly framework label Sep 25, 2025
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Thanks, @Joy-less, for all your effort in making this change.

@jkotas
Copy link
Member

jkotas commented Sep 26, 2025

This change introduced failing tests that are causing many legs to fail on all PRs now. I am submitting a revert (unless these failures get fixed quickly). Example of a failure #120137 , but there may be more.

Build analysis have not flagged these failures for some reason.

@Joy-less
Copy link
Contributor Author

Joy-less commented Sep 26, 2025

This change introduced failing tests that are causing many legs to fail on all PRs now. I am submitting a revert (unless these failures get fixed quickly). Example of a failure #120137 , but there may be more.

Build analysis have not flagged these failures for some reason.

I see the issue with that failing test, it's doing reflection to check that all methods are overridden on the NullTextWriter. I can create a pull request ASAP to fix that failing test if that's the only one.

Edit: Please see #120145.

Joy-less added a commit to Joy-less/runtime that referenced this pull request Sep 26, 2025
tarekgh added a commit that referenced this pull request Sep 27, 2025
…#27912 (Flow System.Text.Rune through more APIs)) (#120145)

* Fix tests from #117168

* Add `SyncTextWriter` overloads as well

* Add missing overloads to BroadcastingTextWriter

* Reapply "Add methods from #27912 (Flow System.Text.Rune through more APIs) (#1…" (#120138)

This reverts commit be80737.

* Override the TextWrite Rune overloads in IndentedTextWriter

---------

Co-authored-by: Tarek Mahmoud Sayed <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Runtime community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants