I just did a full pass through. There were some cases around nonisolated
closures defined in methods and global actor isolated things where we are now
emitting the wrong message. I am going to fix that in subsequent commits.
A name diagnostic looks as follows:
Some notes:
1. VariableNameUtils still doesn't pattern match all cases. In the cases where
we fail to pattern match, I fall back to the old diagnostics. I am going to be
adding tests/etc specific to VariableNameUtils in a later patch.
2. I took this as an opportunity to begin preparing to change the diagnostics
into diags of the following form:
a. The main diagnostic is changed to something short "transferring non-Sendable
%0 could yield races with later accesses".
b. I added a longer note at the same location that explains that our caller is
in a specific isolation domain and our callee is isolated differently.
c. I added a note on the definition location of the identified value.
I tried to make them shorter and standardized on "; later accesses could race"
as the mark on a diagnostic that the problem was that a race was occuring
(before we had several small different variations).
I am not sure if this would ever actually ever cause a bug... but we should be
consistent. The issue here is that in the evaluator switch when processing
transfers in certain cases when we emitted an early error due to an actor
derived value we were performing a transfer and other times we weren't. With
this patch:
1. We now consistently do not actually transfer the value if it is actor
derived. It is always illegal to transfer an actor derived value... so we know
that if we always just error on transferring it, we have a safe model.
2. The switch we breaking so that we could do an assert that we canonicalized
the modified PartitionOp. Rather than do that and allow for people to
potentially return, I moved the assert into a SWIFT_DEFER and added an assert in
the continuation of the switch. This means that if an author ever breaks out of
the switch, they will hit the assert implying that the author in all cases must
explicitly return within the case statement guaranteeing this mistake does not
happen again.
This involved me removing the complex logic for emitting diagnostics we have
today in favor of a simple diagnostic that works by:
1. Instead of searching for transfer instructions, we use the transfer
instruction that we propagated by the dataflow... so there is no way for us to
possible not identify a transfer instruction... we always have it.
2. Instead of emitting diagnostics for all requires, just emit a warning for the
first require we see along a path. The reason that we need to do this is that in
certain cases we will have multiple "require" instructions from slightly
different source locations (e.x.: differing by column) but on the same line. I
saw this happen specifically with print where we treat stores into the array as
a require as well as the actual call to print itself where we pass the array.
An additional benefit of this is that this allowed me to get rid of the
cache of already seen require instructions. By doing this, we now emit errors
when the same apply needs to be required by different transfer instructions for
different arguments.
NOTE: I left in the original implementation code to make it easier to review
this new code. I deleted it in the next commit. Otherwise the git diff for this
patch is too difficult to read.
The reason for this issue is that we were originally checking in our NonSendable
type oracle just if a type conformed to Sendable. But function types do not
conform to protocols, so this would fail for protocols. To fix this, I added
some helper methods to call swift::isSendableType on SILType, called that in our
oracle, and then I added support in swift::isSendableType for both
SILFunctionType and AnyFunctionType so that we correctly handle them depending
on their sendability.
There was also a question if we also handled function conversions correctly. I
added a test case that shows that we do not error in either of the cases.
Another nice aspect of this change is that we no longer need to stash a pointer
to a looked up Sendable protocol to perform the check since that just happens
naturally inside SILType::isSendable() when it calls isSendableType. This is a
better separation of concerns.
rdar://116525224
Sendability.
Sendability of function references depends only on captures, and applies when the
function itself is passed across isolation boundaries. Parameter and result
values can only cross isolation boundaries when the function is called.
This means that:
1. In test cases where minimal is the default (swift 5 without
-warn-concurrency), I added RUN lines for targeted, complete, and complete +
sns.
2. In test cases where complete is the default (swift 6, -warn-concurrency,
specified complete with -strict-concurrency), I added a send non-sendable run
line.
In each of these cases, I added additional expected-* lines as appropriate so
the tests can compile in each mode successfully.
When checking for the sendability in a call, use the sendability of the
original argument expressions rather than the parameter type, looking
through any implicit conversions that might remove `Sendable`.
Fixes rdar://110763694 / FB12343467.
Isolation checking for calls had two separate implementation places:
one that looked at the declaration being called (for member
declarations) and one that worked on the actual call expression. Unify
on the latter implementation, which is more general and has access to
the specific call arguments. Improve diagnostics here somewher so we
don't regress in that area.
This refactoring shouldn't change the actual semantics, but it makes
upcoming semantic changes easier.
When a class has an unavailable conformance to a protocol, do not
inherit that unavailable conformance, because it can get in the way of
subclasses defining their own (properly-available) conformance.
Fixes rdar://89992569.
When determining whether a superclass conforms to a particular protocol,
skip unavailable conformances. This way, we don't minimize away a
constraint that might only apply to subclasses of the specified
superclass.
Fixes rdar://91853658.
The three options are now:
* `explicit`: Enforce Sendable constraints where it has been explicitly adopted and perform actor-isolation checking wherever code has adopted concurrency. (This is the default)
* `targeted`: Enforce Sendable constraints and perform actor-isolation checking wherever code has adopted concurrency, including code that has explicitly adopted Sendable.
* `complete`: Enforce Sendable constraints and actor-isolation checking throughout the entire module.
when a variable with type `<<error-type>>` was captured in a
sendable function's environment, we use to emit a diagnostic
about it not being sendable, but it's for a bogus type.
at some point this issue disappeared as the sendable implementation
evolved, but this commit adds regression coverage.
resolves rdar://82452688
Use the explicit check for a Sendable conformance (even an unavailable
one) as the mechanism for determining whether to diagnose a
missing/unavailable Sendable conformance in a particular context.
When in "existing" Swift code that is Swift 5.x and has not adopted
concurrency, allow mismatches in function types that only involve
ABI-neutral concurrency changes (e.g., adding `@Sendable` or removing
a global actor) by downgrading the diagnostic to a warning. This
improves the story for incremental adoption of concurrency in an
existing code base.
As part of this, generalize the facility for downgrading an error to a
warning when performing diagnostics in the constraint solver, using the
new diagnostic behavior limits rather than duplicating diagnostics.
