[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. #1383

gregomni · 2016-02-21T00:35:49Z

There was a thread on swift-evolution back on Jan 23rd about adding this, and the response from the team was that it was something desirable but that you just hadn’t had time to do it so far.

Parser now accepts multiple patterns in switch cases that contain variables. Every pattern must contain the same variable names, but can be in arbitrary positions. New error for variable that doesn't exist in all patterns.

Sema now checks cases with multiple patterns that each occurrence of a variable name is bound to the same type. New error for unexpected types.

SILGen now shares a basic block for a switch case that contains multiple patterns even if there are variables. That BB takes incoming arguments from each applicable pattern match emission with the specific variables for the pattern that matched.

Added simple tests for the Parse and Sema functionality. I’d greatly appreciate suggestions on what sort of SILGen tests would be helpful. (The existing dominator assertions of variable usage and BB assertions on predecessor args caught a bunch of my mistakes along the way, so I’m pretty confident this all works, but how do I help make everyone else be confident too…)

I’m also not terribly happy with the managed value cleanup stuff between the pattern BBs and case body BBs, which works, but seems like the code could be cleaner. Again, suggestions greatly appreciated.

jckarter · 2016-02-21T01:57:59Z

This looks awesome. Do you have any SILGen tests?

gregomni · 2016-02-21T02:37:59Z

@jckarter I've been looking at the generated SIL with various examples by hand, but I'm not sure what ought to actually be checked in as tests.

Here's a simple example: https://gist.github.com/gregomni/cfaef124bd943168f266
with resulting SIL: https://gist.github.com/gregomni/3fdf53b2c5bdb9f621ab

Mostly verifying that the correct tuple elements get passed into the case block, that the bits of the pattern that aren't used are still released in the pattern emission blocks. But clearly I need more work with the managed cleanups part, since I'm seeing now that %21 is still being leaked. Argh.

Is there an easier / better way I haven't found to transfer a +1 value from one BB as an arg to another BB? I'm having a tough time with wanting the SILGenPattern code structure (and thus the cleanup scopes) being different from the order I'd actually like to generate the SIL.

jckarter · 2016-02-21T03:08:11Z

There's a ConditionalValue abstraction in Condition.h, though I don't think it'll be easy to fit into the framework of SILGenPattern. I think you want to separate the scope in which the patterns are evaluated from the scope of the case body they lead into. That way, you can forward the bindings selectively from the pattern evaluation to the case body basic block and let everything else get cleaned up when you leave the pattern's scope. @rjmccall might be able to help here too.

gregomni · 2016-02-21T03:17:36Z

Yep, I think you are right, the single scope per case instead of also per pattern is at the root of my troubles. Thanks for looking, and sorry for being a bit premature!

gregomni · 2016-02-21T19:35:11Z

Ah, I'd been staring at things too long last night. That example SIL is actually fine, what I was thinking of then as an extra retain_value of %21 is consumed by being an @owned argument to a call. So I'm back to still needing to write some SILGen tests.

jrose-apple · 2016-02-22T17:27:56Z

lib/SILGen/SILGenFunction.h

@@ -945,6 +945,8 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction

  void emitConditionalPBD(PatternBindingDecl *PBD, SILBasicBlock *FailBB);

+  void usingImplicitVariablesForPattern(Pattern *pattern, CaseStmt *stmt,
+                                        const std::function<void(void)> &f);


Nitpick: we have llvm::function_ref for generalized functions that don't need to be copied to the heap.

jrose-apple · 2016-02-22T17:31:39Z

This is pretty great.

jckarter · 2016-02-22T18:27:42Z

Another thing to test: What happens if you try to access a pattern variable from another pattern in the same case?

switch x {
case let a,
     a: // Ought to be an error
}

jrose-apple · 2016-02-22T19:16:18Z

Can you also add code completion tests? They don't have to produce the best results, but they shouldn't crash. Interesting cases off the top of my head:

Completing in a second pattern
Completing in a body with multiple patterns with bindings
Completing in a body where there's an error in the patterns

Completion tests live in test/IDE/.

gregomni · 2016-02-23T02:16:11Z

@jckarter Added test for accessing a pattern var from another pattern in the same case. This would be a problem if it made it through the parser, because it does think that the variable is defined, but it gets caught by the check for the second pattern needing to also define the variable.

Also improved that error message so that it says 'a' must appear "as a variable" so that the diagnosis makes more sense in this case.

@jrose-apple Fixed to use function_ref, thanks for the pointer! And added the code completion tests you suggested. They run and don't produce anything surprising, although ideally it wouldn't suggest using variables defined in the first pattern as an expression in the second (or subsequent) pattern. Added a FIXME for that. Are there docs or other info you could point me at to get started understanding swiftIDE? Not familiar with that part of the code yet, and nothing in the docs folder jumps out as relevant.

Thanks!

jrose-apple · 2016-02-23T02:33:45Z

I think formally the umbrella term for var and let is "binding"; "variable" would just mean var. Maybe "%0 must be bound in every pattern"?

For the swiftIDE question…@nkcsgexi?

gregomni · 2016-02-23T03:46:02Z

You're right, "must be bound" is a better way of putting it.

gregomni · 2016-02-23T15:10:08Z

FWIW, the code completion including the previous pattern binding var decls while in the next pattern is not a regression, it's been that way forever.

jrose-apple · 2016-02-23T17:20:25Z

test/IDE/complete_pattern.swift

+    }
+}
+
+// MULTI_PATTERN_2: Begin completions


Please check that a is part of the output here too!

nkcsgexi · 2016-02-23T17:52:19Z

Sorry for missing this earlier. @gregomni A good start point to understand how Swift code completion works is by studying several straight-forward completion kinds. One example is completing import decl, you can find it at CodeCompletion.swift:4012.

gregomni · 2016-02-24T02:23:23Z

Added the completion test checks @jrose-apple suggested and squashed.

Thanks for the pointer @nkcsgexi! I'll look into improving the completion for additional pattern next. To me it seems like it would make sense for that to be a separate pull.

jrose-apple · 2016-02-24T02:56:08Z

Thumbs-up from me, leaving it to Joe to say whether it's fully ready. (I didn't look too closely at the code, but it looked okay from a skim.)

jckarter · 2016-02-24T19:24:53Z

LGTM. Nice work!

jckarter · 2016-02-24T19:24:58Z

@swift-ci Please test

jckarter · 2016-02-24T23:14:58Z

Test failure looks unrelated. I'll try again when the builders are clear.

jckarter · 2016-02-24T23:15:39Z

@swift-ci Please test

jckarter · 2016-02-24T23:16:44Z

Or first, @gregomni, can you rebase? Are you able to build this branch locally?

Parser now accepts multiple patterns in switch cases that contain variables. Every pattern must contain the same variable names, but can be in arbitrary positions. New error for variable that doesn't exist in all patterns. Sema now checks cases with multiple patterns that each occurence of a variable name is bound to the same type. New error for unexpected types. SILGen now shares basic blocks for switch cases that contain multiple patterns. That BB takes incoming arguments from each applicable pattern match emission with the specific var decls for the pattern that matched. Added tests for all three of these, and some simple IDE completion sanity tests.

gregomni · 2016-02-25T00:01:49Z

This commit e249fd6 changed the order of arguments to RValue.forwardInto(). Should've rebased and retested when I was squashing, sorry! Reran all the tests locally to make sure, that looked like the only issue.

gregomni · 2016-02-25T17:06:41Z

@jckarter Should be good to go now!

jckarter · 2016-02-25T17:46:02Z

@swift-ci Please test

jckarter · 2016-02-25T17:46:09Z

Thanks @gregomni!

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns.

jrose-apple · 2016-02-25T22:19:29Z

Hooray! This deserves an update to CHANGELOG.md. :-)

lattner · 2016-03-24T03:54:10Z

@gregomni Please update the changelog, and include a link to SE-0043, thanks!

gregomni · 2016-03-24T04:16:08Z

@lattner Done, thanks!

gregomni changed the title ~~[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns.~~ WIP [Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. Feb 21, 2016

gregomni changed the title ~~WIP [Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns.~~ [Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. Feb 22, 2016

jrose-apple reviewed Feb 22, 2016
View reviewed changes

jrose-apple reviewed Feb 23, 2016
View reviewed changes

gregomni force-pushed the switch-cases branch from 4a6be65 to 88f2dea Compare February 24, 2016 02:19

gregomni force-pushed the switch-cases branch from 88f2dea to 7848efe Compare February 24, 2016 23:25

gregomni force-pushed the switch-cases branch from 7848efe to 63f810d Compare February 24, 2016 23:46

jckarter self-assigned this Feb 25, 2016

jckarter added a commit that referenced this pull request Feb 25, 2016

Merge pull request #1383 from gregomni/switch-cases

a416650

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns.

jckarter merged commit a416650 into swiftlang:master Feb 25, 2016

gregomni mentioned this pull request Feb 29, 2016

Improve var decl completion in switch cases #1481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. #1383

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. #1383

gregomni commented Feb 21, 2016

jckarter commented Feb 21, 2016

gregomni commented Feb 21, 2016

jckarter commented Feb 21, 2016

gregomni commented Feb 21, 2016

gregomni commented Feb 21, 2016

jrose-apple Feb 22, 2016

jrose-apple commented Feb 22, 2016

jckarter commented Feb 22, 2016

jrose-apple commented Feb 22, 2016

gregomni commented Feb 23, 2016

jrose-apple commented Feb 23, 2016

gregomni commented Feb 23, 2016

gregomni commented Feb 23, 2016

jrose-apple Feb 23, 2016

nkcsgexi commented Feb 23, 2016

gregomni commented Feb 24, 2016

jrose-apple commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

gregomni commented Feb 25, 2016

gregomni commented Feb 25, 2016

jckarter commented Feb 25, 2016

jckarter commented Feb 25, 2016

jrose-apple commented Feb 25, 2016

lattner commented Mar 24, 2016

gregomni commented Mar 24, 2016

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. #1383

[Parse/Sema/SILGen] Variables in 'case' labels with multiple patterns. #1383

Conversation

gregomni commented Feb 21, 2016

jckarter commented Feb 21, 2016

gregomni commented Feb 21, 2016

jckarter commented Feb 21, 2016

gregomni commented Feb 21, 2016

gregomni commented Feb 21, 2016

jrose-apple Feb 22, 2016

Choose a reason for hiding this comment

jrose-apple commented Feb 22, 2016

jckarter commented Feb 22, 2016

jrose-apple commented Feb 22, 2016

gregomni commented Feb 23, 2016

jrose-apple commented Feb 23, 2016

gregomni commented Feb 23, 2016

gregomni commented Feb 23, 2016

jrose-apple Feb 23, 2016

Choose a reason for hiding this comment

nkcsgexi commented Feb 23, 2016

gregomni commented Feb 24, 2016

jrose-apple commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

jckarter commented Feb 24, 2016

gregomni commented Feb 25, 2016

gregomni commented Feb 25, 2016

jckarter commented Feb 25, 2016

jckarter commented Feb 25, 2016

jrose-apple commented Feb 25, 2016

lattner commented Mar 24, 2016

gregomni commented Mar 24, 2016