I think validating this was an oversight from the bringup of coroutines. I
discovered this while writing test cases for coroutine lifetime extension. I
realized it was possible to write a test case that should have triggered this
but was not.
I added some tests to make sure that we continue to flag this in the future.
rdar://69597888
I don't have a test case for this bug based on the current code. But
the fix is clearly needed to have a unique AccessStorage object for
each property. The AccessPath commits will contain test cases for this
functionality.
This patch includes a large number of changes to make sure that:
1. When ExtInfo values are created, we store a ClangTypeInfo if applicable.
2. We reduce dependence on storing SIL representations in ASTExtInfo values.
3. Reduce places where we sloppily create ASTExtInfo values which should
store a Clang type but don't. In certain places, this is unavoidable;
see [NOTE: ExtInfo-Clang-type-invariant].
Ideally, we would check that the appropriate SILExtInfo does always store
a ClangTypeInfo. However, the presence of the HasClangFunctionTypes option
means that we would need to condition that assertion based on a dynamic check.
Plumbing the setting down to SILExtInfoBuilder's checkInvariants would be too
much work. So we weaken the check for now; we should strengthen it once we
"turn on" HasClangFunctionTypes and remove the dynamic feature switch.
The leak happened in this scenario:
1. A function becomes dead and gets deleted (which means: it gets added to the zombie-list)
2. A function with the same name is created again. This can happen with specializations.
In such a case we just removed the zombie function from the zombie-list without deleting it.
But we cannot delete zombie functions, because they might still be referenced by metadata, like debug-info.
Therefore the right fix is to resurrect the zombie function if a new function is created with the same name.
rdar://problem/66931238
For example, the completion below would trigger error recovery within the
closure, which we recover from by skipping to the first inner closure's right
brace. The fact that we recovered though, was not recorded. The closure is
treated as still being an error, triggering another recovery after it that
skips over the 'Thing' token, giving a lone closure expression, rather than a
call.
CreateThings {
Thing { point in
print("hello")
point.#^HERE^#
}
Thing { _ in }
}
This isn't an issue for code completion when the outer closure is a regular
closure, but when it's a function builder, invalid elements result in no types
being applied (no valid solutions) and we end up with no completion results.
The fix here is removing the error status from the parser result after the
initial parser recovery.
The reason why is that due to ARCCodeMotion, they could have moved enough that
the SourceLoc on them is incorrect. That is why you can see in the tests that I
had to update, I am moving the retain to the return statement from the body of
the function since the retain was now right before the return.
I also went in and cleaned up the logic here a little bit. What we do now is
that we have a notion of instructions that we /always/ infer SourceLocs for (rr)
and ones that if we have a valid non-inlined location we use (e.x.:
allocations).
This mucked a little bit with my ability to run SIL tests since the SIL tests
were relying on this not happening to rr so that we would emit remarks on the rr
instructions themselves. I added an option that disables the always infer
behavior for this test.
That being said at this point to me it seems like the SourceLoc inference stuff
is really tied to OptRemarkGenerator and I am going to see if I can move it to
there. But that is for a future commit on another day.
Expand to other cases where we emit a
native-to-foreign thunk, but didn't previously
return true. Because this now includes @objc
entry-points for methods, adjust the linking logic
so it can maintain more restrictive linkages for
things like private decls.
When bringing up ownership I thought that it might be useful to distinguish
"normal users" that just require liveness and are real transitive users vs
implicit users that require liveness (I provide a constrasting example
below). Turns out this distinction did not provide any benefit, so I am ripping
it out so I can refactor this code to be used in other parts of the compiler
where we only want a single array of "normal users".
This is just a pure refactor. NFCI.
--------------------------------------------------------------------------------
An example of the former would be an apply that uses an owned value as a
guaranteed parameter:
```
bb0(%0 : @owned $Klass):
apply %func(%0) : $@convention(thin) (@guaranteed $Klass) -> ()
```
while an example of the latter is an end_apply of a coroutine that takes an
owned value as a guaranteed parameter:
```
bb0(%0 : @owned $Klass):
(%result, %token) = begin_apply %func(%0)
...
end_apply %token
destroy_value %0
```
In the latter, we can not move the destroy_value past the end_apply.
I am currently working on updating SimplifyCFG for ownership. One thing that I
am finding that I need is the ability to compute the lifetime of a guaranteed
argument from a checked_cast_br/switch_enum in order to convert said
instructions to a br. The issue comes from br acting as a consuming (lifetime
ending) use of a borrowed value, unlike checked_cast_br/switch_enum (which are
treated like forwarding instructions). This means that during the conversion we
need to insert a begin_borrow on the value before the new br instruction and
then create an end_borrow at the end of the successor block's argument's
lifetime.
By refactoring out this code from the ownership verifier, I can guarantee that
said lifetime computation will always match what the ownership verifier does
internally preventing them from getting out of sync.
At all call-sites, the extInfo passed as the third argument is computed directly
from the second argument, so we compute it directly in getBridgedFunctionType.
