Wow, this is huge progress. All of the changes are great!
I have some further suggestions on how to improve the type system modeling of actors, which I posted in this thread. Here are some comments from a read through of draft #3, which does not take into consideration that direction:
Minor typos:
- "Implementation note: At an implementation level, the messages are partial tasks (described by the [Structured Concurrency] proposal)". -> it seems like Structured Concurrency should be a link out.
- Broken punctuation: "However, the [Alternatives Considered](#alternatives-considered] section".
I don't understand this description in the closures section of the proposal:
The first bullet is always true, and I think you describe this earlier in the closure section. I think it is enough to say that @concurrent
functions are never actor isolated.
I don't really understand the second bullet though. It would help my understanding to be more specific about what is tied to the callee declaration, the function type the closure is inferred to, and the closure itself. How much of this is clang importer rules (e.g. implicitly importing completion handlers as @concurrent
) vs rules for working with other random Swift 5 code?
I think this is just a writing thing, but in:
It would be good to add some rationale here. There is nothing memory unsafe (AFAIK) about this, it is just likely to trip up memory exclusivity checks and is an unnecessary booby trap to allow this.
If this is required for correctness, then the restriction would have to be consistently applied to local variables as well as stored properties, since they are actor isolated state as well. I think that this is actually ok though because you get diference instances for these things for each stack frame, which makes them ok in practice. It would be good to capture some of this in the writing.
You explained the interaction between actors and ConcurrentValue really nicely in the Cross-actor references and ConcurrentValue
types section!
In detailed design, "An actor may only inherit from another actor. " --> I thought inheritance from NSObject was allowed?
It's not clear to me how nonisolated(unsafe)
works: it looks like it can be applied to stored properties, which means that the property can be accessed from nonisolated methods. However, can it also be applied to methods as well? Are nonisolated(unsafe)
properties only accessible from nonisolated(unsafe)
members? Does unsafety propagate out or no?
Reading later it looks like methods can be declared as nonisolated(unsafe)
which allows them to do things to arbitrary isolated state. This seems overly dangerous to me, I think it would be better to require all the data members to be "opted into" unsafety, rather than having extensions be able to throw arbitrary unsafe logic onto actors.
I'm not super excited about the whole nonisolated
and nonisolated(unsafe)
thing in general, if it remains, then I think it is important to get the safety story right on the (unsafe)
version otherwise it can rapidly undermine safety goals of the entire model.
In the isolation checking section, the term "executable code" is a bit confusing to me (this isn't a .exe file). Is there a better term that can be used here?
The Protocol Conformance section has an offhand mention that protocol requirements can be marked nonisolated. If this is intended, then it would be good to explain the full model here. What do they mean with structs and classes that conform to the protocol, what are the subtyping rules, what does "nonisolated(unsafe)" mean on a requirement, etc.
In Partial Applications it would be good to spell out the type of self.synchronous
explicitly, e.g. something like this:
let fp1 : () -> () = self.synchronous // seems ok
let fp2 : @concurrent () -> () = self.synchronous // invalid conversion
It would also be useful to expand out those two sentences at the end of this subsection into examples, because I don't understand exactly what you mean.
I'm very happy see the discussion of non-reentrancy included in the proposal, and I'm also very very happy to see it is in the future work section.
Overall, really great job on this!
-Chris