[SE-0276] Support multiple patterns in catch clauses #27776

owenv · 2019-10-18T18:06:56Z

e.g. do { ... } catch MyErr.a(let code), MyErr.b(let code) { ... }

The approach I took here was to get rid of CatchStmt entirely and reuse CaseStmt in DoCatchStmt. If anyone has time to take a look, I'd appreciate feedback on whether this seems like a good approach before I pitch this on the forums. Initially I tried keeping CatchStmt around and just modifying it to hold CaseLabelItems, but it resulted in a lot of code duplication in Sema and SILGen. This approach is cleaner overall, but there are still some rough edges (in the profiling code, for instance).

Requires the following PRs for testing:

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

owenv · 2019-10-21T03:43:04Z

Updated to fix code completion tests, this now passes the test suite

rintaro · 2019-10-24T00:03:08Z

I'm a little uncomfortable with making CaseItemSyntax have is_optional Pattern.
@nkcsgexi What do you think?

Also, instead of using CaseStmt for catch, how about making a base class (e.g. AbstractMatchStmt) from which CaseStmt and CatchStmt inherit?

jrose-apple · 2019-10-24T00:40:11Z

I'm not sure what benefit we'd get from having CatchStmt and CaseStmt continue to be different classes. They do exactly the same thing, just with different expectations about the incoming type.

owenv · 2019-10-24T01:04:11Z

@jrose-apple There are a few cases where we need to check the CaseParentKind to handle cases and catches differently which might cause some concern. Specifically, the Profiler and Formatting both need to special-case their CaseStmt handling in a few different places. I think some of the complexity can be cleaned up though, so the tradeoffs are probably worth it unless I discover other problems.

@rintaro I agree that the CaseItemSyntax change is not great. I'll look into ways of cleaning that up a bit when I have some more time to work on this. It might be best to not try to reuse the case constructs at the Syntax level.

owenv · 2019-11-06T21:44:12Z

@swift-ci please test

DougGregor · 2020-01-15T17:22:36Z

@swift-ci please smoke test

DougGregor · 2020-01-15T17:22:45Z

@swift-ci please test source compatibility

DougGregor · 2020-01-15T17:22:51Z

@swift-ci build toolchain

owenv · 2020-01-15T17:35:01Z

@DougGregor I guess I forgot to link them on this PR, swiftlang/llvm-project#538 and swiftlang/swift-syntax#190 are also required to build this. Sorry about that!

theblixguy · 2020-01-15T19:17:46Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please build toolchain

theblixguy · 2020-01-15T19:19:32Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test

theblixguy · 2020-01-15T19:20:00Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test source compatibility

owenv · 2020-01-15T19:21:25Z

Thanks @theblixguy !

swift-ci · 2020-01-15T20:19:07Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - b1aa6b8

Install command
tar zxf swift-PR-27776-341-ubuntu16.04.tar.gz
More info

swift-ci · 2020-01-15T20:48:02Z

Build failed
Swift Test OS X Platform
Git Sha - b1aa6b8

owenv · 2020-03-23T21:27:28Z

For reference, the update above picks up changes from recent updates to function builders and the indentation implementation

swift-ci · 2020-03-23T23:22:14Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-23T23:34:08Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test macOS

swift-ci · 2020-03-23T23:41:57Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-24T21:48:02Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test macOS

swift-ci · 2020-03-24T23:44:53Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-24T23:47:00Z

Looks like something in swift-syntax has fallen out of date. I’ll take a closer look tommorrow

owenv · 2020-03-26T19:51:11Z

testing w/ updated swift-syntax PR

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test

swift-ci · 2020-03-26T19:52:51Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-26T19:55:56Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test macOS

owenv · 2020-03-26T20:08:37Z

@rjmccall do you mind helping review the type checker and SILGen changes here as the original author of do-catch?

swift-ci · 2020-03-26T20:17:03Z

Build failed
Swift Test Linux Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-26T20:19:16Z

My lldb patch seems to be out of date again, looking now

swift-ci · 2020-03-26T20:42:03Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-03-27T13:57:07Z

On further investigation, the failures above may have just been infra issues. trying again:

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test

Like switch cases, a catch clause may now include a comma- separated list of patterns. The body will be executed if any one of those patterns is matched. This patch replaces `CatchStmt` with `CaseStmt` as the children of `DoCatchStmt` in the AST. This necessitates a number of changes throughout the compiler, including: - Parser & libsyntax support for the new syntax and AST structure - Typechecking of multi-pattern catches, including those which contain bindings. - SILGen support - Code completion updates - Profiler updates - Name lookup changes

owenv · 2020-04-04T16:29:39Z

fixing profiler merge conflicts

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test

swift-ci · 2020-04-04T16:32:19Z

Build failed
Swift Test Linux Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

swift-ci · 2020-04-04T16:32:31Z

Build failed
Swift Test OS X Platform
Git Sha - f1f15ea5d57af74be4ec145d3d4ae8e99e8bd370

owenv · 2020-04-04T16:59:46Z

macOS failure looks like flakiness:

09:52:38 /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.4.sdk/usr/include/objc/objc-api.h:29:10: fatal error: cannot open file '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.4.sdk/usr/include/AvailabilityMacros.h': Operation not permitted
09:52:38 #include <AvailabilityMacros.h>
09:52:38          ^
09:52:38 1 error generated.

Windows failure is expected since it doesn't support cross-repo PR testing

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test macOS platform

DougGregor

This looks really good! I have one comment where it looks like we have a lot of code duplication in the type checker between switch and do-catch, but we can clean that up in a follow-up PR.

DougGregor · 2020-04-07T17:11:36Z

lib/Sema/TypeCheckStmt.cpp

+
+    auto clauses = S->getCatches();
+    CaseStmt *previousBlock = nullptr;
+    for (auto i = clauses.begin(), e = clauses.end(); i != e; ++i) {


This code looks nearly identical to what's in visitSwitchStmt. Can they be unified?

I think they can be if I refactor fallthrough handling a little. I'll look into that asap

DougGregor · 2020-04-07T18:24:04Z

@swift-ci please test source compatibility

DougGregor · 2020-04-07T18:24:38Z

swiftlang/llvm-project#538
swiftlang/swift-syntax#190

@swift-ci please test source compatibility

slavapestov · 2020-04-07T19:17:09Z

lib/SILGen/SILGenPattern.cpp

+        // Since we know that we only have one case label item, grab its pattern
+        // vars and use that to update expected with the right SILValue.
+        //
+        // TODO: Do we need a copy here?


@gottesmm Any thoughts on this TODO?

DougGregor · 2020-04-07T19:37:52Z

@owenv Also, can you update the ChangeLog in a follow-up PR?

owenv force-pushed the catch_revamp_take_4 branch from 7d0940a to 6a6bc71 Compare October 27, 2019 03:03

owenv marked this pull request as ready for review October 27, 2019 19:49

This was referenced Oct 28, 2019

[requires-evolution] Conditionally compiled catch clauses #27905

Closed

Add Proposal: Multi-Pattern Catch Clauses swiftlang/swift-evolution#1084

Merged

owenv force-pushed the catch_revamp_take_4 branch 2 times, most recently from ad9a6d3 to 29a40d5 Compare November 6, 2019 21:43

owenv added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Nov 6, 2019

owenv requested review from gottesmm and rintaro November 6, 2019 22:02

owenv force-pushed the catch_revamp_take_4 branch 2 times, most recently from 4267fb3 to da8e73c Compare November 12, 2019 21:56

owenv force-pushed the catch_revamp_take_4 branch 2 times, most recently from 18449b8 to b1aa6b8 Compare January 7, 2020 23:20

This was referenced Jan 8, 2020

LLDB updates to support multi-pattern catches (SE-0276) swiftlang/llvm-project#538

Merged

Update gyb generated files for multi-pattern catch clauses (SE-0276) swiftlang/swift-syntax#190

Merged

owenv force-pushed the catch_revamp_take_4 branch from f1f15ea to 43e2d10 Compare April 4, 2020 16:29

owenv mentioned this pull request Apr 4, 2020

[SE-0276] Syntax and Parse updates #30811

Closed

DougGregor approved these changes Apr 7, 2020

View reviewed changes

slavapestov approved these changes Apr 7, 2020

View reviewed changes

DougGregor merged commit 78880ff into swiftlang:master Apr 7, 2020

nathawes mentioned this pull request Apr 8, 2020

[SourceKit/CodeFormat] Column-align multiple patterns in catch clauses #30870

Merged

owenv mentioned this pull request Apr 8, 2020

[Sema] Eliminate duplication in CaseStmt typechecking for switch and do-catch #30879

Merged

[SE-0276] Support multiple patterns in catch clauses #27776

[SE-0276] Support multiple patterns in catch clauses #27776

Uh oh!

Conversation

owenv commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owenv commented Oct 21, 2019

Uh oh!

rintaro commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owenv commented Oct 24, 2019

Uh oh!

owenv commented Nov 6, 2019

Uh oh!

DougGregor commented Jan 15, 2020

Uh oh!

DougGregor commented Jan 15, 2020

Uh oh!

DougGregor commented Jan 15, 2020

Uh oh!

owenv commented Jan 15, 2020

Uh oh!

theblixguy commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theblixguy commented Jan 15, 2020

Uh oh!

theblixguy commented Jan 15, 2020

Uh oh!

owenv commented Jan 15, 2020

Uh oh!

swift-ci commented Jan 15, 2020

Uh oh!

swift-ci commented Jan 15, 2020

Uh oh!

owenv commented Mar 23, 2020

Uh oh!

swift-ci commented Mar 23, 2020

Uh oh!

owenv commented Mar 23, 2020

Uh oh!

swift-ci commented Mar 23, 2020

Uh oh!

owenv commented Mar 24, 2020

Uh oh!

swift-ci commented Mar 24, 2020

Uh oh!

owenv commented Mar 24, 2020

Uh oh!

owenv commented Mar 26, 2020

Uh oh!

swift-ci commented Mar 26, 2020

Uh oh!

owenv commented Mar 26, 2020

Uh oh!

owenv commented Mar 26, 2020

Uh oh!

swift-ci commented Mar 26, 2020

Uh oh!

owenv commented Mar 26, 2020

Uh oh!

swift-ci commented Mar 26, 2020

Uh oh!

owenv commented Mar 27, 2020

Uh oh!

owenv commented Apr 4, 2020

Uh oh!

swift-ci commented Apr 4, 2020

Uh oh!

swift-ci commented Apr 4, 2020

Uh oh!

owenv commented Apr 4, 2020

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

owenv commented Oct 18, 2019 •

edited

Loading

rintaro commented Oct 24, 2019 •

edited

Loading

jrose-apple commented Oct 24, 2019 •

edited

Loading

theblixguy commented Jan 15, 2020 •

edited

Loading