NSRange: fix init from region RangeExpression in target String #1894

ApolloZhu · 2019-02-11T14:25:48Z

Prior to Swift 5, String uses UTF-16 for its Index and encodedOffset. However, this assumption is no longer valid and encodedOffset might be in UTF-8 for native contents in Swift 5.

This change should make the initializer work as expected under either circumstances.

Prior to Swift 5, String uses UTF-16 for its Index and encodedOffset. However, this assumption is no longer valid and encodedOffset might be in UTF-8 for native contents in Swift 5. This change should make the initializer work as expected under either circumstances.

spevans · 2019-02-11T14:48:57Z

@swift-ci test

ApolloZhu · 2019-02-11T17:04:47Z

I don't think there's any existing test for this. I'll write some now.

ianpartridge · 2019-02-11T17:37:14Z

Can we port the code from https://github.com/apple/swift/blob/master/stdlib/public/Darwin/Foundation/NSRange.swift instead?

ApolloZhu · 2019-02-11T17:57:32Z

@ianpartridge sure. Thanks for pointing it out!

spevans · 2019-02-11T22:52:22Z

@swift-ci test

ApolloZhu · 2019-02-12T20:30:44Z

Looks like tests passed but no status reported?

https://ci.swift.org/job/swift-corelibs-foundation-PR-Linux/1903/

milseman · 2019-02-12T20:31:15Z

You're really going to want to stay up to date with the overlays in swift.

milseman · 2019-02-12T20:31:34Z

Can you add a LIT test that makes sure they don't diff at the very least?

ApolloZhu · 2019-02-12T20:54:45Z

Sorry for my inexperience, but could you please explain where I could write the lit test? @milseman

Just my assumption, but I should add a file here?: https://github.com/apple/swift/blob/master/test/stdlib/NSRange.swift

milseman · 2019-02-12T23:27:45Z

I don't know the testing or build set up for corelibs-foundation, but there could be many approaches.

Is a checkout of the swift source code required to build/test corelibs-foundation? If so, then as a build step corelibs-foundation could copy these overlay files over, that way they are always in sync.

If the build system is not configurable, but corelibs-foundation supports LIT-stye unit tests, then we can at least diff the files and error if there is any difference, similar to how the ABI checker works.

If corelibs-foundation testing is constrained to XCTest only, we should still be able to diff the files, but I don't know XCTest very well.

@ianpartridge or @parkera might know more.

ApolloZhu · 2019-02-13T04:36:15Z

Swift source code had been checked out during CI testing, but didn't seem to be necessary for building/testing in Xcode for me.
I don't see a file named lit.cfg in the swift-corelibs-foundation repository.
https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/NSRange.swift and https://github.com/apple/swift/blob/master/stdlib/public/Darwin/Foundation/NSRange.swift are not identical. It's right that some parts of them are, but this would also be the case for many other files in this repository. Therefore, (I personally believe) the lit test isn't an essential part of this pull request, but should rather be added through another, solemnly dedicated one to make sure all places where identical implementations are expected.

parkera · 2019-02-13T18:15:05Z

We don't use lit for Foundation tests, just XCTest.

parkera · 2019-02-13T18:16:14Z

TestFoundation/TestNSRange.swift

@@ -126,4 +128,52 @@ class TestNSRange : XCTestCase {
            XCTAssertEqual(NSStringFromRange(range), string)
        }
    }
+
+    private func _assert<S: StringProtocol, R: RangeExpression>(


Can we give this a more specific name to indicate that it is specialized for the below test?

And use more robust implementation

ApolloZhu · 2019-02-13T19:07:05Z

@parkera updated

spevans · 2019-02-13T19:08:44Z

@swift-ci test

millenomi · 2019-02-14T06:23:55Z

@swift-ci please test

millenomi · 2019-02-14T06:24:14Z

you have to be courteous to robots. :)

millenomi · 2019-02-14T06:25:03Z

To add to the discussion here: I absolutely would love to unify the overlay and s-c-f code where it’s redundant, but it is really not in the cards for this 5.x cycle.

ApolloZhu · 2019-02-14T17:27:34Z

Looks like the test has failed due to s-c-xctest:

02:14:10 Actual: "/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-corelibs-xctest/Tests/Functional/Asynchronous/Expectations/main.swift:356: error: ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted : Asynchronous waiter <XCTWaiter expectations: 'inner-1'> failed - Interrupted by timeout of containing waiter <XCTWaiter expectations: 'outer'>\n"
02:14:10 Expected: '.*/Tests/Functional/Asynchronous/Expectations/main.swift:366: error: ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted : Asynchronous wait failed - Exceeded timeout of 0.1 seconds, with unfulfilled expectations: outer'

from this code:

// CHECK: Test Case 'ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted' started at \d+-\d+-\d+ \d+:\d+:\d+\.\d+
// CHECK: .*/Tests/Functional/Asynchronous/Expectations/main.swift:[[@LINE+22]]: error: ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted : Asynchronous wait failed - Exceeded timeout of 0.1 seconds, with unfulfilled expectations: outer
// CHECK: .*/Tests/Functional/Asynchronous/Expectations/main.swift:[[@LINE+11]]: error: ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted : Asynchronous waiter <XCTWaiter expectations: 'inner-1'> failed - Interrupted by timeout of containing waiter <XCTWaiter expectations: 'outer'>
// CHECK: .*/Tests/Functional/Asynchronous/Expectations/main.swift:[[@LINE+15]]: error: ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted : Asynchronous waiter <XCTWaiter expectations: 'inner-2'> failed - Interrupted by timeout of containing waiter <XCTWaiter expectations: 'outer'>
// CHECK: Test Case 'ExpectationsTestCase.test_outerWaiterTimesOut_InnerWaitersAreInterrupted' failed \(\d+\.\d+ seconds\)
    func test_outerWaiterTimesOut_InnerWaitersAreInterrupted() {

I'll look into what's causing this, but I'm confident that it has nothing to do with this pull request, since Build #1922 failed for the same reason

spevans · 2019-02-14T17:47:55Z

@swift-ci test

jckarter · 2019-02-14T22:54:19Z

Is there a chance this is related to this bot failure on Ubuntu 18.04? It looks possibly range-in-string-related:

https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-18_04-long-test/1231/console

11:42:46 Test Case 'TestProcess.test_passthrough_environment' started at 2019-02-14 19:44:05.374
11:42:46 
/home/buildnode/jenkins/workspace/oss-swift-package-linux-ubuntu-18_04/swift-corelibs-foundation/TestFoundation/TestProcess.swift:257: error: TestProcess.test_passthrough_environment : failed - Test failed: InvalidEnvironmentVariable("swi")

ApolloZhu · 2019-02-14T23:19:27Z

@jckarter your link and output segment seems to be unrelated, so I’ll address both:

The linked build failed because

<unknown>:0: warning: missing submodule 'CDispatch'
/home/buildnode/jenkins/workspace/oss-swift-incremental-RA-linux-ubuntu-18_04-long-test/swift-corelibs-libdispatch/src/swift/Block.swift:13:8: error: no such module 'CDispatch'
import CDispatch

test_passthrough_environment should be addressed by Workaround SR-9930 to prevent CI failures in test_passthrough_environment #1915

jckarter · 2019-02-14T23:26:49Z

Oops, sorry for pasting the wrong link. #1915 should address this, thanks!

ApolloZhu · 2019-02-17T15:33:39Z

Sorry for brining this up again, but should this fix actually go into the swift-5.0-branch instead/also?

ianpartridge · 2019-02-17T17:13:58Z

Yes, please open a PR against that branch.

ApolloZhu changed the title ~~NSRange: fix init from region RangeExpression in target String~~ [WIP] NSRange: fix init from region RangeExpression in target String Feb 11, 2019

NSRange: add test for init(_:in:)

7e605c3

NSRange: replace init(_:in:) with Darwin implementation

243a89e

ApolloZhu changed the title ~~[WIP] NSRange: fix init from region RangeExpression in target String~~ NSRange: fix init from region RangeExpression in target String Feb 11, 2019

parkera reviewed Feb 13, 2019

View reviewed changes

NSRange: test use more specific name

cf12a0d

And use more robust implementation

millenomi merged commit 5894e8a into apple:master Feb 14, 2019

ApolloZhu mentioned this pull request Feb 17, 2019

[5.0] NSRange: fix init from region RangeExpression in target String #1924

Merged

norio-nomura mentioned this pull request Feb 12, 2019

[SR-9749] The encodedOffset of String.Index has been changed to either UTF-8 or UTF-16. apple/swift#52180

Closed

jckarter mentioned this pull request Feb 14, 2019

[SR-9931] TestProcess.test_passthrough_environment test failing on Ubuntu 18.04 bot #3534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NSRange: fix init from region RangeExpression in target String #1894

NSRange: fix init from region RangeExpression in target String #1894

ApolloZhu commented Feb 11, 2019

spevans commented Feb 11, 2019

ApolloZhu commented Feb 11, 2019

ianpartridge commented Feb 11, 2019

ApolloZhu commented Feb 11, 2019

spevans commented Feb 11, 2019

ApolloZhu commented Feb 12, 2019

milseman commented Feb 12, 2019

milseman commented Feb 12, 2019

ApolloZhu commented Feb 12, 2019 •

edited

milseman commented Feb 12, 2019

ApolloZhu commented Feb 13, 2019

parkera commented Feb 13, 2019

parkera Feb 13, 2019

ApolloZhu commented Feb 13, 2019

spevans commented Feb 13, 2019

millenomi commented Feb 14, 2019

millenomi commented Feb 14, 2019

millenomi commented Feb 14, 2019

ApolloZhu commented Feb 14, 2019 •

edited

spevans commented Feb 14, 2019

jckarter commented Feb 14, 2019

ApolloZhu commented Feb 14, 2019

jckarter commented Feb 14, 2019

ApolloZhu commented Feb 17, 2019

ianpartridge commented Feb 17, 2019

NSRange: fix init from region RangeExpression in target String #1894

NSRange: fix init from region RangeExpression in target String #1894

Conversation

ApolloZhu commented Feb 11, 2019

spevans commented Feb 11, 2019

ApolloZhu commented Feb 11, 2019

ianpartridge commented Feb 11, 2019

ApolloZhu commented Feb 11, 2019

spevans commented Feb 11, 2019

ApolloZhu commented Feb 12, 2019

milseman commented Feb 12, 2019

milseman commented Feb 12, 2019

ApolloZhu commented Feb 12, 2019 • edited

milseman commented Feb 12, 2019

ApolloZhu commented Feb 13, 2019

parkera commented Feb 13, 2019

parkera Feb 13, 2019

Choose a reason for hiding this comment

ApolloZhu commented Feb 13, 2019

spevans commented Feb 13, 2019

millenomi commented Feb 14, 2019

millenomi commented Feb 14, 2019

millenomi commented Feb 14, 2019

ApolloZhu commented Feb 14, 2019 • edited

spevans commented Feb 14, 2019

jckarter commented Feb 14, 2019

ApolloZhu commented Feb 14, 2019

jckarter commented Feb 14, 2019

ApolloZhu commented Feb 17, 2019

ianpartridge commented Feb 17, 2019

ApolloZhu commented Feb 12, 2019 •

edited

ApolloZhu commented Feb 14, 2019 •

edited