Implement collation native functions functions #86895

mkhamoyan · 2023-05-30T13:37:32Z

Implements a chunk of apple native-api based globalization.

Old, icu-based private API:

GlobalizationNative_StartsWith
GlobalizationNative_EndsWith
GlobalizationNative_IndexOf
GlobalizationNative_LastIndexOf

New, non-icu private API:

GlobalizationNative_StartsWithNative
GlobalizationNative_EndsWithNative
GlobalizationNative_IndexOfNative // will be called also for LastIndexOf

Affected public API :

CompareInfo.IsPrefix
CompareInfo.IsSuffix
String.StartsWith
String.EndsWith
CompareInfo.IndexOf
CompareInfo.LastIndexOf
String.IndexOf
String.LastIndexOf

APIs that will not be supported

CompareInfo.GetSortKey
CompareInfo.GetSortKeyLength
CompareInfo.GetHashCode

Fixes #86699
Contributes to #80689
cc @SamMonoRT

ghost · 2023-05-30T13:37:43Z

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

Issue Details

null

Author:	mkhamoyan
Assignees:	mkhamoyan
Labels:	`area-System.Globalization`
Milestone:	-

ghost · 2023-05-30T16:10:37Z

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	mkhamoyan
Assignees:	mkhamoyan
Labels:	`NO-REVIEW`, `area-System.Globalization`, `os-mac-os-x`, `os-ios`, `os-tvos`
Milestone:	-

mdh1418 · 2023-06-16T16:58:33Z

src/libraries/Common/src/Interop/Interop.Collation.OSX.cs

@@ -12,5 +12,18 @@ internal static partial class Globalization
    {
        [LibraryImport(Libraries.GlobalizationNative, EntryPoint = "GlobalizationNative_CompareStringNative", StringMarshalling = StringMarshalling.Utf16)]
        internal static unsafe partial int CompareStringNative(string localeName, int lNameLen, char* lpStr1, int cwStr1Len, char* lpStr2, int cwStr2Len, CompareOptions options);
+
+        [LibraryImport(Libraries.GlobalizationNative, EntryPoint = "GlobalizationNative_EndsWithNative", StringMarshalling = StringMarshalling.Utf16)]
+        [MethodImpl(MethodImplOptions.NoInlining)]


Out of curiosity, why are we adding NoInlining here and on StartsWithNative but not on the other ones?

I did like is done for ICU https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/Interop/Interop.Collation.cs#L34

But not sure why only these 2 functions have MethodImplOptions.NoInlining

My guess is that it is to avoid regressing the hot path of the method by inlining the PInvoke.

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs

src/native/libs/System.Globalization.Native/pal_collation.m

mkhamoyan · 2023-06-19T17:14:01Z

/azp run runtime-ioslike

azure-pipelines · 2023-06-19T17:14:09Z

Azure Pipelines successfully started running 1 pipeline(s).

src/native/libs/System.Globalization.Native/pal_collation.m

mdh1418 · 2023-06-19T17:04:56Z

docs/design/features/globalization-hybrid-mode.md

+Mapped to Apple Native API `rangeOfString:options:range:locale:`(https://developer.apple.com/documentation/foundation/nsstring/1417348-rangeofstring?language=objc)
+
+In `rangeOfString:options:range:locale:` objects are compared by checking the Unicode canonical equivalence of their code point sequences.
+In cases where search string contains diaeresis and has different normalization form than in source string result can be incorrect.


This isn't limited to just the diaeresis diacritic mark is it? If it isn't limited to just diaeresis can we change it to diacritics.

mdh1418 · 2023-06-19T18:14:09Z

docs/design/features/globalization-hybrid-mode.md

+      Search string contains diaeresis and with source string they have same letters with different char lengths but substring is not
+      normalized in source. example: search string: `U\u0308 and \u00FC` source string: `Source is a\u0308\u0308a and \u0075\u0308`
+      as it is visible from example normalizaing search string to form C or D will not help to find substring in source string.


I'm not sure if I'm understanding this.
searchString U\u0308 and \u00FC (Ü and ü)
source string a\u0308\u0308a and \u0075\u0308 (ä̈a and ü)
Is the search string supposed to be a substring in the source string? Was it supposed to be U\u0308 and \u00FC should both be interpreted as substrings of a\u0308\u0308a and (\u0075\u0308) in that it matches the portion in the parenthesis?

I was actually expecting the not covered cases to be something like, source string is a sequence of unicode code points that represent a mixture of precomposed characters and decomposed characters.
i.e. Source string H\u00EBll\u006F\u0308 World (Hëllö World)
Search string \u0065\u0308ll\u00F6 (ëllö)
These would bypass the precomposed vs precomposed comparison because it would be source
string H\u00EBll\u00F6 World and search string \u00EBll\u00F6
However, neither the fully precomposed form of the search string \u00EBll\u00FB nor the fully decomposed form of the search string \u0065\u0308ll\u006F\u0308 would match the unicode code point sequence in the source string \u00EBll\u006F\u0308 so it would fall through the Form C and Form D fallbacks.

My bad. Source should be "Source is \u00DC and \u0075\u0308".
search string: U\u0308 and \u00FC (Ü and ü) source string: Source is \u00DC and \u0075\u0308 (Source is Ü and ü). Will update it.

mdh1418 · 2023-06-19T18:24:09Z

src/native/libs/System.Globalization.Native/pal_collation.m

+    // localizedStandardRangeOfString is performing a case and diacritic insensitive, locale-aware search and finding first occurance.
+    if ((comparisonOptions & IgnoreCase) && lNameLength == 0 && fromBeginning)
+    {      
+        NSRange localizedStandardRange = [sourceStrCleaned localizedStandardRangeOfString:searchStrCleaned];
+        if (localizedStandardRange.location != NSNotFound)
+        {
+            result.location = localizedStandardRange.location;
+            result.length = localizedStandardRange.length;                    
+            return result;
+        }       


Migrating comment

localizedStandardRangeOfString is performing case and diacritic insensitive, locale-aware search by implementation, that is why I am checking for IgnoreCase and if locale is not passed .(see https://developer.apple.com/documentation/foundation/nsstring/1413574-localizedstandardrangeofstring)

Right, I see that it finds the first match with a case insensitive and diacritic insensitive search, but are the other 7 permutations of these options (!(compareOptions & IgnoreCase) || lNameLength != 0 || !fromBeginning) covered by the following logic?

I'm wondering if this block is truly necessary. If ((comparisonOptions & IgnoreCase) && lNameLength == 0 && !fromBeginning) is covered by the following logic, I would imagine that this case is covered as well without this block.

Why does lNameLength == 0 mean locale-aware? If its supposed to be the system locale [NSLocale systemLocale], then shouldn't this check for localeName == NULL || lNameLength == 0

Wouldn't the IgnoreNonSpace be the diacritic insensitive condition as well?

localizedStandardRangeOfString is not necessary , if it makes the function more complicated I will remove it.

I think the block suggested to me that localizedStandardRangeOfString could cover one specific case that the rest of the following logic couldn't, which felt odd specifically because that this wouldn't be hit if fromBeginning was false. Another consideration is that it seems to ignore the rest of the flags of options besides case insensitive and diacritic insensitive, so I wonder if this block would have been correct in those scenarios (if we had the test cases for them). I think it could make sense to keep it if it was much faster than rangeOfString:options:range:locale:, but it seems cleaner to rely on the following logic if it can be handled correctly.

Now it handles correctly all cases without localizedStandardRangeOfString.

src/native/libs/System.Globalization.Native/pal_collation.m

mkhamoyan · 2023-06-20T05:38:17Z

/azp run runtime-ioslike

azure-pipelines · 2023-06-20T05:38:25Z

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418

Looks good to me! Thanks for your patience!

mdh1418 · 2023-06-20T15:05:48Z

docs/design/features/globalization-hybrid-mode.md

+  1. Search string contains diaeresis and has same normalization form as in source string.
+  2. Search string contains diaeresis but with source string they have same letters with different char lengths but substring is normalized in source.


nit: diacritic

mdh1418 · 2023-06-20T15:14:05Z

docs/design/features/globalization-hybrid-mode.md

+      Search string contains diacritics and with source string they have same letters with different char lengths but substring is not
+      normalized in source. example: search string: `U\u0308 and \u00FC` (Ü and ü) source string: `Source is \u00DC and \u0075\u0308` (Source is Ü and ü)


nit: Source string's intended substring match containing characters of mixed composition forms cannot be matched by 2. because partial precomposition/decomposition is not performed.

mdh1418 · 2023-06-20T16:08:23Z

src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs

@@ -107,13 +107,16 @@ public static IEnumerable<object[]> IsSuffix_TestData()
            {
                yield return new object[] { s_hungarianCompare, "foobardzsdzs", "rddzs", CompareOptions.None, false, 0 };
                yield return new object[] { s_frenchCompare, "\u0153", "oe", CompareOptions.None, false, 0 };
-                yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uDC00", CompareOptions.None, false, 0 };
-                yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uDC00", CompareOptions.IgnoreCase, false, 0 };
+                if (!PlatformDetection.IsHybridGlobalizationOnOSX) // TODO: check this for OSX


Is this still a todo?

mdh1418 · 2023-06-20T16:14:57Z

src/native/libs/System.Globalization.Native/pal_collation.m

+        return result;
+
+    // in case search string is inside source string but we can't find the index return -2
+    result.location = -2;


This may conflict with the Debug.Assert(result.Location != -2); in CompareInfo.OSX.cs because we might actually hit -2 if we hit the unsupported mixed composition source string case. Perhaps we can use something different to indicate that this is a case that is currently unsupported.

For this case changed return value to -3 and added exception.

jkotas · 2023-06-20T16:20:28Z

src/libraries/Common/src/Interop/OSX/NSRange.cs

@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+using System.Runtime.InteropServices;


Suggested change

using System.Runtime.InteropServices;

Unnecessary

jkotas · 2023-06-20T16:21:55Z

src/libraries/Common/src/Interop/OSX/NSRange.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+using System.Runtime.InteropServices;
+namespace System.Globalization


Suggested change

namespace System.Globalization

internal static partial class Interop

This type is not specific to globalization. It should have neutral namespace.

I see the unmanaged side is not returning the NSRange type in the latest iteration. It returns its own Range type. In that case, it should not be called NSRange. It should be called Range to match the unmanaged side and it should be in System.Globalization.iOS.cs.

I'm using NSRange https://developer.apple.com/documentation/foundation/nsrange?language=objc on pal_collation.m,
but to make it compiled on https://github.com/dotnet/runtime/blob/main/src/native/libs/System.Globalization.Native/pal_collation.h I needed to add Range type.
Do you mean Range struct should be added in System.Globalization.iOS.cs ? I couldn't find that file

I meant src/libraries/Common/src/Interop/Interop.Collation.OSX.cs

mkhamoyan · 2023-06-21T05:52:24Z

/azp run runtime-ioslike

azure-pipelines · 2023-06-21T05:52:31Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-06-21T08:17:28Z

Failures are not related.

Implemented IndexOf and LastIndexOf functions

164bb0a

dotnet-issue-labeler bot added the area-System.Globalization label May 30, 2023

ghost assigned mkhamoyan May 30, 2023

mkhamoyan added the NO-REVIEW Experimental/testing PR, do NOT review it label May 30, 2023

tarekgh added os-mac-os-x macOS aka OSX os-ios Apple iOS os-tvos Apple tvOS labels May 30, 2023

build-analysis bot mentioned this pull request May 30, 2023

Tracking issue for CI build timeouts #76454

Closed

mkhamoyan added 2 commits May 31, 2023 09:53

Updated test cases

1e4e9a5

Remove not needed parts

292b915

build-analysis bot mentioned this pull request May 31, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

mkhamoyan added 2 commits June 1, 2023 17:36

Implemented IsPrefix, IsSuffix functions

9d23f7b

Remove logs

1f8b5d5

mkhamoyan changed the title ~~Implemented IndexOf and LastIndexOf functions~~ Implemented IndexOf, LastIndexOf, StartsWith, EndsWith functions Jun 1, 2023

mkhamoyan changed the title ~~Implemented IndexOf, LastIndexOf, StartsWith, EndsWith functions~~ Implement IndexOf, LastIndexOf, StartsWith, EndsWith functions Jun 1, 2023

mkhamoyan changed the title ~~Implement IndexOf, LastIndexOf, StartsWith, EndsWith functions~~ Implement collation native functions functions Jun 1, 2023

Fix CI build failures

7b91b6c

mkhamoyan removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jun 2, 2023

Refactored

c4a2d3c

mkhamoyan requested review from steveisok and mdh1418 June 2, 2023 09:42

mkhamoyan added 4 commits June 6, 2023 11:05

Fixed some test cases for OSX

001c366

Minor changes in test cases

0ec8d79

test case minor refactoring

fa7322c

Merge branch 'main' into hybrid_collation_functions

fc2837b

mkhamoyan requested review from akoeplinger and ilonatommy June 7, 2023 14:10

Merge branch 'main' into hybrid_collation_functions

a7ab572

mdh1418 reviewed Jun 19, 2023

View reviewed changes

mkhamoyan added 2 commits June 19, 2023 10:45

Refactored some files

caebcbe

Make the doc more readable

71b026e

mkhamoyan mentioned this pull request Jun 19, 2023

[iOS][globalization][icu] Refactor GlobalizationNative_IndexOfNative function #87767

Closed

Refactored IndexOf function

9dda83a

mkhamoyan requested review from mdh1418 and ilonatommy June 19, 2023 16:27

Add more comments in IndexOF function

88c6861

mdh1418 reviewed Jun 19, 2023

View reviewed changes

src/native/libs/System.Globalization.Native/pal_collation.m Outdated Show resolved Hide resolved

mdh1418 reviewed Jun 19, 2023

View reviewed changes

src/native/libs/System.Globalization.Native/pal_collation.m Outdated Show resolved Hide resolved

mdh1418 reviewed Jun 19, 2023

View reviewed changes

src/native/libs/System.Globalization.Native/pal_collation.m Outdated Show resolved Hide resolved

remove localizedStandardRangeOfString

460aba0

mkhamoyan requested a review from mdh1418 June 20, 2023 10:21

runfoapp bot mentioned this pull request Jun 20, 2023

Infra improvements for Helix #68176

Closed

mdh1418 approved these changes Jun 20, 2023

View reviewed changes

jkotas reviewed Jun 20, 2023

View reviewed changes

Added exception in case mixed compositions

3d5a195

mkhamoyan merged commit eccc410 into main Jun 21, 2023

mkhamoyan mentioned this pull request Jun 22, 2023

Fix runtime-extra-platforms failures related to HybridGlobalization #87913

Merged

MichalStrehovsky deleted the hybrid_collation_functions branch June 22, 2023 09:21

ghost locked as resolved and limited conversation to collaborators Jul 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement collation native functions functions #86895

Implement collation native functions functions #86895

mkhamoyan commented May 30, 2023 •

edited

Loading

ghost commented May 30, 2023

ghost commented May 30, 2023

mdh1418 Jun 16, 2023

mkhamoyan Jun 19, 2023

jkotas Jun 19, 2023

mkhamoyan commented Jun 19, 2023

azure-pipelines bot commented Jun 19, 2023

mdh1418 Jun 19, 2023

mdh1418 Jun 19, 2023

mkhamoyan Jun 20, 2023

mdh1418 Jun 19, 2023

mdh1418 Jun 19, 2023

mdh1418 Jun 19, 2023

mkhamoyan Jun 20, 2023

mdh1418 Jun 20, 2023

mkhamoyan Jun 20, 2023

mkhamoyan commented Jun 20, 2023

azure-pipelines bot commented Jun 20, 2023

mdh1418 left a comment

mdh1418 Jun 20, 2023

mdh1418 Jun 20, 2023

mdh1418 Jun 20, 2023

mdh1418 Jun 20, 2023

mkhamoyan Jun 21, 2023

jkotas Jun 20, 2023

jkotas Jun 20, 2023

jkotas Jun 20, 2023

mkhamoyan Jun 20, 2023

jkotas Jun 20, 2023

mkhamoyan commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

mkhamoyan commented Jun 21, 2023

		1. Search string contains diaeresis and has same normalization form as in source string.
		2. Search string contains diaeresis but with source string they have same letters with different char lengths but substring is normalized in source.

		Search string contains diacritics and with source string they have same letters with different char lengths but substring is not
		normalized in source. example: search string: `U\u0308 and \u00FC` (Ü and ü) source string: `Source is \u00DC and \u0075\u0308` (Source is Ü and ü)

	namespace System.Globalization
	internal static partial class Interop

Implement collation native functions functions #86895

Implement collation native functions functions #86895

Conversation

mkhamoyan commented May 30, 2023 • edited Loading

ghost commented May 30, 2023

ghost commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Jun 19, 2023

azure-pipelines bot commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Jun 20, 2023

azure-pipelines bot commented Jun 20, 2023

mdh1418 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

mkhamoyan commented Jun 21, 2023

mkhamoyan commented May 30, 2023 •

edited

Loading