[libSyntax] Make parsing of attributes more structured #16117

ahoppen · 2018-04-24T00:08:57Z

Add significantly more structure to the way the attributes are parsed in libSyntax by no longer parsing the arguments as a TokenList. This way we can also parse @_specialize attributes correctly which can have a GenericWhereClause as one of its arguments.

ahoppen · 2018-04-24T00:09:06Z

@swift-ci Please smoke test

ahoppen · 2018-04-24T01:35:21Z

@swift-ci Please smoke test

rintaro · 2018-04-24T07:49:25Z

utils/gyb_syntax_support/AttributeNodes.py

+                   If the attribute takes arguments, the closing parenthesis.
+                   '''),
+             # TokenList to gather remaining tokens of invalid attributes
+             Child('TokenList', kind='TokenList', is_optional=True),


Is it possible to make TokenList be a choice of Child('Argument') ?
I mean, invalid argument could be constructed as '@' identifier ('(' token-list ')')?.

I would actually like to get rid of TokenList completely. It is only used here and for closure capture items. Maybe I should add a FIXME instead.

rintaro · 2018-04-24T07:50:04Z

utils/gyb_syntax_support/AttributeNodes.py

+                   node_choices=[
+                       Child('Identifier', kind='IdentifierToken'),
+                       Child('String', kind='StringLiteralToken'),
+                       Child('Integer', kind='IntegerLiteralToken'),


Which attribute uses IntegerLiteralToken as the argument?

@_alignment

Thanks! Could you add a test case?

rintaro · 2018-04-24T08:23:04Z

lib/Parse/SyntaxParsingContext.cpp

@@ -189,6 +190,8 @@ void SyntaxParsingContext::createNodeInPlace(SyntaxKind Kind) {
  case SyntaxKind::MemberTypeIdentifier:
  case SyntaxKind::FunctionCallExpr:
  case SyntaxKind::SubscriptExpr:
+  case SyntaxKind::AvailabilitySpecList:
+  case SyntaxKind::AvailabilityArgument:


I don't think these are used. Am I missing something?

AvailabilityArgument are the children of AvailabilitySpecList and AvailabilitySpecList is one of the options for Attribute.Arguments or AvailabilityCondition.

Ah, sorry. I meant createNodeInPlace(SyntaxKind::AvailabilitySpecList) is not used, nor AvailabilityArgument.
This switch is not supposed to be exhaustive.

Ah, OK. I added them while trying to fix it and will remove them again.

rintaro · 2018-04-24T08:44:13Z

lib/Parse/ParseDecl.cpp

-  StringRef AttrName = "_specialize";
+AvailableAttr *Parser::parseExtendedAvailabilitySpecList(
+    SourceLoc AtLoc, SourceLoc AttrStartLoc, StringRef Platform,
+    StringRef AttrName, SourceLoc AttrLoc, bool &DiscardAttribute) {


This function returns the attribute itself, naming parseExtendedAvailabilityAttribute might be more relevant IMO.

Return ParserResult<AvailableAttr> without bool &DiscardAttribute like parseImplementsAttribute.

AttrLoc and AttrStartLoc is the same. Please consolidate them.

Platform can be retrieved with Tok.getText() inside the function.

The motivation for factoring out this function was that it could create a SyntaxParsingContext with kind AvailabilitySpecList for its entire scope so we don't have to create a code block that for that context that spans nearly 200 lines of code. That's why it also doesn't parse the trailing ).
I will implement 2, 3, and 4. Thanks.

Have you tried collectNodesInPlace(SyntaxKind::AvailabilitySpecList) without creating SyntaxParsingContext in parseExtendedAvailabilitySpecList?

Actually, AtLoc andAttrLoc are different, AtLoc refers to where the @ is, AttrLoc where the name of the attribute starts.

Yes, that doesn't work because the opening ( is still on the stack of items to be parsed and doesn't fit into the AvailabilitySpecList

AtLoc != AttrLoc but AttrStartLoc == AttrLoc, right?

Yes. Sorry, misread your comment. You're right of course

Yes, that doesn't work because the opening ( is still on the stack of items

It should work. For example, in parseDeclEnumCase(), collectNodesInPlace(SyntaxKind::EnumCaseElementList) turns
{ 'case', element, element, element ...} into { 'case', element-list }.

OK, it does seem to work. I'm pretty sure I tried before and it didn't but maybe I'm mixing things up. It's not needed though since I keep the parsing logic for ) outside of the refactored function.

rintaro · 2018-04-24T08:59:47Z

lib/Parse/ParseDecl.cpp

-
-    AttrRange = SourceRange(Loc, Tok.getLoc());
+    auto AvailabilityAttr = parseExtendedAvailabilitySpecList(
+        AtLoc, Loc, Platform, AttrName, Loc, DiscardAttribute);

    if (!consumeIf(tok::r_paren)) {


IMO, ) should be parsed in the refactored function.
I guess you leave it here because ( is parsed in this function, but still... What do you think?

Combining comments above, the call-site can be something like:

ParserResult<AvailableAttr> Attr = parseExtendedAvailabilityAttribute(AtLoc, Loc, AttrName); if (Attr.isNonNull()) { Attributes.add(Attr.get()); } break;

Thinking about it, I would leave it as it is. The parsing of @available attributes is slightly weird because there are the two variants @available(*, …) and @available(iOS 10, *) that are handled inside the switch. I don't want to parse ) inside the refactored function and ( outside and everything else would require ripping apart the entire parsing logic and I don't think it's worth it.

ahoppen · 2018-04-24T16:14:51Z

@swift-ci Please smoke test

This also fixes several issues where attribute arguments could not be parsed as a TokenList since some of its arguments already had structure and were not tokens

ahoppen · 2018-04-24T20:18:32Z

@swift-ci Please smoke test

ahoppen · 2018-04-24T21:02:57Z

@rintaro Unless you have any further comments, I'll be merging this tonight.

rintaro

Looks good!

ahoppen requested review from rintaro and nkcsgexi April 24, 2018 00:08

rintaro self-assigned this Apr 24, 2018

ahoppen force-pushed the attributes branch from 444f7d3 to 5a34620 Compare April 24, 2018 01:35

rintaro reviewed Apr 24, 2018

View reviewed changes

ahoppen force-pushed the attributes branch 2 times, most recently from a5ac617 to 7a49737 Compare April 24, 2018 16:14

[libSyntax] Make parsing of attribute arguments more structured

3e9ae80

This also fixes several issues where attribute arguments could not be parsed as a TokenList since some of its arguments already had structure and were not tokens

ahoppen force-pushed the attributes branch from 7a49737 to 3e9ae80 Compare April 24, 2018 20:18

nkcsgexi approved these changes Apr 24, 2018

View reviewed changes

rintaro approved these changes Apr 24, 2018

View reviewed changes

rintaro removed their assignment Apr 25, 2018

ahoppen merged commit a5314af into swiftlang:master Apr 25, 2018

ahoppen deleted the attributes branch April 26, 2018 14:50

[libSyntax] Make parsing of attributes more structured #16117

[libSyntax] Make parsing of attributes more structured #16117

Uh oh!

Conversation

ahoppen commented Apr 24, 2018

Uh oh!

ahoppen commented Apr 24, 2018

Uh oh!

ahoppen commented Apr 24, 2018

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!

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!

rintaro Apr 24, 2018 • 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!

rintaro Apr 24, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

rintaro Apr 24, 2018 • 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!

ahoppen commented Apr 24, 2018

Uh oh!

ahoppen commented Apr 24, 2018

Uh oh!

ahoppen commented Apr 24, 2018

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro Apr 24, 2018 •

edited

Loading

rintaro Apr 24, 2018 •

edited

Loading

rintaro Apr 24, 2018 •

edited

Loading