Closed shadyelmaadawy closed 1 year ago
As documented, OrderedSet.update(_:at:)
requires that the supplied item compare equal to the original one that's already in the set.
/// Replace the member at the given index with a new value that compares equal
/// to it.
///
/// This is useful when equal elements can be distinguished by identity
/// comparison or some other means.
///
/// - Parameter item: The new value that should replace the original element.
/// `item` must compare equal to the original value.
///
/// - Parameter index: The index of the element to be replaced.
///
/// - Returns: The original element that was replaced.
///
/// - Complexity: Amortized O(1).
As noted above, this is useful when the Element
type has components that it doesn't consider significant for its definition of equality -- it allows the replacement of such items without going through a full hash lookup, as long as the position is already known.
What problem are you trying to solve?
This is a very unusual requirement for a method named update
; I think it's reasonable for someone to be confused why doing something that is on its face what the name of the method suggests will be done leads to a runtime trap.
Xiaodi, what would you expect this method to do?
This is a very unusual requirement for a method named
update
; I think it's reasonable for someone to be confused why doing something that is on its face what the name of the method suggests will be done leads to a runtime trap.As documented,
OrderedSet.update(_:at:)
requires that the supplied item compare equal to the original one that's already in the set./// Replace the member at the given index with a new value that compares equal /// to it. /// /// This is useful when equal elements can be distinguished by identity /// comparison or some other means. /// /// - Parameter item: The new value that should replace the original element. /// `item` must compare equal to the original value. /// /// - Parameter index: The index of the element to be replaced. /// /// - Returns: The original element that was replaced. /// /// - Complexity: Amortized O(1).
As noted above, this is useful when the
Element
type has components that it doesn't consider significant for its definition of equality -- it allows the replacement of such items without going through a full hash lookup, as long as the position is already known.What problem are you trying to solve?
it's doesn't about function name or usage, the problem is when trying to update an element that is not the same as the old one, that leads to application crashes, you could simply test the code I wrote above.
The function is trapping because you’re violating its documented precondition.
Again, what problem are you trying to solve?
@lorentey I would expect a method named update
to update the element at the specified index to any value of my choosing, provided that the invariants of the collection are maintained (in this case, that elements are unique).
I would say that might be a useful operation to add, but replace(at:with:)
would be a far more sensible name for it than update(_:at:)
. The latter doesn't read like a general replacement operation at all.
update(_:at:)
is a core low-level operation for set-like collection types, sitting at the same level of importance as insert(_:)
. It is the primitive operation underlying all mutations that update existing members of the set, including the standard update(with:)
:
extension SomeSet {
mutating func update(with item: Element) -> Element? {
let (inserted, index) = self.insert(item)
if inserted { return nil }
return self.update(item, at: index)
}
}
(Note: this is using the true primitive insert
operation that returns an index rather than a member value. Unfortunately, SetAlgebra
got this badly wrong -- a very unfortunate bug. (Understandable, given that SetAlgebra
doesn't even define an Index type. But nonetheless, it's a crucial design issue.))
The update(_:at:)
primitive is not often useful, but when it is, it can be pretty handy. For example, this operation can be used to normalize the members of a set to some canonical form without incurring any hashing at all:
var items: SomeSet<String> = ...
for i in items.indices {
items.update(items[i].normalizationFormC, at: i)
}
It isn't possible to do this with a regular Set
, because it doesn't provide the equivalent operation. (It should!)
I chose the particular name update(_:at:)
because it reflects that the operation has a clear subject -- the item we're updating must already exist in the given set.
var items: OrderedSet = ["café", "reëlect", "façade", "jalapeño"]
// Update the set by replacing the existing item "café" with a refined variant.
let old = items.update(with: "caf\u{301}e")
// Update the existing item "façade" at index 2 with a refined variant.
let old = items.update("fa\u{0327}cade", at: 2)
"Update the item "façade" at index 2" is a precise description of what this operation does, matching exactly what happens.
Passing this method a value that isn't member of the set reads like a nonsensical command:
let old = items.update("peach", at: 1)
This isn't cromulent -- there is no peach at index 1 to update. If this call successfully mutated the set somehow, I would have no idea what its mutated value would be. (If there was already a peach in some other location, would it somehow move the existing item? Leave it in place? Why doesn't it return something indicating this? If peach is a new item, why does this operation have such an awkward name? It isn't sensible to say "update [the value] peach at [index] 1" to mean "replace the member at [index] 1 with [the value] peach" or "insert [the value] "peach" at [index] 1".)
Contrast with:
let (index, old) = items.replace(at: 1, with: "peach") // or `replaceMember(at:with:)`
I can tell that this operation is supposed to replace whatever was at index 1 with the word "peach". The call site still doesn't explicitly tell me what happens if "peach" was already in the set at a different position, however the fact that the operation returns an index is a strong indication that it leaves it there. (If the method only returned a value, then my expectation would be that it would trap if the new item already existed in the set.)
I'm begging @shadudiix to explain what they're trying to do in order to understand whether to consider this issue a feature request for a new operation, and if so, what precise semantics are they looking for.
(For what it's worth, removing an existing item and replacing it with a brand new one is not a primitive operation -- it is already possible to implement this in O(1) hash+copy operations using append
, swapAt
, update(_:at:)
and removeLast()
. However, it may be useful to provide this for convenience.)
For reference, the primitive operations for a collection type that implements a set construct include:
// Member lookup
func find(_ value: Element) -> Index? // a.k.a. firstIndex(of:)
// Member insertion
mutating func insert(_ value: Element) -> (inserted: Bool, index: Index)
// Member update
mutating func update(_ member: Element, at index: Index) -> Element
// Member removal
mutating func remove(at index: Index) -> Element
Behind the scenes, I usually find more useful to split the insertion primitive into two distinct phases:
mutating func mutatingFind(_ value: Element) -> (found: Bool, index: Index)
mutating func insertNew(_ value: Element, at index: Index)
If the item isn't found, mutatingFind
returns an index addressing the position where a newly inserted item belongs. (Depending on the data structure, this may or may not happen to fall onto a valid index -- Set
's find operation always returns an invalid index, but in the upcoming SortedSet
, it'll be always valid.) This allows higher-level operations to optimize the number of full lookup operations. (For maximum versatility, mutatingFind
and insertNew
ought to be public API, but the impracticality of validating the index passed to indexNew
makes this unlikely to happen.)
I'm not arguing that it doesn't serve a very useful purpose to have this operation, but I think this bug is demonstrating that the update
is a poor choice of name.
The very meaning of "update" is that you make some salient change, and the meaning of two things comparing equal is that there is no salient difference (for some blessed definition of salience). The name is setting up users for failure because it is by the dictionary definition of the word telling users to use it for the very thing that trips a precondition.
There is no need for a niche operation to use a tempting common name, even if it is a primitive; it can have a longer name, or it can use different terminology, or both. For instance, one might borrow the term "touch" to indicate a minor and limited modification without changing the salient aspects of the value.
The name is setting up users for failure because it is by the dictionary definition of the word telling users to use it for the very thing that trips a precondition.
The dictionary definition of the verb "update" is "to bring up to date", not "to replace with something unrelated".
Reasonable people will disagree on naming choices; the world will go on. As much fun it would be endlessly argue on past API naming decisions, the public API of OrderedSet
has been stable for a year or so, and I'm not particularly keen to re-litigate minor aspects of its design at this point -- I don't see how renaming this particular entry point could possibly justify the migration costs. As I have explained above, (1) the name we chose perfectly fits the function's purpose, and (2) we would not use this name for a function that implements an unconstrained replacement (if it was possible to implement that).
For what it's worth, the upcoming BitSet
will not provide an update(_:at:)
method (there is no point in updating an integer value), but I expect PersistentSet
and SortedSet
will do so. This package does not fall under the scope of the Swift Evolution process, but I expect I will run some sort of a loose API review on the associated forum topic -- that thread might be a good opportunity to revisit this naming decision. Of course, any concrete suggestion for a replacement must be an obviously better choice to overcome the hurdle of breaking precedent.
I'm still interested in learning what exactly @shadudiix is trying to do -- we can certainly add new functionality to cover common use cases!
The dictionary definition of the verb "update" is "to bring up to date", not "to replace with something unrelated".
Yes—to bring up-to-date by changing it. Indeed, to replace with something new (I'm not arguing that the new value must be necessarily unrelated):
verb to make something more modern or suitable for use now by adding new information or changing its design: • an updated version of the software to give someone the most recent information: • We'll update you on this news story throughout the day.
(Source: Cambridge English Dictionary)
This is, in fact, the meaning given to the APIs named update
in SE-0370, deprecating (updating!) the original name of assign
—there must have been some value initialized originally in order to correctly use the update
API, but the meaning of the API is to replace the previous value with a new value.
It's also how the term is used colloquially. If my boss asks me if there's any update about some ongoing work that I haven't made any progress on, I'd tell him that there's "no update." I wouldn't say, "Yes, here's the update!", then rephrase my last report using different words, providing no new information.
Anyway, I too am curious what @shadudiix is trying specifically to do. That will clarify this bug report.
Ah, good point about SE-0370.
when calling the update function in an ordered set, the application get crashes.
Click to expand crash report
Full Report ------------------------------------- Translated Report (Full Report Below) ------------------------------------- Incident Identifier: 64E09DB1-54A0-49C2-8CB3-7763DE7603D3 CrashReporter Key: 57D363D4-8EA7-3301-7C86-FBDCC8956CD6 Hardware Model: MacBookPro16,1 Process: asdasdasdasd [3446] Path: /Users/USER/Library/Developer/CoreSimulator/Devices/FECA0831-B583-4273-B0FE-573FB0B2E82C/data/Containers/Bundle/Application/2D524AB7-E62B-4BB9-AD9E-077A2C127C14/asdasdasdasd.app/asdasdasdasd Identifier: test.asdasdasdasd Version: 1.0 (1) Code Type: X86-64 (Native) Role: Foreground Parent Process: launchd_sim [1864] Coalition: com.apple.CoreSimulator.SimDevice.FECA0831-B583-4273-B0FE-573FB0B2E82C [1227] Responsible Process: SimulatorTrampoline [646] Date/Time: 2022-09-02 14:09:39.6781 +0200 Launch Time: 2022-09-02 14:09:36.6389 +0200 OS Version: macOS 12.4 (21F79) Release Type: User Report Version: 104 Exception Type: EXC_BAD_INSTRUCTION (SIGILL) Exception Codes: 0x0000000000000001, 0x0000000000000000 Exception Note: EXC_CORPSE_NOTIFY Termination Reason: SIGNAL 4 Illegal instruction: 4 Terminating Process: exc handler [3446] Triggered by Thread: 0 Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 asdasdasdasd 0x10d0415b4 Swift runtime failure: precondition failure + 0 [inlined] 1 asdasdasdasd 0x10d0415b4 $ss14_stringCompare__9expectingSbs11_StringGutsV_ADs01_D16ComparisonResultOtF + 0 [inlined] 2 asdasdasdasd 0x10d0415b4 $sSS2eeoiySbSS_SStFZ + 0 [inlined] 3 asdasdasdasd 0x10d0415b4 $sSSSQsSQ2eeoiySbx_xtFZTW + 0 [inlined] 4 asdasdasdasd 0x10d0415b4 specialized OrderedSet.update(_:at:) + 228 5 asdasdasdasd 0x10d04142a ViewController.removeRandomClicked(_:) + 74 (ViewController.swift:56) 6 asdasdasdasd 0x10d0415fa @objc ViewController.removeRandomClicked(_:) + 58 7 UIKitCore 0x7fff250a49d3 -[UIApplication sendAction:to:from:forEvent:] + 83 8 UIKitCore 0x7fff24933354 -[UIControl sendAction:to:forEvent:] + 110 9 UIKitCore 0x7fff24933758 -[UIControl _sendActionsForEvents:withEvent:] + 345 10 UIKitCore 0x7fff2492fc3b -[UIButton _sendActionsForEvents:withEvent:] + 148 11 UIKitCore 0x7fff24931faf -[UIControl touchesEnded:withEvent:] + 485 12 UIKitCore 0x7fff250e5dd6 -[UIWindow _sendTouchesForEvent:] + 1292 13 UIKitCore 0x7fff250e7e3a -[UIWindow sendEvent:] + 5312 14 UIKitCore 0x7fff250bdeac -[UIApplication sendEvent:] + 820 15 UIKitCore 0x7fff25155f0a __dispatchPreprocessedEventFromEventQueue + 5614 16 UIKitCore 0x7fff2515935d __processEventQueue + 8635 17 UIKitCore 0x7fff2514faf5 __eventFetcherSourceCallback + 232 18 CoreFoundation 0x7fff203724a7 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 19 CoreFoundation 0x7fff2037239f __CFRunLoopDoSource0 + 180 20 CoreFoundation 0x7fff2037186c __CFRunLoopDoSources0 + 242 21 CoreFoundation 0x7fff2036bf68 __CFRunLoopRun + 871 22 CoreFoundation 0x7fff2036b704 CFRunLoopRunSpecific + 562 23 GraphicsServices 0x7fff2cba9c8e GSEventRunModal + 139 24 UIKitCore 0x7fff2509e65a -[UIApplication _run] + 928 25 UIKitCore 0x7fff250a32b5 UIApplicationMain + 101 26 libswiftUIKit.dylib 0x7fff59d55cc2 UIApplicationMain(_:_:_:_:) + 98 27 asdasdasdasd 0x10d041db5 $sSo21UIApplicationDelegateP5UIKitE4mainyyFZ12asdasdasdasd03AppB0C_Tg5 + 80 [inlined] 28 asdasdasdasd 0x10d041db5 $s12asdasdasdasd11AppDelegateC5$mainyyFZ + 97 (