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 the review manager via the forum messaging feature. When contacting the review manager directly, please keep the proposal link at the top 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?
Super important improvement for API authors that want to eventually align their runtime data-safety standards with Swift, but can't afford directly introducing crashes for existing end users.
if isIsolatingCurrentContext is not available on the expected executor, but checkIsolatedis available
What does it means by saying "be available", does it mean "the customized implementation of the method is used, not the default implementation", or, does it use some magical runtime introspection?
If I understand it correctly, it can be achieved by just using default implementations, for example:
It means "a non-default implementation" / "not the default implementation that exists in the stdlib" was provided by the developer. Yes, this is the compiler detecting this. Since we want to (soft-)deprecate the previous APIs and this one supersedes it we're taking these special affordances to be able to do this without breaking adopters.
This implementation approach sadly is not workable for the requirement we're facing: being able to issue warnings when able to.
If we'd just naively start calling isIsolatingCurrentContext with such default implementation when we'd want to issue a warning... We'd potentially end up calling checkIsolated, we'd cause a crash where we really wanted to only issue a warning if able to detect and issue a warning for such executor. This isn't acceptable for building tools or libraries which want to issue warnings and specifically not crash when unable to determine if a warning should be issued.
A specific use case here is frameworks which may have illegal usage patterns today and as they adopt Swift Concurrency want to start warning about those patterns which actually always were somewhat unsafe but "happened to work most of the time". They'd like to start issuing warnings for now, and maybe in future versions start crashing as developers had time to fix their code.
I want to dig into this more, because it came up in the pitch thread as well. The proposal makes use of the ability to detect whether isIsolatingCurrentContext was given the default implementation vs. the user providing that information, to decide whether it's safe to call checkIsolated(). However, it reserves that ability for itself: a user cannot tell whether isIsolatingCurrentContext provides a meaningful result, or whether it's getting the default false result.
I have three concerns about this aspect of the proposal:
It seems likely that users will want to build abstractions on top of isIsolatingCurrentContext that may need to be able to tell the difference. We shouldn't have to be tied in with the concurrency runtime to be able to implement something akin to "if isolated, do this; otherwise, do that" and be able to account for the "we can't know" case.
The proposed design requires specific, additional complexity in the compiler and runtime to handle this detection, when alternative solutions (such as the enum in the future directions discussion of this topic) model it with already-existing, well-understand language features.
While we were able to resolve the technical concerns that led to the checkIsolated() design previously with one library's executors, can we rule out that other libraries won't have this same issue and not find an appropriate solution? Is "I don't know" truly only a temporary thing that will go away, or might we need it in the future?
I know we want the Bool API, because it's simpler. But in doing so, we've created more complexity and shut out potential use cases in the process. This isn't a beginner API---indeed, most people shouldn't use it directly, but should use higher-level APIs like assumeIsolated instead---so I don't think we should shut out potential use cases to make it simpler. I'm much more concerned about being here again in a year or two generalizing the API a third time to accommodate those use cases than I am about each client of isIsolatingCurrentContext having to decide how to deal with the "unknown" case.
Primarily we ended up with the bool and dismiss the tri state because I didn't really see a meaningful way to handle "we don't know" as we'd generally have to assume it means "no" I think. Perhaps for some form of warning mode it could be useful to ONLY issue a warning if we concretely know that we're in the wrong isolation, but not if we don't know?
The only other implementation difference if we are to remove the special logic in the compiler and rely on the nil is that we will now always try to call the new method, and if "don't know" need to call checkIsolated next. Since this is the "failing path" it is likely okey to take the hit and make two (or three if there's complex executor equality) runtime calls. I think making a number of runtime calls is probably managable here but wanted to call this out that we'll be making more calls -- which can add up for things like as? with isolated conformances etc. But I think we're okey with those checks being more expensive.
In the future as most executors implement the new API we'd be back down to one check call; We only have to make two calls if we get the .unknown and then have to call checkIsolated as a fallback. That seems like an ok state to end up in the future -- most executors should be able to answer within a single runtime call.
--
Overall I think we can move to the tri-state, and the only thing to figure out then are the names of that. Currently the alternatives includes this type:
enum DetectedSerialExecutorIsolation {
case isolated // returned when isolated by this executor
case notIsolated // returned when definitely NOT isolated by this executor
case unknown // when the isIsolatingCurrentContext could not determine if the caller is isolated or not
}
I think we could change notIsolated to nonisolated as well, even though that's a runtime isolation and not the one typically associated with the nonisolated keyword, but it would make sense to rename that. isolated and unknown sound okey. And the type name is proably fine as well, it's not like this will be passed around much so a verbose name is okey here.
Good morning everyone,
it's been a while since this finished it's review period, however in the meantime we worked out some improvements I'd like to propose here.
The two changes I'm proposing here are:
changing the return type of isIsolatingCurrentContext to Bool?
this reduces special casing in the compiler as well as improves the usability of this API; nil signifies that an executor did not implement this check, and we cannot be sure if we are isolated to it or not.
adding a way to call this newly introduced API if all we have is an Actor
this comes in the form of Actor/withSerialExecutor which allows us to get to SerialExecutor/isIsolatingCurrentContext from e.g. an #isolation parameter or an @isolation(any) closure's .isolation.
These changes make this API more accessible from end-user Swift code, and not so tightly tied to the Swift runtime's internal uses.
Is there any reason not to make the generic return values of the two with closures T: ~Copyable? That would offer more flexibility to clients in case they want to return a non-copyable value.