Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Swift 5 #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update to Swift 5 #22

wants to merge 5 commits into from

Conversation

lewisbar
Copy link
Collaborator

I've made a few updates to switch to Swift 5, the biggest being replacing "Hashable.hashValue" with "hash(into:)". I hope I did everything correctly there.
I have not yet seen into updating Completer, I'd have to do some reading about Strings first.

@lewisbar
Copy link
Collaborator Author

Oh, the tests cause a crash because Completer is not fixed yet.

@lewisbar
Copy link
Collaborator Author

That was just a fix for the warnings in Completer. The tests still crash.

@lewisbar
Copy link
Collaborator Author

lewisbar commented Nov 18, 2021

Only these two tests crash:

  • test_ColonAfterCase_WithEmojis_AdoptsSwitchIndentation()
  • test_ColonAfterDefault_WithEmojis_AdoptsSwitchIndentation()

It's an index-out-of-bounds error, by the way.
Feels like I'm getting closer to to root of the problem.

@lewisbar
Copy link
Collaborator Author

Another thought, for anyone reading this, maybe someone knows: The cause must be some change between Swift 3 and Swift 4.2, I think, because the tests used to pass back then. I hope I find the time to look further into it.

@lewisbar
Copy link
Collaborator Author

Oh, of course the problem could have been introduced by dff687b ... That's actually quite likely, I almost forgot about that.

@lewisbar
Copy link
Collaborator Author

dff687b seems correct, though. Next step: I must look at the two crashing tests and inspect their results. Maybe at some point in the next days.

@lewisbar
Copy link
Collaborator Author

lewisbar commented Nov 21, 2021

I wonder if this is a Swift bug.
When I do

var newText = text.replacingCharacters(in: selection, with: insertion)

the test crashes with "String index is out of bounds" when trying to use the selection, for example with newText[selection]. (selection is in this case a Range<String.Index> with a length of 0, so you could think of it as the cursor or insertion point. The insertion is a colon ":".)

But when I do

var newText = text
newText.insert(Character(insertion), at: selection.lowerBound)

the test passes, and newText[selection] works as expected.

But the utf16.count is the same for both strings, as well as the count, and they are even ==.

Still when I do

let newRange = newText.index(stringRange.lowerBound, offsetBy: 1)..<newText.index(stringRange.upperBound, offsetBy: 1)
newText[..<newRange.lowerBound]

it works for the string that has been changed by insert(at:), but crashes for the string that has been created by replacingCharacters(in:with:). How is that possible when both are ==?

The problem is, I need to use replacingCharacters(in:with:), because the user could have selected multiple characters when entering a new one. Also, insert(at:) can only insert single characters, so it wouldn't work if the user pastes several characters. Maybe I could build a workaround, but the question is: Is this a bug in Swift and should be reported?

EDIT:
In the playground, the error is not "String index is out of bounds" but "error: Execution was interrupted, reason: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0).
The process has been left at the point where it was interrupted, use "thread return -x" to return to the state before expression evaluation." (But still only for the string that's created by replacingCharacters(in:with:).)

@lewisbar
Copy link
Collaborator Author

Here is a playground in which I tried to show the bug as clearly as possible. https://github.com/lewisbar/ReplacingCharactersBug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant