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 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 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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