[Sema] NFC - Make #15419 work with weak vars

[davezarzycki]

This extends the tuple liternal type checking optimization to handle weak variables.

[davezarzycki]

@swift-ci please smoke test

[rudkx]

I don't think this makes sense. weak can only be used on class and class-bound protocol types.

[davezarzycki]

Can you elaborate? The validation test suite passes on my Linux box.

[davezarzycki]

I think I get it now, the #15419 optimization doesn't optimize anything if the expression is not a tuple/dictionary/array literal, and weak variables will never be tuple/array/dictionary literals.

I guess this pull request is superficial. Do you find the resulting code to be prettier? Or would you rather have two switch statements?

And what about weak var (x,y,z) = (a,b,c)? Won't the optimization kick in then?

[rudkx]

Oh sorry, I just meant that it wouldn't be legal code.

Compiling:

class C {}
weak var x = (C(), C())

Results in:

/tmp/weak.swift:2:10: error: 'weak' may only be applied to class and class-bound protocol types, not '(C, C)'
weak var x = (C(), C())

I had originally tried to make this work, as well as things with inout on the RHS, and all forms of nested patterns. It would be nice to get the latter working.

Now having said that, it would produce a diagnostic faster (or at all the the case of expressions where we would otherwise bail out with "expression was too complex").

When I had a similar change I wasn't really able to convince myself that it was actually correct, though, since the RHS might be inferred to be an optional as well, so whether you're wrapping it in another optional or not really shouldn't depend on whether we knew we had an optional on the RHS before the type checker runs.

[rudkx]

And what about weak var (x,y,z) = (a,b,c)? Won't the optimization kick in then?

That's a tuple pattern, which already recursively decomposes the elements (see PatternKind::Tuple father down).

[davezarzycki]

Oh, duh, yes you're right about tuple patterns. As for weak variables, they're kind of gross. They intentionally let this work, which is why this pull request does what it does:

class C {}
let c = C()
let oc : C? = c
weak var w1 = c
weak var w2 = oc

Are faster weak diagnostics worth it?

[rudkx]

Are faster weak diagnostics worth it?

Probably not worth the effort. We've only had the exponential tuple type checking issue reported a few times. I happened to be looking into exponential cases involving arrays and dictionaries and noticed that the tuple case was pretty easy to fix for the simple pattern kinds, so I did that, and then spent way too much time trying to generalize it :).

[davezarzycki]

What about bypassing the optimization in the unowned or unmanaged cases, which like weak can never be tuples?

[rudkx]

What about bypassing the optimization in the unowned or unmanaged cases, which like weak can never be tuples?

I think it's fine to leave it as is, since we should report the error faster with the optimization in place.

[davezarzycki]

I get the impression that you want me to abandon this pull request then. For the record, it seems lame to me unnecessarily different logic for weak versus unowned/unmanaged, especially when the work to unify the code has already been done in this PR. Please confirm that you want me to abandon this PR. Thanks.

[slavapestov]

I think this PR is reasonable as a cleanup.

[rudkx]

@davezarzycki I'm not suggesting you abandon the PR. I'm just not sure it's worth trying to fix this case, and as I mentioned I abandoned a similar change because I couldn't convince myself it would work.

I just spent a few minutes putting together a test case, and this is the sort of thing that I think will be broken if you were to use the same type variable that appears on the RHS and then attempt to wrap that type variable in an optional: #15557

There may be some way to generalize this to this case as well as other nested pattern kinds, I'm just not sure it's worth the effort.

[davezarzycki]

I updated this PR to handle #15557.

@swift-ci please smoke test

[davezarzycki]

BTW – Whether this PR is "worth the effort" or not, good things have happened because of it. Like your test in #15557, and my understanding of this code improved too (which improved the QoI of my branch). Thanks. :-)

[rudkx]

Sure, I don’t mean to tell you how to spend your time for sure, and appreciate any efforts towards generalizing things to remove special cases. I’ll take another look at your updated code when I am in front of a keyboard again rather than on my phone.

[davezarzycki]

@swift-ci please smoke test

[rudkx]

@swift-ci Please smoke test

[rudkx]

@swift-ci Please test source compatibility

[rudkx]

@swift-ci Please test source compatibility

[davezarzycki]

Unless I'm reading the source compatibility results incorrectly, that failure looks unrelated:

UPasses:
  UPASS: https://bugs.swift.org/browse/SR-7287, ReSwift, 3.0, db35c2, ReSwift-watchOS, generic/platform=watchOS
  UPASS: https://bugs.swift.org/browse/SR-7287, ReSwift, 3.0, db35c2, ReSwift-tvOS, generic/platform=tvOS
  UPASS: https://bugs.swift.org/browse/SR-7287, ReSwift, 3.0, db35c2, ReSwift-macOS, generic/platform=macOS
  UPASS: https://bugs.swift.org/browse/SR-7287, ReSwift, 3.0, db35c2, ReSwift-iOS, generic/platform=iOS

Also, I'm going to need to rebase this to fix a merge conflict due to #15545

[davezarzycki]

@swift-ci please smoke test

[davezarzycki]

Hi @rudkx – Out of respect to the tentative "make reviewing mandatory" policy discussion, can you please approve, decline, or actionably comment on this change? Thanks!

[rudkx]

One request: Please add a test case showing that we are now fast when weak is used.

[davezarzycki]

@rudkx – I'm not familiar with performance sensitive tests. How does one show that a particular semantic scenario is "fast"?

[rudkx]

Actually I just realized that I didn't do that with my original change. Let's merge your change and then I will set up a validation test with my original test update and a new test for weak.

We have two mechanisms for this, neither of which is perfect. Timer-based testing, and scale-test-based testing. The existing tests sit under validation-test/Sema/type_checker_perf and I inadvertently just left the tests that I had written for these patterns in the pattern test rather than moving them over.

[rudkx]

Merge at will, thanks!