Implement SE-0095: Replace protocol<P1, P2> syntax with P1 & P2 #3293

joewillsher · 2016-07-01T15:54:14Z

What's in this pull request?

Implementation for SE-0095: Replace protocol<P1, P2> syntax with P1 & P2.

This PR implements:

Parsing of the new syntax (in parseTypeIdentifierOrTypeComposition)
A warning for the old syntax which will soon be changed to an error, plus a fixit to move to new syntax
Any becomes a keyword and is removed from the standard library
Changed the demangler & AST printer to use the new syntax
IRGen mangles compositions slightly differently
PreCheckExpression supports parsing compositions
Code completion for the Any keyword

Resolved bug number: (SR-1938)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

CodaFi · 2016-07-01T17:45:44Z

Looks like you have conflicts. Could you rebase onto master and fix them please?

JaSpa · 2016-07-03T22:59:49Z

lib/Parse/ParseExpr.cpp

@@ -1744,8 +1744,7 @@ DeclName Parser::parseUnqualifiedDeclName(bool afterDot,
  // Consume the base name.
  Identifier baseName = Context.getIdentifier(Tok.getText());
  SourceLoc baseNameLoc;
-  if (Tok.is(tok::identifier) || Tok.is(tok::kw_Self) ||
-      Tok.is(tok::kw_self)) {
+  if (Tok.isAny(tok::identifier, tok::kw_Self, tok::kw_Self)) {


I guess you want the S on the second kw_Self lowercase

Thanks, fixed

joewillsher · 2016-07-04T16:37:43Z

Added PR to swift-corelibs-foundation to accompany this one.

joewillsher · 2016-07-11T18:14:33Z

@lattner Could you (or someone else) please review the code in this PR when you get a chance? I think it is ready to merge and I have some local work for another problem I’d like to submit (but can’t until this is merged), thanks.

jrose-apple · 2016-07-13T17:34:55Z

include/swift/AST/KnownIdentifiers.def

@@ -60,6 +60,7 @@ IDENTIFIER(RawValue)
 IDENTIFIER(Selector)
 IDENTIFIER(self)
 IDENTIFIER(Self)
+IDENTIFIER(Any)


Nitpick: Please keep this list alphabetized.

joewillsher · 2016-07-14T01:18:40Z

@jrose-apple Changes made, I’ve done everything apart from verifying that (P1 & P2).self will always be resolved into a OverloadedDeclRefExpr.

How does it look, and thanks for your help so far!

joewillsher · 2016-07-16T15:23:49Z

@jrose-apple And that is done too, we no longer look at the result of the OverloadedDeclRefExpr in simplifyTypeExpr.

CodaFi · 2016-07-16T17:25:40Z

lib/Parse/ParseType.cpp

@@ -58,11 +59,14 @@ ParserResult<TypeRepr> Parser::parseTypeSimple(Diag<> MessageID,

  switch (Tok.getKind()) {
  case tok::kw_Self:
+      ty = parseTypeIdentifier();
+      break;


Nitpick: whitespace

CodaFi · 2016-07-16T17:29:03Z

Jordan is away on vacation for the next few days. In the meantime I can pick up some of his end on code review.

In general the first thing I've noticed this patch overruns our 80-column limit in a few places and introduces extraneous whitespace into the diff. Could you wrap things and tidy up please?

CodaFi · 2016-07-16T18:25:29Z

Second, I believe you've been git pulling without rebasing, so could you squash the commits that mention merging upstream and even fixup the old irrelevant/duplicate commits. Ideally this patch should introduce 3-5 commits at most to the repo.

DougGregor · 2016-07-17T04:39:02Z

include/swift/AST/DiagnosticsParse.def

@@ -654,6 +654,11 @@ ERROR(expected_rangle_protocol,PointsToFirstBadToken,
 ERROR(disallowed_protocol_composition,PointsToFirstBadToken,
      "protocol composition is neither allowed nor needed here", ())

+WARNING(deprecated_protocol_composition,PointsToFirstBadToken,


PointsToFirstBadToken isn't needed here; just use 'none' for all of these.

joewillsher · 2016-07-18T16:25:13Z

@jckarter Looks like it came from the renaming of ErrorProtocol to Error, should I fix in this PR or do it separately?

jckarter · 2016-07-18T16:29:31Z

Ah, I didn't realize this wasn't merged yet. Sorry.

joewillsher · 2016-07-18T16:34:53Z

No problem — would you mind triggering the CI bot?

DougGregor · 2016-07-18T16:50:47Z

@swift-ci please test and merge

DougGregor · 2016-07-18T18:49:03Z

@swift-ci please test and merge

This commit defines the ‘Any’ keyword, implements parsing for composing types with an infix ‘&’, and provides a fixit to convert ‘protocol<>’ - Updated tests & stdlib for new composition syntax - Provide errors when compositions used in inheritance. Any is treated as a contextual keyword. The name ‘Any’ is used emit the empty composition type. We have to stop user declaring top level types spelled ‘Any’ too.

Also adds: - Any is caught before doing an unconstrained lookup, and the protocol<> type is emitted - composition expressions can be handled by `PreCheckExpression::simplifyTypeExpr` to so you can do lookups like (P & Q).self - Fixits corrected & new tests added - Typeref lowering cases should have been optional - This fixes a failing test case.

…ition syntax - All parts of the compiler now use ‘P1 & P2’ syntax - The demangler and AST printer wrap the composition in parens if it is in a metatype lookup - IRGen mangles compositions differently - “protocol<>” is now “swift.Any” - “protocol<_TP1P,_TP1Q>” is now “_TP1P&_TP1Q” - Tests cases are updated and added to test the new syntax and mangling

Now that Any isn’t in the stdlib we need to add it to code completion separately.

- Any is made into a keyword which is always resolved into a TypeExpr, allowing the removal of the type system code to find TheAnyType before an unconstrained lookup. - Types called `Any` can be declared, they are looked up as any other identifier is - Renaming/redefining behaviour of source loc methods on ProtocolCompositionTypeRepr. Added a createEmptyComposition static method too. - Code highlighting treats Any as a type - simplifyTypeExpr also does not rely on source to get operator name. - Any is now handled properly in canParseType() which was causing generic param lists containing ‘Any’ to fail - The import objc id as Any work has been relying on getting a decl for the Any type. I fix up the clang importer to use Context.TheAnyType (instead of getAnyDecl()->getDeclaredType()). When importing the id typedef, we create a typealias to Any and declare it unavaliable.

DougGregor · 2016-07-19T07:11:54Z

@swift-ci please smoke test and merge

DougGregor · 2016-07-19T15:55:48Z

ARGH! Should have forced a merge before it conflicted again.

joewillsher · 2016-07-19T16:00:30Z

I've fixed a few of the failing cases, I'll resolve conflicts and push when I can in a bit.

DougGregor · 2016-07-19T16:18:14Z

Okay!

DougGregor · 2016-07-19T19:02:33Z

FYI, I did a merge + fix-up and just pushed the result

DougGregor · 2016-07-19T19:03:17Z

0eaa576

joewillsher · 2016-07-19T19:04:44Z

Oh fantastic, thanks for that!

DougGregor · 2016-07-19T19:05:55Z

@joewillsher, I'm going to close out this PR because all of the commits from it have been integrated in master. THANK YOU!

joewillsher · 2016-07-19T19:43:32Z

@DougGregor One last thing: we should merge this corelibs-foundation PR too.

DougGregor · 2016-07-19T20:13:13Z

Okay, done. Thanks!

jrose-apple · 2016-07-21T15:47:45Z

…sorry for not handing this off explicitly before going on vacation! Thanks for all the work, Joe.

JaSpa reviewed Jul 3, 2016
View reviewed changes

joewillsher mentioned this pull request Jul 4, 2016

Update protocol compositions to SE-0095 syntax swiftlang/swift-corelibs-foundation#432

Merged

jrose-apple changed the title ~~Implement SE-0095~~ Implement SE-0095: Replace protocol<P1, P2> syntax with P1 & P2 Jul 5, 2016

joewillsher force-pushed the master branch from ff6d7c5 to f1649fd Compare July 6, 2016 17:38

jrose-apple reviewed Jul 13, 2016
View reviewed changes

joewillsher force-pushed the master branch from f0c109d to c857da4 Compare July 15, 2016 13:23

CodaFi reviewed Jul 16, 2016
View reviewed changes

DougGregor reviewed Jul 17, 2016
View reviewed changes

joewillsher force-pushed the master branch from f8f38c3 to 87c73db Compare July 18, 2016 18:36

joewillsher force-pushed the master branch 2 times, most recently from 57c8fae to 2050200 Compare July 18, 2016 20:48

joewillsher added 6 commits July 18, 2016 23:38

[SE-0095] Converted the docs to new composition syntax

38312c9

[SE-0095] [Code completion] for the Any type keyword

4f079b4

Now that Any isn’t in the stdlib we need to add it to code completion separately.

joewillsher force-pushed the master branch from 2050200 to a775dae Compare July 18, 2016 22:39

joewillsher closed this Jul 18, 2016

joewillsher reopened this Jul 18, 2016

DougGregor closed this Jul 19, 2016

swift-ci mentioned this pull request Jul 1, 2016

[SR-1938] Implement support for SE-0095: Replace protocol<P1, P2> syntax with P1&P2 #44547

Closed

Implement SE-0095: Replace protocol<P1, P2> syntax with P1 & P2 #3293

Implement SE-0095: Replace protocol<P1, P2> syntax with P1 & P2 #3293

Uh oh!

Conversation

joewillsher commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Resolved bug number: (SR-1938)

Uh oh!

CodaFi commented Jul 1, 2016

Uh oh!

JaSpa Jul 3, 2016

Choose a reason for hiding this comment

Uh oh!

joewillsher Jul 4, 2016 • 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!

joewillsher commented Jul 4, 2016

Uh oh!

joewillsher commented Jul 11, 2016

Uh oh!

jrose-apple Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

joewillsher commented Jul 14, 2016

Uh oh!

joewillsher commented Jul 16, 2016

Uh oh!

CodaFi Jul 16, 2016

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 16, 2016

Uh oh!

CodaFi commented Jul 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor Jul 17, 2016

Choose a reason for hiding this comment

Uh oh!

joewillsher commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Jul 18, 2016

Uh oh!

joewillsher commented Jul 18, 2016

Uh oh!

DougGregor commented Jul 18, 2016

Uh oh!

DougGregor commented Jul 18, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

joewillsher commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

joewillsher commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

joewillsher commented Jul 19, 2016

Uh oh!

DougGregor commented Jul 19, 2016

Uh oh!

jrose-apple commented Jul 21, 2016

Uh oh!

Uh oh!

joewillsher commented Jul 1, 2016 •

edited

Loading

joewillsher Jul 4, 2016 •

edited

Loading

CodaFi commented Jul 16, 2016 •

edited

Loading

joewillsher commented Jul 18, 2016 •

edited

Loading