Skip to content

Commit 016bc24

Browse files
gribozavrgribozavr
authored
Merge pull request #4658 from apple/stdlib-fix-string-comparison-with-icu
stdlib: disable an incorrect String comparison optimization
2 parents 7cdab0b + ef974af commit 016bc24

File tree

7 files changed

+557
-35
lines changed

7 files changed

+557
-35
lines changed

‎stdlib/private/StdlibUnicodeUnittest/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ add_swift_library(swiftStdlibUnicodeUnittest ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES}
99
# This file should be listed the first. Module name is inferred from the
1010
# filename.
1111
StdlibUnicodeUnittest.swift
12+
Collation.swift
1213

1314
SWIFT_MODULE_DEPENDS StdlibUnittest
1415
SWIFT_COMPILE_FLAGS ${swift_stdlib_unittest_compile_flags}

‎stdlib/private/StdlibUnicodeUnittest/Collation.swift

Lines changed: 375 additions & 0 deletions
Large diffs are not rendered by default.

‎stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1960,7 +1960,8 @@ public func checkEquatable<Instances : Collection>(
19601960
_checkEquatableImpl(
19611961
Array(instances),
19621962
oracle: { oracle(indices[$0], indices[$1]) },
1963-
allowBrokenTransitivity: allowBrokenTransitivity)
1963+
allowBrokenTransitivity: allowBrokenTransitivity,
1964+
${trace})
19641965
}
19651966

19661967
internal func _checkEquatableImpl<Instance : Equatable>(
@@ -1985,18 +1986,23 @@ internal func _checkEquatableImpl<Instance : Equatable>(
19851986
let predictedXY = oracle(i, j)
19861987
expectEqual(
19871988
predictedXY, oracle(j, i),
1988-
"bad oracle: broken symmetry between indices \(i), \(j)")
1989+
"bad oracle: broken symmetry between indices \(i), \(j)",
1990+
stackTrace: ${stackTrace})
19891991

19901992
let isEqualXY = x == y
19911993
expectEqual(
19921994
predictedXY, isEqualXY,
1993-
"lhs (at index \(i)): \(x)\nrhs (at index \(j)): \(y)",
1995+
(predictedXY
1996+
? "expected equal, found not equal\n"
1997+
: "expected not equal, found equal\n") +
1998+
"lhs (at index \(i)): \(String(reflecting: x))\n" +
1999+
"rhs (at index \(j)): \(String(reflecting: y))",
19942000
stackTrace: ${stackTrace})
19952001

19962002
// Not-equal is an inverse of equal.
19972003
expectNotEqual(
19982004
isEqualXY, x != y,
1999-
"lhs (at index \(i)): \(x)\nrhs (at index \(j)): \(y)",
2005+
"lhs (at index \(i)): \(String(reflecting: x))\nrhs (at index \(j)): \(String(reflecting: y))",
20002006
stackTrace: ${stackTrace})
20012007

20022008
if !allowBrokenTransitivity {
@@ -2010,7 +2016,8 @@ internal func _checkEquatableImpl<Instance : Equatable>(
20102016
for k in transitivityScoreboard[i].value {
20112017
expectTrue(
20122018
oracle(j, k),
2013-
"bad oracle: broken transitivity at indices \(i), \(j), \(k)")
2019+
"bad oracle: broken transitivity at indices \(i), \(j), \(k)",
2020+
stackTrace: ${stackTrace})
20142021
// No need to check equality between actual values, we will check
20152022
// them with the checks above.
20162023
}
@@ -2147,11 +2154,28 @@ public func checkComparable<Instances : Collection>(
21472154
for i in instances.indices {
21482155
let x = instances[i]
21492156

2150-
expectFalse(x < x, ${trace})
2151-
expectFalse(x > x, ${trace})
2152-
expectTrue(x <= x, ${trace})
2153-
expectTrue(x >= x, ${trace})
2154-
2157+
expectFalse(
2158+
x < x,
2159+
"found 'x < x'\n" +
2160+
"at index \(i): \(String(reflecting: x))",
2161+
stackTrace: ${stackTrace})
2162+
2163+
expectFalse(
2164+
x > x,
2165+
"found 'x > x'\n" +
2166+
"at index \(i): \(String(reflecting: x))",
2167+
stackTrace: ${stackTrace})
2168+
2169+
expectTrue(x <= x,
2170+
"found 'x <= x' to be false\n" +
2171+
"at index \(i): \(String(reflecting: x))",
2172+
stackTrace: ${stackTrace})
2173+
2174+
expectTrue(x >= x,
2175+
"found 'x >= x' to be false\n" +
2176+
"at index \(i): \(String(reflecting: x))",
2177+
stackTrace: ${stackTrace})
2178+
21552179
for j in instances.indices where i != j {
21562180
let y = instances[j]
21572181

@@ -2163,11 +2187,30 @@ public func checkComparable<Instances : Collection>(
21632187
+ "(\(String(reflecting: i)), \(String(reflecting: j)))",
21642188
stackTrace: ${stackTrace})
21652189

2166-
expectEqual(expected.isLT(), x < y, ${trace})
2167-
expectEqual(expected.isLE(), x <= y, ${trace})
2168-
expectEqual(expected.isGE(), x >= y, ${trace})
2169-
expectEqual(expected.isGT(), x > y, ${trace})
2170-
2190+
expectEqual(expected.isLT(), x < y,
2191+
"x < y\n" +
2192+
"lhs (at index \(i)): \(String(reflecting: x))\n" +
2193+
"rhs (at index \(j)): \(String(reflecting: y))",
2194+
stackTrace: ${stackTrace})
2195+
2196+
expectEqual(expected.isLE(), x <= y,
2197+
"x <= y\n" +
2198+
"lhs (at index \(i)): \(String(reflecting: x))\n" +
2199+
"rhs (at index \(j)): \(String(reflecting: y))",
2200+
stackTrace: ${stackTrace})
2201+
2202+
expectEqual(expected.isGE(), x >= y,
2203+
"x >= y\n" +
2204+
"lhs (at index \(i)): \(String(reflecting: x))\n" +
2205+
"rhs (at index \(j)): \(String(reflecting: y))",
2206+
stackTrace: ${stackTrace})
2207+
2208+
expectEqual(expected.isGT(), x > y,
2209+
"x > y\n" +
2210+
"lhs (at index \(i)): \(String(reflecting: x))\n" +
2211+
"rhs (at index \(j)): \(String(reflecting: y))",
2212+
stackTrace: ${stackTrace})
2213+
21712214
for k in instances.indices {
21722215
let expected2 = oracle(j, k)
21732216
if expected == expected2 {

‎stdlib/public/core/StringComparable.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ extension String {
110110
#if _runtime(_ObjC)
111111
// We only want to perform this optimization on objc runtimes. Elsewhere,
112112
// we will make it follow the unicode collation algorithm even for ASCII.
113+
// This is consistent with Foundation, but incorrect as defined by Unicode.
113114
if _core.isASCII && rhs._core.isASCII {
114115
return _compareASCII(rhs)
115116
}
@@ -120,6 +121,10 @@ extension String {
120121

121122
extension String : Equatable {
122123
public static func == (lhs: String, rhs: String) -> Bool {
124+
#if _runtime(_ObjC)
125+
// We only want to perform this optimization on objc runtimes. Elsewhere,
126+
// we will make it follow the unicode collation algorithm even for ASCII.
127+
// This is consistent with Foundation, but incorrect as defined by Unicode.
123128
if lhs._core.isASCII && rhs._core.isASCII {
124129
if lhs._core.count != rhs._core.count {
125130
return false
@@ -128,6 +133,7 @@ extension String : Equatable {
128133
lhs._core.startASCII, rhs._core.startASCII,
129134
rhs._core.count) == 0
130135
}
136+
#endif
131137
return lhs._compareString(rhs) == 0
132138
}
133139
}

‎validation-test/stdlib/HashingICU.swift

Lines changed: 0 additions & 20 deletions
This file was deleted.

‎validation-test/stdlib/StringHashableComparable.swift.gyb

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// RUN: %target-run-simple-swiftgyb
2+
// REQUIRES: executable_test
3+
4+
// This test requires that the standard library calls ICU
5+
// directly. It is not specific to Linux, it is just that on
6+
// Apple platforms we are using the NSString bridge right now.
7+
8+
// REQUIRES: OS=linux-gnu
9+
10+
import StdlibUnittest
11+
import StdlibUnicodeUnittest
12+
13+
func assertASCIIRepresentationIfPossible(_ s: String) {
14+
for us in s.unicodeScalars {
15+
if !us.isASCII {
16+
return
17+
}
18+
}
19+
precondition(s._core.isASCII)
20+
}
21+
22+
func forceUTF16Representation(_ s: String) -> String {
23+
var s = s
24+
s += "\u{fffd}"
25+
s.removeSubrange(s.index(before: s.endIndex)..<s.endIndex)
26+
precondition(!s._core.isASCII)
27+
return s
28+
}
29+
30+
func calculateSortOrder(_ tests: inout [StringComparisonTest]) {
31+
tests.sort {
32+
collationElements(
33+
$0.collationElements,
34+
areLessThan: $1.collationElements
35+
)
36+
}
37+
38+
tests[0].order = 0
39+
for i in tests.indices.dropFirst() {
40+
if collationElements(
41+
tests[i - 1].collationElements,
42+
areLessThan: tests[i].collationElements
43+
) {
44+
tests[i].order = tests[i - 1].order! + 1
45+
} else {
46+
tests[i].order = tests[i - 1].order!
47+
}
48+
}
49+
}
50+
51+
func checkStringHashableComparable(
52+
_ tests: [StringComparisonTest],
53+
stackTrace: SourceLocStack = SourceLocStack(),
54+
file: String = #file, line: UInt = #line
55+
) {
56+
var tests = tests
57+
calculateSortOrder(&tests)
58+
59+
func comparisonOracle(_ i: Int, _ j: Int) -> ExpectedComparisonResult {
60+
return tests[i].order! <=> tests[j].order!
61+
}
62+
63+
checkHashable(
64+
tests.map { $0.string },
65+
equalityOracle: { comparisonOracle($0, $1).isEQ() },
66+
stackTrace: stackTrace.pushIf(true, file: file, line: line))
67+
68+
checkComparable(
69+
tests.map { $0.string },
70+
oracle: comparisonOracle,
71+
stackTrace: stackTrace.pushIf(true, file: file, line: line))
72+
}
73+
74+
var StringTests = TestSuite("StringTests")
75+
76+
StringTests.test("StringComparisonTest.allTests: tests are in ASCII representation")
77+
.forEach(in: StringComparisonTest.allTests) {
78+
test in
79+
assertASCIIRepresentationIfPossible(test.string)
80+
}
81+
82+
StringTests.test("Comparable") {
83+
let allTestsInUTF16Representation = StringComparisonTest.allTests.map {
84+
test -> StringComparisonTest in
85+
return StringComparisonTest(
86+
forceUTF16Representation(test.string),
87+
test.collationElements,
88+
sourceLocation: SourceLoc(
89+
test.loc.file,
90+
test.loc.line,
91+
comment: (test.loc.comment ?? "") + "\nin Unicode representation"))
92+
}
93+
checkStringHashableComparable(StringComparisonTest.allTests + allTestsInUTF16Representation)
94+
}
95+
96+
runAllTests()
97+

‎validation-test/stdlib/StringLowercasedUppercased.swift

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
// RUN: %target-run-simple-swiftgyb
2+
// REQUIRES: executable_test
3+
4+
// This test requires that the standard library calls ICU
5+
// directly. It is not specific to Linux, it is just that on
6+
// Apple platforms we are using the NSString bridge right now.
7+
8+
// REQUIRES: OS=linux-gnu
9+
10+
import StdlibUnittest
11+
12+
var StringTests = TestSuite("StringTests")
13+
14+
StringTests.test("smoketest") {
15+
expectEqual("i\u{0307}", "\u{0130}".lowercased())
16+
expectEqual("SS", "\u{00df}".uppercased())
17+
}
18+
19+
runAllTests()
20+

0 commit comments

Comments
 (0)