Skip to content

[release/9.0-staging][iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 - #112012

Merged
matouskozak merged 3 commits into
dotnet:release/9.0-stagingfrom
matouskozak:backport9.0/fix-indexOf-empty-strings
Feb 13, 2025
Merged

[release/9.0-staging][iOS][globalization] Fix IndexOf on empty strings on iOS to return -1#112012
matouskozak merged 3 commits into
dotnet:release/9.0-stagingfrom
matouskozak:backport9.0/fix-indexOf-empty-strings

Conversation

@matouskozak

@matouskozakmatouskozak commented Jan 30, 2025

Copy link
Copy Markdown
Member

Manual backport of #111898.

Fix incorrect return condition for iOS IndexOf implementation and add test case for IndexOf("", <something>) and "".Contains(<something>). Clean-up of some styling issues and adding comments.

Customer Impact

  • Customer reported
  • Found internally

Customers using CompareInfo.IndexOf or string.Contains APIs on empty source strings involving ICU logic are returning a result 0 (indicates found at index 0) instead of -1 (indicates not found).

Some of the issues reported by customers:

Regression

  • Yes
  • No

The regression was introduced in #86895 and initially limited to when hybrid globalization is enabled. However, in .NET 9, we switched fully to Apple native globalization APIs (initially also referred to as hybrid globalization), thus spreading this issue to all iOS Globalization code using the IndexOf API.

Testing

Previously there were no test cases for this code path. This PR adds test scenarios for both CompareInfo.IndexOf or string.Contains.

Risk

Low

This change is correcting previously incorrect behavior. It is unlikely that .NET 9 iOS customers are depending on this new behavior as it is fundamentally incorrect as per .net doc.

@tarekghtarekgh added the Servicing-consider Issue for next servicing release review label Jan 30, 2025
@tarekghtarekgh added this to the 9.0.3 milestone Jan 30, 2025
@matouskozak

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jeffschwMSFTjeffschwMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm. we will take for consideration in 9.0.x

@jeffschwMSFTjeffschwMSFT modified the milestones: 9.0.3, 9.0.xFeb 3, 2025
@matouskozak

Copy link
Copy Markdown
MemberAuthor

/ba -g the timeout on CoreCLR Windows x64 Debug is not related to this change

@rbhandarbhanda modified the milestones: 9.0.x, 9.0.3Feb 4, 2025
@rbhandarbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 4, 2025
@matouskozak

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@matouskozak

matouskozak commented Feb 11, 2025

Copy link
Copy Markdown
MemberAuthor

I had to disable the newly added test for wasm hybrid globalization due to an incorrect output 551ba2e (fyi: @ilonatommy) Rerunning the CI to confirm there is nothing else related. @jeffschwMSFT

@matouskozak

Copy link
Copy Markdown
MemberAuthor

The timeouts on osx-arm64 Release NativeAOT* jobs are not related and are already present on rolling-builds, e.g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=947948&view=results.

@matouskozak

Copy link
Copy Markdown
MemberAuthor

The runtime-extra-platforms failures are not related to this PR.

@matouskozak

Copy link
Copy Markdown
MemberAuthor

/ba-g timeouts are not related to this PR

@matouskozak
matouskozak merged commit 0eef239 into dotnet:release/9.0-stagingFeb 13, 2025
@akoeplingerakoeplinger modified the milestones: 9.0.3, 9.0.4Feb 17, 2025
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 20, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Globalizationos-iosApple iOSServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@matouskozak@jeffschwMSFT@vitek-karas@tarekgh@ilonatommy@akoeplinger@rbhanda