-
Notifications
You must be signed in to change notification settings - Fork 465
Introduce try/await/unsafe macro lexical contexts with unfolded sequence handling
#3037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,20 @@ extension SyntaxProtocol { | |
| case let freestandingMacro as FreestandingMacroExpansionSyntax: | ||
| return Syntax(freestandingMacro.detached) as Syntax | ||
|
|
||
| // Try and await are preserved: A freestanding expression macro preceded | ||
| // by try or await may need to know whether those keywords are present so it | ||
| // can propagate them to any expressions in its expansion which were passed | ||
| // as arguments to the macro. The expression of the try or await is replaced | ||
| // with a trivial placeholder, though. | ||
| case var tryExpr as TryExprSyntax: | ||
| tryExpr = tryExpr.detached | ||
| tryExpr.expression = ExprSyntax(TypeExprSyntax(type: IdentifierTypeSyntax(name: .wildcardToken()))) | ||
| return Syntax(tryExpr) | ||
| case var awaitExpr as AwaitExprSyntax: | ||
| awaitExpr = awaitExpr.detached | ||
| awaitExpr.expression = ExprSyntax(TypeExprSyntax(type: IdentifierTypeSyntax(name: .wildcardToken()))) | ||
| return Syntax(awaitExpr) | ||
|
|
||
| default: | ||
| return nil | ||
| } | ||
|
|
@@ -92,6 +106,39 @@ extension SyntaxProtocol { | |
| if let parentContext = parentNode.asMacroLexicalContext() { | ||
| parentContexts.append(parentContext) | ||
| } | ||
| if let sequence = parentNode.as(SequenceExprSyntax.self) { | ||
| var sequenceExprContexts: [Syntax] = [] | ||
| for elt in sequence.elements { | ||
| if elt.range.contains(self.position) { | ||
| // `sequenceExprContexts` is built from the top-down, but we | ||
| // build the rest of the contexts bottom-up. Reverse for | ||
| // consistency. | ||
| parentContexts += sequenceExprContexts.reversed() | ||
| break | ||
| } | ||
| var elt = elt | ||
| while true { | ||
| if let tryElt = elt.as(TryExprSyntax.self) { | ||
| sequenceExprContexts.append(tryElt.asMacroLexicalContext()!) | ||
| elt = tryElt.expression | ||
| continue | ||
| } | ||
| if let awaitElt = elt.as(AwaitExprSyntax.self) { | ||
| sequenceExprContexts.append(awaitElt.asMacroLexicalContext()!) | ||
| elt = awaitElt.expression | ||
| continue | ||
| } | ||
| if let unsafeElt = elt.as(UnsafeExprSyntax.self) { | ||
|
Comment on lines
+131
to
+141
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR, but this makes me think we need a protocol for the "effect-like" expression nodes. I feel like this pattern is going to come up a bit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #3040
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I requested that a while back, actually. :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| // No scope for this currently, but we need to look through it | ||
| // since it's similar to 'try' in that it's hoisted above a | ||
| // binary operator when appearing on the LHS. | ||
| elt = unsafeElt.expression | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like us to include
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to include |
||
| continue | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| currentNode = parentNode | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t it make more sense to move this after the
forloop? Just to make sure that we add elements toparentContextin case we should terminate the loop for some other reason than thebreakbelow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there cases where we might not be able to determine which element the node is within? The main goal here is to only add the effects for nodes that occur after them in the sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just thought that it would read nicer to have
// `sequenceExprContexts` is built from the top-down, but we // build the rest of the contexts bottom-up. Reverse for // consistency. parentContexts += sequenceExprContexts.reversed()after the
forloop and only having abreakin here because the addition of elements toparentContextsconceptually happens after iterating over the elements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I don't really feel all that strongly about it, happy to change