Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager, either via the forum messaging feature or by email. When contacting the review manager directly, please put "[SE-0458]" at the start of the subject line of the message.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I've been involved a lot in the discussion, and I'm of course a +1. The problem being addressed is important and this solution will ensure that developers who need to use Swift in environments that have critical memory safety requirements can maintain accountability on their use of unsafe code. I think that the design we have takes better into account the perspective of security auditors.
A late-breaking change to the proposal that I wasn't able to evaluate before now is the addition of the @safe attribute, which allows developers to specify that certain functions are safe despite @unsafe types appearing in their signature. I am not convinced that @safe pulls its own weight. As I understand it, the problem it solves is that it allows closure-taking functions like withUnsafeBufferPointer to not themselves be unsafe. I don't believe that there can be any other use to this attribute because any reference to an unsafe value needs to be marked with unsafe: for instance, if UnsafeBufferPointer had a @safe count, you would still need to write unsafe ubp.count because the expression references ubp, an unsafe value. Paradoxically, should we ever find another useful way to use @safe, adopting functions need to be considered more dangerous by auditors because they create an opportunity for unsafe code to sneak in unnoticed. Simultaneously, the future of underlying storage access is through Span, not UnsafeBufferPointer; we should encourage people to vend Spans.
I believe the proposal is fairly clear that marking a function that takes an UnsafeBufferPointer as @safedoes remove the need to mark the call with unsafe. It is something you would want to be fairly circumspect about with a generally-unsafe type, of course, but there are operations (like getting the count of an UBP) that it makes sense for.
I said it in the vision thread, but I'll say it again — I think this should be opt-out, not opt-in. I see the logic in the proposal, I just don't believe it! I think the vast majority of the swift open-source ecosystem will want to adopt the proposal and promptly; the places that don't want it will largely be the ones that interact directly with Apple's frameworks. Xcode could opt-out by default, but I feel that erring on the side of safety is the better default for the ecosystem.
Unsafe imports
I think there's a hole here around imports. For example, Apple's "Combine" is written in Swift, but not safe (by Swift 6's standard), and unlikely to become safe nor even acquire unsafe annotations. If I import Combine into my WarnUnsafe package, I need to see warnings for all Combine API usage. I suspect each built module needs to know whether it was built with strict memory safety, in order for consuming code to treat its APIs appropriately.
Open Classes
I think that this would be allowed, per the proposal:
// Module A, with WarnUnsafe
// @safe, or without it
open class A {
func doSomething() {}
}
public func doSomethingWithA(a: A) { // implicitly safe
a.doSomething() // implicitly safe
}
// Module B, without WarnUnsafe
class B: A {
func doSomething() {
withUnsafeEverything { ... }
}
}
doSomethingWithA(a: B())
Probably open class should be unsafe and unable to be marked @safe?
I think this would need to be clarified. Doug introduced the @safe attribute with count as the example, and I commented to say that based on discussion elsewhere, since even let x = y needs to be spelled let x = unsafe y if y’s type is unsafe, it would seem that using @safe would usually do nothing. Doug didn’t reply to that, but he changed the example to be withUnsafeBufferPointer, which does sidestep the issue.
The swap example has unsafe in all the places I would expect it to (assuming that unsafe has a lower precedence than &&):
extension UnsafeMutableBufferPointer {
@unsafe public func swapAt(_ i: Element, _ j: Element) {
guard i != j else { return }
precondition(i >= 0 && j >= 0)
precondition(unsafe i < endIndex && j < endIndex)
let pi = unsafe (baseAddress! + i)
let pj = unsafe (baseAddress! + j)
let tmp = unsafe pi.move()
unsafe pi.moveInitialize(from: pj, count: 1)
unsafe pj.initialize(to: tmp)
}
}
The two lines that don't have it don't reference self. (With that said, although it doesn't change the analysis, it does seem that i and j should be Index, not Element.) There's more copies of the example with unsafe in fewer places in "alternatives considered".
If we're assuming that x and y have unsafe types here, then the references to x and y (whether reading or assigning) will result in warnings about unsafe uses.
Context:
if var x = unsafe unsafeGetX() { // Semi-undocumented lifetime transfer, maybe OK since I still said 'unsafe' nearby
if let y = unsafe unsafeGetY() { // Semi-undocumented lifetime transfer, maybe OK since I still said 'unsafe' nearby
...
x = y // Undocumented lifetime transfer, very hard to audit!
Even if that is the case (which I, personally, do not believe), no-one is stopping anyone from enabling that setting for your package. But it should always be a decision to enable additional restrictions.
If it‘s enabled per default, beginners will start putting @safe everywhere to silence that warning as it’s the easiest way to get rid of the warnings. This is done without thinking much about possible consequences.
If you, as a library author, choose to explicitly enable that feature, it‘s much more likely that you understand what you are doing.
Anyways, I would say I‘m neutral on that proposal.
I see the need for warnings if code is potentially unsafe. But I‘m concerned about the implications of library authors trying to make the use of unsafe types safe by manually handling the safety so they can mark the method as safe which is objectively better to provide as a public API.
Memory safety issues are notoriously hard to diagnose before they actually happen in edge-cases.
There is no way for a library author to test against all possible uses of their method, so marking a method as safe will always be a best guess. No one it striving to write unsafe code but as it happens with C and C++ programs, it happens.
As I do not have a better idea on how to handle this, I’m neural and not against this proposal.
I'm in favour of the overall proposal. I think unsafe blocks should be added, but I understand the reasoning against them.
Nitpick: withUnsafeTemporaryAllocation() should be listed among the stdlib API that needs annotation.
More broadly: we should have some discussion about macros here. Because they operate solely on syntax, it is not always possible for them to know if a particular function is safe or unsafe, so something like:
@Test func foo(x: SomeUnsafeType) { ... }
May be impossible to expand correctly. As a result, we may have to have the compiler silently disable the feature when compiling expanded macro code (or otherwise find a solution to the problem so that Swift Testing and other macro adopters don't break.)
I have a few questions. Can string interpolation be unsafe? For instance:
let bad: UnsafePointer<String> = UnsafePointer(bitPattern: 0xBAD)!
let str = "\(bad)" // unsafe?
print(bad) // unsafe?
None of this crash, so in practice at least I guess it's fine to not require unsafe expressions here. But I suppose it's possible the compiler won't identify the last two statements as safe, a sort of false positive.
Then when it comes to generics, what happens with a function like this one:
func describe<T>(_ array: [T]) {
print("\(array.count) value(s) of type \(T.self)")
}
let a = ["a", "b", "c"]
describe(a) // safe
let b = [UnsafePointer(bitPattern: 0xBAD)]
describe(b) // safe or not?
Can you add @safe in front of func describe to make it safe for all generic arguments?
If yes, how can the implementer of the function know it really is safe? While this version is innocuous, a similar function could be storing the value in a global variable somewhere, outliving the pointers's lifetime.
If no, will all generic functions require the "double unsafe" marker @safe was supposed to avoid:
let b = [UnsafePointer(bitPattern: 0xBAD)]
let c = unsafe b.map { ptr in
unsafe "\(ptr)"
}
I like the direction this is going, but generics make me wonder if maybe we need a way to express some form of conditional unsafely.
Note: code under the section "Unsafe overrides" is lacking an override keyword.
extension UnsafeBufferPointer { @safe public var count: Int @safe public var startIndex: Int { 0 } @safe public var endIndex: Int { count }
}
Nit: count is actually let, not var, right? (That's important! If it were var it would be @unsafe, since assigning to it would change the buffer's bounds with no way to verify the new value.)
Very much +1 for me as I have been watching this happen over the years with great interest. I have not done an in-depth read of this however so nothing specific to mention in regards to keywords / specific usages, only that I'm favor of the concept.
Done a quick read.
+1 for me if treating as a nice-to-have perspective and appreciate the concept.
For applications/SDKs/etc., sometimes you need to tell the users whether the implementation is memory safe or not.
Being neutral/not against on the implementation.
This is a +1 from me as well. Especially I can commend the decision to make this opt-in. I think this is a right default for most swift users (and definitely me).
I have a note on the decision to not provide message of the @unsafe constructs in favor of regular comments.
I think it would be useful for library authors to tell the end user, how to ensure that the construct is safe to use in their context, raised as an editor/compiler diagnostic.
@unsafe(message: "Make sure not to pass -1")
func doSomethingSketchy(_ arg: Int) -> Int {
// Yeah, I know this is a stupid example
if arg == -1 {
UnsafeRawPointer(bitPattern: 0x1)
?.loadUnaligned(fromByteOffset: 0, as: Int.self)
}
return 42
}
in client code:
var result = doSomethingSketchy(userProvidedValue)
ERROR: calling `doSomethingSketchy(_:)` is unsafe and should be
used in an unsafe expression or in a @safe function
1 | var result = doSomethingSketchy(userProvidedValue)
^^^^^^^^^^^^^^^^^^
Note: possible solution
1 | var result = unsafe doSomethingSketchy(userProvidedValue)
Note: Make sure not to pass -1
I'm not aware of the tooling that can surface comments as a part of the diagnostic.
This does not paint my views negative towards this evolution proposal. Just something I wanted to mention that might make this more approachable.
The assumption this proposal makes here is that APIs from a module are unsafe iff the signature involves any unsafe types, unless annotated otherwise with @safe/@unsafe. Yes, it is a hole.
We have this information for each module. The compiler is currently writing out into a "trace" file it (optionally) builds for auditing purposes, but we could consider other (more visible) ways to surface this information.
I suspect this will be very noisy to use in practice. One answer here could be that an import of a module not built with strict memory safety could diagnose unless the import is marked with either @safe or @unsafe. If it's marked with @safe, it follows the rules I described above. If it's marked with @unsafe, everything from that module could be treated as unsafe. I don't know if I like this, but it would at least address this hole at the language level rather than relying on some separate auditing tool.
Here, B.doSomething is using unsafe constructs in its body. Under the strict safety mode, it'll get a warning and need an unsafe. Now, if the developer determines that this unsafe code usage in the body makes the whole function unsafe, they can mark it @unsafe. This will require the type B itself to be marked @unsafe (this is the Unsafe overrides section of the proposal). In your example, B isn't using strict memory safety, so we cannot account for the effects it will have on the rest of the program.
Right. The issue with endIndex is that self is of an unsafe type, so the (implicit) reference to self in self.endIndex is considered unsafe code even though endIndex itself is marked as @safe. There are a couple of ways in which we could consider this safe:
Don't diagnose the implicit object argument of a method/property access, and only diagnose the method or property being accessed
Don't diagnose references to local variables / parameters of unsafe type
In both cases, we would drop the unsafe from the endIndex references in the swapAt example above. The latter feels more in line with what the proposal does: right now, the proposal effectively implies that local variables involving unsafe types are implicitly @unsafe. This latter bullet says that they are implicitly @safe... but it's the operations on them that could be unsafe.
(I fixed the Index vs. Element issue in the proposal text, thank you)
I never actually had a model that would make those uses of endIndex safe. We should consider whether the option above meets our goals here.
Yes. By the rules in the proposal, this would be marked @safe, because the function itself is correctly managing the unsafe buffer allocation... but the closure passed in needs to be careful with the unsafe buffer it was given. Most of the operations on that buffer are @unsafe.
For the signature, if you're building in the strict safety mode, foo would either need to be marked @safe or @unsafe and the macro could do the same with any derived declarations we create. However, it's harder at the expression level, because the macro won't know what specific expressions will be unsafe to mark them. I... don't have an answer.
I don't think we should silently suppress diagnostics about unsafe code introduced through macros; they goes against the auditability principle of the proposal. But, we do need a way to suppress these diagnostics. One possibility is that we allow unsafe / @unsafe / @safe outside the macro expansion to very broadly suppress diagnostics within the macro expansion. Thinking of Swift Testing here, but for example:
unsafe #expect(unsafe myUnsafeThing() == 17)
could perhaps say that the unsafe outside the use of the macro covers all unsafe code within its expansion.
There may be a point in time where we want to push the strict safety mode more aggressively, and this is one way to do so. I do not think that time is now. It's better to have the features in place and adopt them in the most critical parts of the system, and then evaluate whether to nudge more toward their use.
The reference to bad is considered unsafe, so both the string interpolation and the use of bad in the print call are unsafe. Had you printed str, that would be safe, because it's a String and all of the unsafety has been dealt with.
Not safe, because when you provide unsafe types to a generic function, it's implementation can end up having unsafe behavior that wasn't accounted for.