[Concurrency] Suggest adding a method, when mutating actor property cross isolation #75922
Conversation
ktoso
force-pushed
the
wip-note-for-mutating-actor-properties
branch
from
9e30c7b to
e0673c9
Compare
| return actorInstance.get<NominalTypeDecl *>(); | ||
| } | ||
|
|
||
| NominalTypeDecl *ActorIsolation::getActorOrNullPtr() const { |
There was a problem hiding this comment.
You added this one recently but it seems to be a copy paste of the other and it is not used so I removed it @gottesmm if we should keep it let me know.
| return actorType | ||
| ->getReferenceStorageReferent()->getAnyActor(); | ||
| if (getKind() == GlobalActor) { | ||
| return getGlobalActor()->getAnyNominal(); |
There was a problem hiding this comment.
ActorIsolation::getActor() used to return garbage if it contained a GlobalActor before this fix.
We initialize the globalActor property but fall through to returning the actorInstance.get<NominalTypeDecl *>(); which is just some uninitialized memory for the global actor case.
This is fixed now, we properly handle both kinds of isolations the method claimed to support.
Just FYI @hborla
lib/Sema/TypeCheckConcurrency.cpp
Outdated
|
|
||
| if (auto actor = isolation.getActor()) { | ||
| // TODO: isolation is printing without the type name for instance actors, | ||
| // it'd be nicer to print with type name and avoid this switch |
There was a problem hiding this comment.
The ways the isolation is printed ended up not work with the sentence too well; we always want to offer the actor name, while actorIsolation printing did not print that for instance isolation, so for now this workaround (rather than fixing tons of tests)
| @@ -2,7 +2,7 @@ public struct Globals { | |||
| public static let integerConstant = 0 | |||
| public static var integerMutable = 0 | |||
|
|
|||
| public static nonisolated(unsafe) let nonisolatedUnsafeIntegerConstant = 0 | |||
There was a problem hiding this comment.
this caused an un-necessary warning we're not testing for; removed the warning
…ross isolation This comes as an idea from the [Why do not actor-isolated properties support 'await' setter](https://forums.swift.org/t/why-do-not-actor-isolated-properties-support-await-setter/73867/26) thread. The note that > Actor-isolated property 'colorFill' can not be mutated from the main actor Leads people to assume that this is somehow "broken" while they could add a metho to perform the mutation and move on. A developer comments that: > So I just assumed that this meant that it was fundamentally wrong to > mutate this actor's properties from another actor, while all I actually > needed was... a 3 line setter.
ktoso
force-pushed
the
wip-note-for-mutating-actor-properties
branch
from
e0673c9 to
50d221a
Compare
|
@swift-ci please smoke test |
ktoso
added
the
concurrency
|
@swift-ci please smoke test |
| memberLoc, | ||
| diag::note_consider_method_for_isolated_property_mutation, | ||
| actor); | ||
| // } else if (isolation.getKind() == swift::ActorIsolation::GlobalActor) { |
There was a problem hiding this comment.
Was the commented out code here and above meant to be left in? NominalTypeDecl *actorTy is also unused
This comes as an idea from the Why do not actor-isolated properties support 'await' setter thread.
The note that
Leads people to assume that this is somehow "broken" while they could add a metho to perform the mutation and move on.
A developer comments that:
resolves rdar://134041422