Correctly distinguish between tuple fields and associated values in patterns #26357

varungandhi-apple · 2019-07-25T21:47:22Z

See the second commit message for more details.

I'm not sure whether I should've broken things up into multiple commits, since there are couple of unrelated things that I noticed and cleaned up, but they're not really related to the main content of the PR.

Fixes SR-11160 and related rdar://problem/53312914.

varungandhi-apple · 2019-07-25T22:31:03Z

@swift-ci please smoke test

varungandhi-apple · 2019-07-25T23:43:43Z

Skimming through the failure logs, I think the only failure is because of SwiftPM where they're relying on buggy behavior. 😢 Looking at this line and this one:

enum Z { case z(Int, Int) }
func f() {
  switch Z.z(1, 2) {
  case .z(let (a, b)): print(a, b)
  }
}

should compile fine for source compat but it doesn't now. There is a similar problem with this switch.

I'm trying to think of how we could make the fix without breaking them...

theblixguy · 2019-07-25T23:48:28Z

cc @aciidb0mb3r

aciidgh · 2019-07-26T00:10:48Z

I am fine with fixing the SwiftPM code but the source compatibility probably needs to be discussed somewhere ~

varungandhi-apple · 2019-07-26T03:44:50Z

@aciidb0mb3r thanks! Actually, I think it is a blessing in disguise -- we caught this sooner rather than later thanks to SwiftPM 😅. I've added this hazard to SR-11212.

varungandhi-apple · 2019-07-30T22:16:15Z

Current status: we have a couple of cases failing on master (non-exhaustive match) thanks to SR-11212.

https://github.com/apple/swift/blob/039bd76159f34c770b272fde3f7a62e888a9a7e1/test/Sema/exhaustive_switch.swift#L1226-L1236

Everything else still works.

Also fixes rdar://problem/53312914. The fact that ParenType is being used to distinguish the two cases makes me uncomfortable but I don't have better ideas. Related: - Fixed a bug in pattern projection which I encountered once I fixed SR-11160. This is the 3 line change around conArgSpaces. - Opened SR-11212 for ill-typed patterns that are permitted to compile. Unrelated cleanup: - Removed a redundant switch case in 'isSubspace'. - Added names for referenced papers for faster lookup.

…tly.

varungandhi-apple · 2019-08-02T02:00:38Z

@swift-ci please smoke test

varungandhi-apple · 2019-08-02T02:04:21Z

Notes for reviewer(s):

As an aside, I have added some examples for PatternKinds but I'm not sure of all of those, so I'd appreciate some help with fixing them.
I've addressed the core issues (we emit a warning, not an error), so the PR is ready for review (barring point 3).
~~I will add more test cases that make sure everything runs as expected (i.e. we don't have a type-checks fine but some inconsistency causes a runtime crash).~~ Done.

varungandhi-apple · 2019-08-02T23:36:23Z

@swift-ci please smoke test

xedin

I think this looks great, @varungandhi-apple! I have left a small comment about Type usage.

xedin · 2019-08-12T15:44:20Z

lib/Sema/TypeCheckPattern.cpp

+//   2. pat ~ (P1, ..., Pm) (m >= 2)
+void implicitlyUntuplePatternIfApplicable(TypeChecker *TC,
+                                          Pattern *&enumElementInnerPat,
+                                          const Type &enumPayloadType) {


Just an FYI - Type is an immutable word sized container for TypeBase * so there is no need to pass it by const &

xedin · 2019-08-12T16:23:03Z

@varungandhi-apple I think exhaustiveness checker should be consistent with SE-0110 so we should consider these source breaks as fixing invalid implementation behavior. It still needs to be discussed on the forums though.

Make sure that we still emit warnings even if the let bindings are in different positions.

There is a small chance that codegen and everything works fine, but the generated code is wrong because of mismatched expectations on two sides, so we have some tests to catch that.

varungandhi-apple · 2019-08-12T19:52:30Z

@swift-ci please smoke test

jrose-apple · 2019-08-13T16:04:58Z

I think exhaustiveness checker should be consistent with SE-0110 so we should consider these source breaks as fixing invalid implementation behavior.

That's not a sufficient reason for a source-break these days. Remember, we're not just talking about a developer's code; we're also talking about the packages they are using, and starting with Swift 5.1 any already-compiled binary frameworks with inlinable code as well.

xedin · 2019-08-13T16:20:47Z

I'm trying to say that we should, I'm simplify pointing out my opinion on the matter.

Since the introduction of swiftlang#26357/SR-11160 the TestJSONEncoder seems to have been emitting the new warnings related to tuples being incorrectly pattern matched. The changes remove the incorrect code (removing the parenthesis around the pattern). Should not have any other effect (and the effect was not normally visible if the test was successful, anyway).

varungandhi-apple added 4 commits July 31, 2019 16:29

Add test case characterizing issue in SR-11160.

bbf7e7a

Fix related bug in pattern projection, handling the tuple case correc…

afd83ba

…tly.

[Docs] Add examples for PatternKinds.

8eaf81b

varungandhi-apple requested review from jrose-apple and xedin August 2, 2019 02:05

xedin approved these changes Aug 12, 2019

View reviewed changes

varungandhi-apple added 3 commits August 12, 2019 09:58

Implicitly untuple patterns if applicable and add warnings.

e365b6b

Add test cases for more permutations.

25e5b15

Make sure that we still emit warnings even if the let bindings are in different positions.

Add codegen test cases to make sure that patching patterns works.

881de88

There is a small chance that codegen and everything works fine, but the generated code is wrong because of mismatched expectations on two sides, so we have some tests to catch that.

varungandhi-apple merged commit 68fb3b1 into swiftlang:master Aug 12, 2019

varungandhi-apple deleted the vg-fix-nested-exhaustiveness-check branch August 12, 2019 20:59

drodriguez mentioned this pull request Sep 5, 2019

Remove warnings from TestJSONEncoder compilation. #27044

Merged

varungandhi-apple mentioned this pull request Nov 7, 2019

[Diagnostics] Improve diagnostics for implicit (un)tupling. #28076

Merged

typesanitizer mentioned this pull request Aug 12, 2019

[SR-11160] Inconsistent behavior with exhaustive switching over optionals of tuples #53557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correctly distinguish between tuple fields and associated values in patterns #26357

Correctly distinguish between tuple fields and associated values in patterns #26357

Uh oh!

varungandhi-apple commented Jul 25, 2019 •

edited

Loading

Uh oh!

varungandhi-apple commented Jul 25, 2019

Uh oh!

varungandhi-apple commented Jul 25, 2019 •

edited

Loading

Uh oh!

theblixguy commented Jul 25, 2019

Uh oh!

aciidgh commented Jul 26, 2019 •

edited

Loading

Uh oh!

varungandhi-apple commented Jul 26, 2019

Uh oh!

varungandhi-apple commented Jul 30, 2019

Uh oh!

varungandhi-apple commented Aug 2, 2019

Uh oh!

varungandhi-apple commented Aug 2, 2019 •

edited

Loading

Uh oh!

varungandhi-apple commented Aug 2, 2019

Uh oh!

xedin left a comment

Uh oh!

xedin Aug 12, 2019

Uh oh!

xedin commented Aug 12, 2019

Uh oh!

varungandhi-apple commented Aug 12, 2019

Uh oh!

jrose-apple commented Aug 13, 2019

Uh oh!

xedin commented Aug 13, 2019

Uh oh!

Uh oh!

Correctly distinguish between tuple fields and associated values in patterns #26357

Correctly distinguish between tuple fields and associated values in patterns #26357

Uh oh!

Conversation

varungandhi-apple commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varungandhi-apple commented Jul 25, 2019

Uh oh!

varungandhi-apple commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theblixguy commented Jul 25, 2019

Uh oh!

aciidgh commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varungandhi-apple commented Jul 26, 2019

Uh oh!

varungandhi-apple commented Jul 30, 2019

Uh oh!

varungandhi-apple commented Aug 2, 2019

Uh oh!

varungandhi-apple commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varungandhi-apple commented Aug 2, 2019

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

xedin commented Aug 12, 2019

Uh oh!

varungandhi-apple commented Aug 12, 2019

Uh oh!

jrose-apple commented Aug 13, 2019

Uh oh!

xedin commented Aug 13, 2019

Uh oh!

Uh oh!

varungandhi-apple commented Jul 25, 2019 •

edited

Loading

varungandhi-apple commented Jul 25, 2019 •

edited

Loading

aciidgh commented Jul 26, 2019 •

edited

Loading

varungandhi-apple commented Aug 2, 2019 •

edited

Loading