Find all the usages of `--enable-experimental-feature` or
`--enable-upcoming-feature` in the tests and replace some of the
`REQUIRES: asserts` to use `REQUIRES: swift-feature-Foo` instead, which
should correctly apply to depending on the asserts/noasserts mode of the
toolchain for each feature.
Remove some comments that talked about enabling asserts since they don't
apply anymore (but I might had miss some).
All this was done with an automated script, so some formatting weirdness
might happen, but I hope I fixed most of those.
There might be some tests that were `REQUIRES: asserts` that might run
in `noasserts` toolchains now. This will normally be because their
feature went from experimental to upcoming/base and the tests were not
updated.
Under OSSA, the instruction may still be structurally responsible for consuming
its operand even if the result is dead, so we can't remove it without breaking
invariants.
More generally, this should probably apply to any instruction which consumes
one or more of its operands, has no side effects, and doesn't produce any
nontrivial results that require further consumption to keep the value alive.
I went with this targeted fix, since it addresses a problem that shows up
in practice (rdar://125381446) and the more general change appears to
disturb the optimizer pipeline while building the standard library.
Previously, the lexical attribute on begin_borrow instructions was used.
This doesn't work for values without lexical lifetimes which are
consumed, e.g. stdlib CoW types. Here, the new var_decl attribute on
begin_borrow is keyed off of instead. This flag encodes exactly that a
value corresponds to a source-level VarDecl, which is the condition
under which checking needs to run.
rdar://118059326
The reason why I am doing this is that this was not part of the original
evolution proposal (it was called an extension) and after some discussion it was
realized that partial consumption would benefit from discussion on the forums.
rdar://111353459
Pattern matching as currently implemented is consuming, but that's not
necessarily what we want to be the default behavior when borrowing pattern
matching is implemented. When a binding of noncopyable type is pattern-matched,
require it to be annotated with the `consume` operator explicitly. That way,
when we introduce borrowing pattern matching later, we have the option to make
`switch x` do the right thing without subtly changing the behavior of existing
code. rdar://110073984
this also fixes a bug where sometimes we simply emit
'consumed here' twice and other times we'd said 'other
consume here' for the same "consumed more than once"
message. so I went through and changed all of the 2nd
consumes into "consumed again".
rdar://109281444
- refer to a "consuming use" as simply a "consume", to reserve "use" for non-consuming uses.
- refer to "non-consuming uses" as just a "use".
- don't call it a "user defined deinit" and instead a "deinitializer" to match Sema
- be specific about what binding a closure is capturing that is preventing consumption.
rdar://109281444
- replaces "move-only" terminology with "noncopyable"
- replaces compiler jargon like "guaranteed parameters"
and "lvalue" with corresponding language-level notions
- simplifies diagnostics about closures.
and probably more.
rdar://109281444
Previously, we would emit a "compiler doesn't understand error" since we would
detect the escape and fail. That is the correct behavior here if the
partial_apply is not already identified as escaping by an earlier pass. But in
the case where we see that the partial_apply's callee was marked with
semantics::NO_MOVEONLY_DIAGNOSTICS, then we:
1. Suppress the "compiler doesn't understand error" for this specific
mark_must_check.
2. Suppress function wide the "copy of noncopyable type" error. Since we stopped
processing the mark_must_check that was passed to the partial_apply, we may
have left copies of noncopyable types on that mark_must_check value. This is
ok since the user will get the error, will recompile, and if any further show
up after they fix the inout escaping issue, they will be emitted
appropriately.
I also extended the tests to handle more interesting cases.
NOTE: There are a few cases where we introduced some new do not understand
errors. I am going to fix that in the next commit. I just wanted to completely
update the tests for the manner in which the allocbox to stack change affects
them.
With some of the changes that I have made, we began to emit a mark_must_check
[no_copy] on a copy_addr here. This change teaches the move address checker how
to recognize that in this case if we have a project_box it is actually b/c we
have something captured by an escaping closure.
Specifically, we already have the appropriate semantics for arguments captured
by escaping closures but in certain cases allocbox to stack is able to prove
that the closure doesn’t actually escape. This results in the capture being
converted into a non-escaping SIL form. This then causes the move checker to
emit the wrong kind of error.
The solution is to create an early allocbox to stack that doesn’t promote move
only types in boxes from heap -> stack if it is captured by an escaping closure
but does everything else normally. Then once the move checking is completed, we
run alloc box to stack an additional time to ensure that we keep the guarantee
that heap -> stack is performed in those cases.
rdar://108905586
These are the same semantically, just the mangling is slightly different. The
benefit of doing this is that we are actually testing what we expect our users
to do.
rdar://108511703
The reason why I am doing this is that currently SILGen knows when emitting said
closure that we are going to emit an error (that it does not have enough
information to emit itself since it doesn't know the caller), so doesn't emit
move checking markers. The result is that the move checker will not eliminate
any copies in the closure and thus will emit a "copy of noncopyable type found"
error and tell the user to file a bug. This just suppresses that.
rdar://108511866
The reason why I am doing this is that otherwise if one has a function that
takes both a guaranteed and an owned parameter, we will break OSSA invariants
since the load [take] will invalidate the load_borrow. So instead, we put in a
load_borrow knowing that the move checker will convert it to a load_borrow
assuming that the two pass exclusivity checking.
NOTE: Because of some missing functionality in subsequent tests, I had to
disable one test (moveonly_escaping_definite_initialization.swift) and also add
some checks for copy of noncopyable object errors. They will go away in the next
2 commits.
rdar://108510987
the main things still left behind the experimental flag(s) are
- move-only classes (guarded by MoveOnlyClasses feature)
- noimplicitcopy
- the _borrow operator
Some notes:
1. This ensures that if we capture them, we just capture the box by reference.
2. We are still using the old incorrect semantics for captures. I am doing this
so I can bring this up in separate easy to understand patches all of which
pass all of the moveonly tests.
3. Most of the test edits are due to small differences in error messages in
between the object and address checker.
4. I had to add a little support to the move only address checker for a small
pattern that doesn't occur with vars but do es occur for lets when we codegen
like this, specifically around enums. The pattern is we perform a load_borrow
and then copy_value and then use the result of the copy_value. Rather than fight
SILGen pattern I introduced a small canonicalization into the address checker which
transforms that pattern into a load [copy] + begin_borrow to restore the codegen
to a pattern the checker expects.
5. I left noimplicitcopy alone for now. But we should come back around and fix
it in a similar way. I just did not have time to do so.
Specifically, previously if we emitted an error we just dumped all of the
consuming uses. Now instead for each consuming use that needs a copy, we perform
a search for a specific boundary use (consuming or non-consuming) that is
reachable from the former and emit a specialized error for it. Thus we emit for
the two consuming case the normal consumed twice error, and now for
non-consuming errors we emit the "use after consume" error.
For those who are unaware, CanonicalizeOSSALifetime::canonicalizeValueLifetime()
is really a high level driver routine for the functionality of
CanonicalizeOSSALifetime that computes liveness and then rewrites copies using
boundary information. This change introduces splits the implementation of
canonicalizeValueLifetime into two parts: a first part called computeLiveness
and a second part called rewriteLifetimes. Internally canonicalizeValueLifetime
still just calls these two methods.
The reason why I am doing this is that it lets the move only object checker use
the raw liveness information computed before the rewriting mucks with the
analysis information. This information is used by the checker to compute the raw
liveness boundary of a value and use that information to determine the list of
consuming uses not on the boundary, consuming uses on the boundary, and
non-consuming uses on the boundary. This is then used by later parts of the
checker to emit our errors.
Some additional benefits of doing this are:
1. I was able to eliminate callbacks in the rewriting stage of
CanonicalOSSALifetimes which previously gave the checker this information.
2. Previously the move checker did not have access to the non-consuming boundary
uses causing us to always fail appropriately, but sadly not emit a note showing
the non-consuming use. I am going to wire this up in a subsequent commit.
The other change to the implementation of the move checker that this caused is
that I needed to add an extra diagnostic check for instructions that consume the
value twice or consume the value and use the value. The reason why this must be
done is that liveness does not distinguish in between different operands on the
same instruction meaning such an error would be lost.
A number of our existing tests put move-only types in optionals and
used them as arguments to generic type parameters. It turns out that
many of these appearances in the tests were not crucial for the test's
functionality itself.
The little coverage for force-unwrapping an Optional<moveOnly>
that was lost will be regained once we make these types sound.
I also tried to tidy up some of the tests with consistent naming for
operations on values, i.e., `consumeVal` and `borrowVal` functions.
NOTE: The additional errors that are occuring in the move only object checker is
b/c I tweaked checkDestructureUsesOnBoundary so that when it detects an error it
continues instead of returns. This ensures that we get more that we emit errors
for multiple violations instead of just the first one.
rdar://104900171
