When should standard library functions crash?

Is there a standard lib guideline on when a function can/should crash?

For example: Dictionary.init(uniqueKeysAndValues:) crashes on duplicate keys. While I can understand the reasoning to throw somehow on a collision, nothing about the name makes it feel "unsafe." I'd hope the type system would have my back and use throws for these kinds of things. Or to use a safe, opinionated default, like:

Dictionary.init(keysAndValues, uniquingKeysWith: { $1 })

The possibility of crashing is currently undocumented, but even if it were documented I'm not sure crashing is a good idea here. It's very easy for a developer to use autocomplete to discover this method, use it, and introduce a potential bug that could crash an app or take down a Swift web server.

4 Likes

In general, I don't think there's been a clear guideline, though the standard library isn't too shy around putting preconditions on things. Any collection method that takes an index, for example, will crash if you pass it the wrong inputs.

For your specific example, I can see the argument for throwing instead of crashing. I don't totally remember how much that was discussed during the drafting and review process. I do remember discussing the option of including a "safe, opinionated default"—the problem with that was that there wasn't a consensus about what the default should be (keep the latest vs. keep the original data), so we decided to force the user to supply it in the case where there could be duplicate keys.

Here's my paraphrasing of the argument that I remember against making this throwing... If init(uniqueKeysAndValues:) is a throwing initializer, your recovery options are basically to crash or to try to create the dictionary again using init(_:uniquingKeysWith:). In that case, we should just push people toward using init(_:uniquingKeysWith:) in any case where they might have duplicate keys.

In some ways, that's a strong argument for not having init(uniqueKeysAndValues:) at all, so init(_:uniquingKeysWith:) is the only option. However, for the cases where you know you're not going to have any duplicates, then you've got a weird closure that never gets called.

5 Likes

Right, in the Swift standard library, failing to meet a documented precondition results in a runtime error. It’s pretty clear here, even in the name itself, what the precondition for using this initializer is. If it’s not called out as a precondition in the written documentation, then it should be.

Throwing is an error recovery mechanism used when the thrown error adds actionable information. That is not the case here.

There is a persistent misconception that Swift functions should not “crash” because it’s a “safe” language. In fact, what’s referred to is memory safety. When a precondition is violated, there’s a logic error somewhere such that not stopping execution is unsafe.

8 Likes

I don't know if I'd call it "pretty clear". If you stop and read and think about the label, it definitely makes sense, but on a casual glance nothing really screams "crash". Meanwhile, the pop-up documentation makes no mention of the precondition. A note about "runtime error" is buried in the discussion and it takes a careful reading to see.

That's fine. It was a quick mention and I don't think we need to belabor that point anymore.

I don't have this "misconception". My complaint is that I'd like potential runtime errors to be much louder. It's too easy to let this particular function sneak into a code base without thought. The fact that Collection subscripting crashes when out-of-bounds makes sense from a performance and correctness perspective, but it's also a common enough case that folks new to the language and the behavior will encounter it quickly. Encountering preconditions on lesser-used APIs that don't particularly scream "unsafe" is gonna lead to bugs.

1 Like

Yeah, I was going to mention this. I think in this case I'd prefer:

  • A slightly louder name that conveys "unsafe"
  • This function to just not exist (let users define their common case)
  • At the very least for Xcode's documentation viewer to surface preconditions loudly in the pop-up docs.

It'd also be nice to codify how preconditions are communicated in the names of functions themselves (Collection subscripting being a common exception).

3 Likes

Agree with @xwu here. Since assuming unique keys is a programmer error when it's not guaranteed by the code, it's an inappropriate use of throw/nil.

You should also avoid making something failable/throwing when a common use case is statically known to never fail. Otherwise this leads to a common need for force-unwrap/try.

In this case there are lots of times when you can be guaranteed that no checks are needed e.g. Dictionary(uniqueKeysWithValues: zip(1..., something).

Since a throwing variant is trivially composable by throwing from the uniquing closure of the other version, the only question would be about making the argument label more emphatic.

3 Likes

This is the misconception: the method is safe. “Unsafe” in Swift would be not stopping when the precondition is violated.

Xcode supports showing preconditions under a separate heading, and you’re right that this should be called out. This can be addressed immediately.

Yep, again, no need to belabor this point. I agree that throws isn't appropriate here.

I think you're niggling on semantics. I mean "unsafe" here as in "this can crash". I would like preconditions to be communicated more effectively, ideally in both the name and the documentation. The precondition is arguably communicated in the label here, but it's quiet, and it's bit me and others before, and the fact that it could lead to security issues on Swift web servers and cause annoying app crashes is something I'd hope could be addressed.

We should use terminology carefully, as otherwise confusion results. Crashing is safe. Nothing is better for preventing memory safety issues than not executing code. This is a tentpole feature of Swift’s design and this needs to be communicated clearly. Crashing is safe.

We need to have clear documentation: I agree.

Disagree with changes to the name: the requirements are already unambiguous. Many, if not most, methods in the standard library have preconditions; those that don’t are likely to call others that do. It wouldn’t help to prefix everything.

3 Likes

I've opened a quick PR to at least add documentation here:

https://github.com/apple/swift/pull/15049

I think as Swift makes a bigger and bigger push on the server, these kinds of crashes are going to become more and more of an issue. We're dealing with a security vector that people will have to be aware of when dealing with user input.

This is a short initializer that takes a sequence and it's easy to discover and reach for without thinking (or discovering) the longer, uniquingKeysWith alternative.

Better failure recovery is a known deficiency of ours, but defining away runtime errors is never going to be the solution. If a user manages to feed you input that violates preconditions in your code, maliciously or not, crashing is the least bad thing the program can do. As time goes on we should be able to provide mechanisms that contain the damage and make recovery less expensive than it is now.

7 Likes

Yep, I don't disagree, but it'd be nice to call out these preconditions more loudly in the interim.

Was there discussion around these failure boundaries in the async actor model?

It is. But at the same time Optionals are (rightfully!) sold as a safety device that prevents the app from crashing with a null pointer exception. So this is another meaning of safety, as viewed by Elm or Haskell, for example – not crashing without explicit warnings. I think the question here is if the name of an argument is explicit warning enough.

2 Likes

Yes, optionals are a way to prevent dereferencing null pointers (which is undefined behavior in C). But there are other kinds of undefined behavior such as out-of-bounds memory access and integer overflow. The way in which Swift provides safety is, as it is when you force-unwrap a nil optional value, deterministically crashing.

The logic of Swift standard library design is that, any time you feed input that doesn't fulfill X to a method that says it takes an argument fulfilling X, the result you should expect is a crash unless some form of error recovery is noted. It's not the occasional method that has this behavior, but almost all (if not all methods) are written in this way. The warning you're asking about would apply to the entire library.

I think this initializer is probably a bit more severe than other comparable ones in the standard library. There's no memory safety issue here, like there is with trying to access an element outside of an array's bounds, so it's just a matter of what to do when the input isn't valid. It's probably fair to compare this to an initializer like Int("one"), which fails as nil, not as a runtime error.

My hope is that with a better abstract the attractiveness of using it in the wrong context will be lessened. If this truly is a significant problem that better docs can't solve, I'd be in the camp of removing it entirely rather than changing the signature.

3 Likes

I have no problem with the current Dictionary(uniqueKeysAndValues:) behavior. When you fail complying with preconditions, you get what you deserve. It's correct.

But correct is not always enough. I like a lot what Nate says. It would have been sensible if Dictionary(uniqueKeysAndValues:) had returned an optional instead of crashing.

The current behavior would just be a bang away. One could get a "free" check for duplicate keys by testing for nil. And nobody could miss the fact that this initializer can fail (which is what Stephen talks about).

1 Like

Yes, briefly. Memory ownership model will enable the foundation of improved isolation of actors so that we could design specific actors in a way that it would be safe for them to crash and restart without taking the whole app down, somewhat similar to Erlang.

4 Likes