Introduce try/await/unsafe macro lexical contexts with unfolded sequence handling
#3037
Conversation
… expression macro in lexicalContext Resolves rdar://109470248
We can say that any `try`/`await` element also covers all elements to the right of it in an unfolded sequence. Cases where this isn't true will be rejected by the compiler, e.g: ``` 0 * try foo() + bar() _ = try foo() ~~~ bar() // Assuming `~~~` has lower precedence than `=` ``` rdar://109470248
hamishknight
requested review from
DougGregor,
ahoppen and
bnbarham
as code owners
|
@swift-ci please test |
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I requested that a while back, actually. :)
| // 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 |
There was a problem hiding this comment.
I would like us to include unsafe expressions in the lexical context as well.
There was a problem hiding this comment.
Updated to include unsafe
Match the behavior of `try` and `await`.
|
@swift-ci please test |
|
@swift-ci please test Windows |
hamishknight
changed the title
try and await macro lexical contexts with unfolded sequence handlingtry/await/unsafe macro lexical contexts with unfolded sequence handling
| // `sequenceExprContexts` is built from the top-down, but we | ||
| // build the rest of the contexts bottom-up. Reverse for | ||
| // consistency. | ||
| parentContexts += sequenceExprContexts.reversed() |
There was a problem hiding this comment.
Wouldn’t it make more sense to move this after the for loop? Just to make sure that we add elements to parentContext in case we should terminate the loop for some other reason than the break below.
There was a problem hiding this comment.
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.
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 for loop and only having a break in here because the addition of elements to parentContexts conceptually happens after iterating over the elements.
There was a problem hiding this comment.
Ah okay, I don't really feel all that strongly about it, happy to change
…wait in recorded issues (#1092) This fixes an issue where code comments placed before an expectation like `#expect()` which has effect introducer keywords like `try` or `await` are ignored, and ensures they are included in the recorded issue if the expectation fails. Consider this example of two failing expectations: ```swift // Uh oh! #expect(try x() == 2) // Uh oh! try #expect(x() == 2) ``` Prior to this PR, if `x()` returned a value other than `2`, there would be two issues recorded, but the second one would not have the comment `“Uh oh!”` because from the macro’s perspective, that code comment was on the `try` keyword and it could only see trivia associated with `#expect()`. Now, with the recent swift-syntax fix from swiftlang/swift-syntax#3037, the `try` keyword and its associated trivia can be included and this bug can be fixed. We recently adopted a new-enough swift-syntax in #1069, so the only fix needed is to adopt `lexicalContext` for this new purpose in our macro. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
This is #2724 with additional logic for handling
try/await/unsafein unfolded sequence expressions. We can say that anytry/await/unsafeelement also covers all elements to the right of it (matching the logic in swiftlang/swift#80461). Cases where this isn't true will be rejected by the compiler, e.g:rdar://109470248