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

[SR-10858] Bridging NSString-keyed Dictionaries should only set allowsDuplicates if there are non-ASCII (non-NFC?) Strings #53248

Open
Catfish-Man opened this issue Jun 7, 2019 · 9 comments
Labels
Dictionary Area → standard library: the `Dictionary` type good first issue Good for newcomers improvement standard library Area: Standard library umbrella

Comments

@Catfish-Man
Copy link
Member

Catfish-Man commented Jun 7, 2019

Previous ID SR-10858
Radar None
Original Reporter @Catfish-Man
Type Improvement
Status In Progress
Resolution
Additional Detail from JIRA
Votes 1
Component/s Standard Library
Labels Improvement, StarterBug
Assignee valeriyvan (JIRA)
Priority Medium

md5: d18df56f9579713f39cb8ff948cf30ca

Issue Description:

Currently we have code like this:

// String and NSString have different concepts of equality, so
// string-keyed NSDictionaries may generate key collisions when bridged
// over to Swift. See rdar://problem/35995647
let handleDuplicates = (Key.self == String.self)
      
result = Dictionary(_unsafeUninitializedCapacity: numElems,
   allowingDuplicates: handleDuplicates) { keys, vals in
        

This is overly conservative. We only actually need to set allowingDuplicates if there are non-NFC NSStrings!

@swift-ci
Copy link
Collaborator

Comment by Tim Searle (JIRA)

Hey, I'm interested in taking a look at this as a potentially first contribution to the standard library. What would be the correct (and efficient way) to check if the `String` contained non-NFC characters? Would the check itself outweigh any benefits to not setting `allowingDuplicates` for all `Dictionary`'s with `String` keys?

Also, where would I find the test cases for this part of the library? Would be good to confirm whether or not the existing tests already cover this case to detect any regressions a change might make.

@Catfish-Man
Copy link
Member Author

Awesome, thanks for taking a look 🙂

I’m on my phone at the moment, so don’t have the source handy to reference, but a simple subset of this that’s easy to check is all ASCII Strings are automatically NFC and there’s a fast isASCII flag on _StringGuts.

The simplest way to find the relevant tests is probably just to always set allowingDuplicates to false and then see which tests break.

@swift-ci
Copy link
Collaborator

Comment by Tim Searle (JIRA)

Apologies for the delay! Only just got around to getting my Swift development environment setup. Quick question, I built the Swift source for Xcode, with the debug flag for the stdlib. Would that make the correct command to run the tests afterwards `swift/utils/build-script --release-debuginfo --debug-swift-stdlib -x -t` ? Also interested to know what sort of workflow/environments others are using? As building and running tests will have a fair bit more overhead than general application development.

Pretty new to this! So I am still trying to figure out what my workflow will look like. 🙂 Pretty sure once I have this sorted, getting an initial PR won't be too much effort.

@Catfish-Man
Copy link
Member Author

test comment

@Catfish-Man
Copy link
Member Author

No worries, and no hurry. That command line looks about right except I'd leave the -x off once you have the project generated. Workflow for the stdlib is… well, I suggest patience… you won't have fully working tooling support (so only partial autocomplete or jump to definition at best, for example), and builds take a while. I use 'git grep' a lot to find things. Sometimes it can be worth yanking some code out into a standalone project to play with it in a less heavyweight environment.

@belkadan
Copy link
Contributor

Still working on this, tim_ (JIRA User)?

(I'm checking in on all the StarterBugs that haven't been touched in over a month; it's totally fine if you just haven't had time but still want to keep it.)

@swift-ci
Copy link
Collaborator

Comment by Tim Searle (JIRA)

Hey @belkadan, disappointingly I haven't had the opportunity to make as much progress as I'd like (beyond getting my environment up and running) and the next few weeks are quite busy for me - so for now I'll unassign the ticket in case anyone else wants to pick it up, thanks for the reminder!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@k-thorat
Copy link

Hey @Catfish-Man Is this still valid? I don't see any references to _unsafeUninitializedCapacity

Thank you!

@Catfish-Man
Copy link
Member Author

The situation got complicated for a bit. I think it may become valid again in the future, but for the moment it's not, sorry about the confusion.

@AnthonyLatsis AnthonyLatsis added the Dictionary Area → standard library: the `Dictionary` type label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dictionary Area → standard library: the `Dictionary` type good first issue Good for newcomers improvement standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

5 participants