Wrap SE-0481 into an upcoming feature until source incompatibilities are resolved

[nickolas-pohilets]

Wrapped into the feature:

• mutability of the weak capture
• changes in diagnostic of implicit self capture

Not wrapped into the feature:

• Allowing weak let declarations
• Warning about weak var declarations which are never written to (except closure captures).
• Removed redundant boxing of the weak and unowned captures

Most of the tests run both with feature off and on, but a couple of less relevant for the change - only with feature on (i.e. preserving status quo).

[nickolas-pohilets]

@swift-ci Please smoke test macOS platform

[nickolas-pohilets]

@swift-ci Please smoke test Linux platform

[nickolas-pohilets]

@swift-ci Please smoke test Windows platform

[rjmccall]

I don't think we need to move the entire feature behind a flag, just the change to the capture semantics. In that light, please call the feature something like WeakLetCaptures or ImmutableWeakCaptures.

[nickolas-pohilets]

@swift-ci Please smoke test

[nickolas-pohilets]

@swift-ci Please smoke test

[nickolas-pohilets]

@swift-ci Please Test Source Compatibility Release

[rjmccall]

Functionality LGTM, with a question about the VarDeclUsageChecker that can totally be dealt with in a follow-up. Some minor comment requests.

Would you mind preparing a cherry-pick PR against the release/6.2 branch? We'll need to get LSG sign-off before we can merge that one, but hopefully that comes tomorrow.

[rjmccall]

Would you mind preparing a cherry-pick PR against the release/6.2 branch? We'll need to get LSG sign-off before we can merge that one, but hopefully that comes tomorrow.

FYI, I think we're going to aim for 6.2.x for this at this point.

[nickolas-pohilets]

@swift-ci Please smoke test

[nickolas-pohilets]

@rjmccall, could pls respond to the unresolved discussions above. Or shall I just merge the PR as is?

[rjmccall]

Hey. So in the original PR, we talked about whether we needed the change that makes weak/unowned captures use a box. Apple has seen what seems to be a regression that's fixed by restoring that block and turning the condition into var->getInterfaceType()->is<ReferenceStorageType>() && !Context.LangOpts.hasFeature(Feature::ImmutableWeakCaptures). We haven't narrowed why this fixes the problem yet — the bug appears to be that code is seeing a nil'ed out weak reference when it shouldn't — but it's clearly not completely semantically equivalent, which is something we need to understand.

[nickolas-pohilets]

Interesting. Do you have a code sample that reproduces the issue (link to the GitHub issue would work)? Any other links to include in the code comment?

&& !Context.LangOpts.hasFeature(Feature::ImmutableWeakCaptures)

Does it mean that the issue does not reproduce with weak let? Does it reproduce when capturing a regular weak var with a feature enabled? Maybe we need to check for mutability of the declaration, and not for the feature flag.

Does it matter if it is a native or ObjC weak reference?

[rjmccall]

Interesting. Do you have a code sample that reproduces the issue (link to the GitHub issue would work)? Any other links to include in the code comment?

I believe the investigation was done by bisecting the compiler history and then looking at the implicated PRs; we haven't reduced a test case. Your PR isn't supposed to affect code generation for non-adopting projects, though, so it's definitely suspicious that it is.

&& !Context.LangOpts.hasFeature(Feature::ImmutableWeakCaptures)

Does it mean that the issue does not reproduce with weak let? Does it reproduce when capturing a regular weak var with a feature enabled? Maybe we need to check for mutability of the declaration, and not for the feature flag.

This is in code that I'm fairly certain hasn't adopted the feature at all. The actual effect of the tentative patch is therefore just to restore the old behavior, so there's no particular reason to think that the new condition is right.

var->supportsMutation() is definitely true for weak captures when the feature is disabled, right? So the patch is just pre-empting the special handling for non-escaping captures?

It is possible that the actual bug is further downstream, like maybe the capture handling in IRGen doesn't handle reference storage types properly.

Does it matter if it is a native or ObjC weak reference?

I would be surprised if this were the difference.

[nickolas-pohilets]

I believe the investigation was done by bisecting the compiler history and then looking at the implicated PRs; we haven't reduced a test case. Your PR isn't supposed to affect code generation for non-adopting projects, though, so it's definitely suspicious that it is.

I was trying to align capture behavior of the weak let with capture behavior with immutable single-field structs wrapping a weak reference. Which makes me suspect that there can be an existing issue with that as well.

var->supportsMutation() is definitely true for weak captures when the feature is disabled, right? So the patch is just pre-empting the special handling for non-escaping captures?

Yes.

Shall we for now merge a version where all the changes are wrapped into an experimental feature?

[rjmccall]

I believe the investigation was done by bisecting the compiler history and then looking at the implicated PRs; we haven't reduced a test case. Your PR isn't supposed to affect code generation for non-adopting projects, though, so it's definitely suspicious that it is.

I was trying to align capture behavior of the weak let with capture behavior with immutable single-field structs wrapping a weak reference. Which makes me suspect that there can be an existing issue with that as well.

var->supportsMutation() is definitely true for weak captures when the feature is disabled, right? So the patch is just pre-empting the special handling for non-escaping captures?

Yes.

Shall we for now merge a version where all the changes are wrapped into an experimental feature?

No, I don't think we need to go that far. Let's just restore this block unconditionally for now, and then we can look into what's causing the miscompile separately.

[nickolas-pohilets]

@swift-ci Please smoke test