Sema: Support optional promotion of tuple patterns #63992

AnthonyLatsis · 2023-03-01T08:39:30Z

As with enum element patterns — at least the unqualified ones — this allows tuple patterns to omit syntactic noise in the form of enclosing .some(...) or trailing ?.

enum E { case a }

let t: (E, E)?
switch t {
  case nil: break
  case (.a, .a): break
}

AnthonyLatsis · 2023-03-01T08:39:51Z

@swift-ci please smoke test macOS

xedin · 2023-03-01T08:58:52Z

A general note: Any changes made to TypeCheckPattern.cpp need to be reflected in CSGen otherwise the behavior in multi-statement closures and result builder transformed closures would not change…

hamishknight · 2023-03-01T11:25:21Z

FWIW I'm also hoping to eventually remove coercePatternToType such that the constraint system pattern solving implementation is always used (main...hamishknight:swift:patte)

AnthonyLatsis · 2023-03-07T19:54:29Z

@xedin I took care of all the closure stuff; thanks for the reminder.

xedin · 2023-03-07T19:56:12Z

Great! I'll try to take a look today.

AnthonyLatsis · 2023-03-07T20:01:17Z

@swift-ci please smoke test macOS

hamishknight

Some initial comments from a quick look through, I can look in more detail tomorrow. I think you'll need to adjust the logic in getTypeForPattern rather than reversing the contextual conversion constraint though

lib/Sema/CSSyntacticElement.cpp

hamishknight · 2023-03-07T21:28:50Z

test/Sema/pattern_matching_optional_promotion.swift.gyb

+        // FIXME: '.a' is OK but 'E.a' is not?
+        case E.a: () // expected-error {{enum case 'a' is not a member of type 'E?'}}


Heh yeah, this is something I'm planning on fixing

lib/Sema/CSSyntacticElement.cpp

AnthonyLatsis · 2023-04-11T18:11:30Z

@swift-ci please smoke test macOS

AnthonyLatsis · 2023-04-11T18:23:32Z

@xedin How does this look? As of now the only known issue is a failure in the case of a for-in loop where the sequence is an array literal. Haven’t figured that one out yet.

AnthonyLatsis · 2023-04-12T04:04:27Z

test/Sema/pattern_matching_optional_promotion.swift.gyb

+  contexts = ['do_block', 'closure', 'result_builder_closure']
+  def get_test_case_introducer(context):
+    if context == 'do_block':
+      return 'do'
+    elif context == 'closure':
+      return 'let _: () -> Void ='
+    elif context == 'result_builder_closure':
+      return '@DummyBuilder var x: Void'


Does anyone have an opinion on removing the result builder context now that constraint generation for closures has been unified? It makes these tests somewhat harder to codify.

Fine with me, as long as you add a couple of simple test cases within result builders

xedin · 2023-04-15T00:54:52Z

@hamishknight could you please take a look? I will try to get some time for this next week.

hamishknight

Thanks! A few comments, but otherwise this LGTM. I'll let @xedin have the final say though.

hamishknight · 2023-04-17T11:12:20Z

lib/Sema/TypeCheckPattern.cpp

+    // switch 0 {
+    //   case (int: 0): break
+    //   default: break
+    // }
+    // Do we really want/need to support this?


Urgh no, I don't think we want to support that. The constraint system will already reject such cases (including if case, as that already goes through the constraint system even outside of closures). Hopefully we can drop it for switch statements outside of closures without much fallout.

lib/Sema/TypeCheckPattern.cpp

test/SILGen/pattern_matching_optional_promotion.swift.gyb

test/Sema/pattern_matching_optional_promotion.swift.gyb

lib/Sema/TypeCheckPattern.cpp

hamishknight · 2023-04-17T13:12:05Z

include/swift/Sema/Constraint.h

+  /// The first type is an optional of non-negative depth of the second type,
+  /// ignoring lvalueness (because the constraint is currently applied only to
+  /// pattern types, which are always rvalue). For example, given a RHS of
+  /// 'Int', a LHS of 'Int' or 'Int??' would satisfy this constraint.
+  EqualOrOptional,


It doesn't quite feel right to me to have Equal in the name since the constraint isn't doing rvalue conversions, it's just asserting that no lvalues are present. Additionally, I think if we did ever want to apply this constraint to something else that could have lvalues, I think it would probably make most sense to preserve lvalueness like OptionalObject, as lvalues are generally preserved through optional chains.

How about something like UnwrappingOptionals? I think the plurality also helps make it clear it unwraps an arbitrary number of optionals.

lib/Sema/TypeCheckPattern.cpp

…ernToType`

xedin · 2023-05-01T18:51:43Z

lib/Sema/CSGen.cpp

+                                             TVO_CanBindToNoEscape);
+          CS.addConstraint(
+              ConstraintKind::EqualOrOptional, tyVar, patternTy,
+              locator.withPathElement(LocatorPathElt::PatternMatch(pattern)));


As far as I understand this problem, what we really want is not to look through optionals but disregard the structure of the tuple pattern and let context impose it. So how about instead of this new constraint we actually model it this way:

Create a "tuple" pattern type variable (just like you did);

For each pattern in the tuple create a member constraint to i.e. <tuple var>[member: .<index or name>] == <element-var>;

Set tuple variable as a type of the tuple pattern.

This scheme should make it possible to infer tuple type from context and look through optionals in pattern matching context while resolving individual tuple elements.

WDYT?

cc @hamishknight @AnthonyLatsis

Sorry for the late reply here! I'm curious what the benefit of that approach would be, as wouldn't the new tuple member constraint need to handle the same optional unwrapping that the EqualOrOptional constraint currently handles, plus it would need to have tuple matching logic? Or is there something I'm missing here?

Or is the aim to have the <tuple var> be inferred as a non-optional? I should note that the constraint which attaches the tuple var to the surrounding pattern would fail with that, as it should only allow conversions that propagate into pattern, not out of them (though I'm becoming increasingly of the opinion that we should limit those to equality constraints where we can).

There are no new constraints and existing member already handles unwrapping of optional base type.

Ah, I see! Sorry, I totally missed that. I guess one worry with that approach is we don't want to allow e.g (x: Int, y: Int) to match with (y: Int, x: Int), or even worse some nominal type T that just so happens to have the right members.

One other thing that we have to contend with is that we currently can use the tuple type of the pattern to introduce conversions, e.g this is currently allowed:

if case (x: 0, y: 0) = (0, 0) {}

Also just discovered that we currently allow duplicate labels in tuple patterns :(

if case (x: 0, x: 0) = (0, 0) {}

I don’t think it should be too hard to maintain the structural matching, we can even add additional information about index/label to member constraint to make sure that the base tuple does indeed match structurally.

AnthonyLatsis requested review from hborla, slavapestov and xedin as code owners March 1, 2023 08:39

AnthonyLatsis force-pushed the tuple-pattern-optional-promotion branch 2 times, most recently from f3f6be7 to 85d8479 Compare March 7, 2023 19:54

AnthonyLatsis requested a review from hamishknight March 7, 2023 19:54

AnthonyLatsis force-pushed the tuple-pattern-optional-promotion branch from 85d8479 to 9d9a385 Compare March 7, 2023 20:00

hamishknight requested changes Mar 7, 2023

View reviewed changes

AnthonyLatsis force-pushed the tuple-pattern-optional-promotion branch from 9d9a385 to a53186e Compare April 11, 2023 18:09

AnthonyLatsis requested a review from hamishknight April 11, 2023 18:10

AnthonyLatsis commented Apr 12, 2023

View reviewed changes

AnthonyLatsis changed the title ~~Sema: Support optional promotion of tuple patterns when pattern matching~~ Sema: Support optional promotion of tuple patterns Apr 15, 2023

hamishknight reviewed Apr 17, 2023

View reviewed changes

AnthonyLatsis added 3 commits April 18, 2023 05:49

[NFC] Sema: Tidy up a bit of control flow in `TypeChecker::coercePatt…

2888edc

…ernToType`

[NFC] Sema: Tidy up tuple pattern handling in coercePatternToType

236e88d

Sema: Optional-promote tuple patterns during solution application

1b901a9

AnthonyLatsis mentioned this pull request Apr 18, 2023

Bad diagnostic when matching tuple pattern to existential #65243

Open

Sema: Allow optional promotion of tuple patterns

44b91a6

AnthonyLatsis force-pushed the tuple-pattern-optional-promotion branch from a53186e to 44b91a6 Compare April 18, 2023 13:56

xedin reviewed May 1, 2023

View reviewed changes

		// FIXME: '.a' is OK but 'E.a' is not?
		case E.a: () // expected-error {{enum case 'a' is not a member of type 'E?'}}

Sema: Support optional promotion of tuple patterns #63992

Are you sure you want to change the base?

Sema: Support optional promotion of tuple patterns #63992

Uh oh!

Conversation

AnthonyLatsis commented Mar 1, 2023

Uh oh!

AnthonyLatsis commented Mar 1, 2023

Uh oh!

xedin commented Mar 1, 2023

Uh oh!

hamishknight commented Mar 1, 2023

Uh oh!

AnthonyLatsis commented Mar 7, 2023

Uh oh!

xedin commented Mar 7, 2023

Uh oh!

AnthonyLatsis commented Mar 7, 2023

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnthonyLatsis commented Apr 11, 2023

Uh oh!

AnthonyLatsis commented Apr 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Apr 15, 2023

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin May 1, 2023 •

edited

Loading