Skip to content
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

Include try and await expressions which are parents of a freestanding expression macro in lexicalContext #2724

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stmontgomery
Copy link

This modifies the behavior of MacroExpansionContext.lexicalContext such that any preceding try or await expressions are included.

Motivation: The #expect macro in Swift Testing currently cannot propagate try or await placed before the freestanding expression macro to expressions in its expanded code. See Forum discussion. This change would allow us to fix that.

Resolves rdar://109470248

… expression macro in lexicalContext

Resolves rdar://109470248
@stmontgomery stmontgomery added enhancement New feature or request Macros Issues in the SwiftSyntaxMacro… modules labels Jul 10, 2024
@stmontgomery stmontgomery self-assigned this Jul 10, 2024
// with a trivial placeholder, though.
case var tryExpr as TryExprSyntax:
tryExpr = tryExpr.detached
tryExpr.expression = ExprSyntax(TypeExprSyntax(type: IdentifierTypeSyntax(name: .wildcardToken())))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wildcard token was chosen arbitrarily here. We don't necessarily have to reassign the expression at all, but I figured it shouldn't be necessary since the macro implementation already has it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably we do need to reassign it, otherwise in an expression like this one:

await f(a, b, c, #d)

d is able to see a, b, and c.

@xwu
Copy link
Contributor

xwu commented Jul 10, 2024

Just spitballing—could there be some way to alter the macro expansion itself so as to be insensitive to any try or await that precedes it? For example, would wrapping the current expansion in an immediately executed closure allow for this to work:

/* Would `try` need to be hoisted here too (?) */ { Testing.__checkValue(try f(), expression: .__fromSyntaxNode("try f()"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected() }()

One imagines that there could exist other syntax which impact the 'hygiene' of the #expect macro that could benefit from a more clever/defensive expansion.

@grynspan
Copy link
Contributor

Just spitballing—could there be some way to alter the macro expansion itself so as to be insensitive to any try or await that precedes it? For example, would wrapping the current expansion in an immediately executed closure allow for this to work:

/* Would `try` need to be hoisted here too (?) */ { Testing.__checkValue(try f(), expression: .__fromSyntaxNode("try f()"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected() }()

One imagines that there could exist other syntax which impact the 'hygiene' of the #expect macro that could benefit from a more clever/defensive expansion.

No, that wouldn't work unfortunately. All that does is add another spot where we might need try or await, because the call to Testing.__checkValue() would still need them.

@xwu
Copy link
Contributor

xwu commented Jul 10, 2024

No, that wouldn't work unfortunately. All that does is add another spot where we might need try or await

I wasn't under the impression that the specific text I sketched would work (in fact, I was 100% certain it wouldn't)—rather, that was a freehand sketch to try to illustrate a direction that I'm wondering whether you all have explored. The crux of the question is whether it's been concluded that there exists no solution within the confines of what macros support today that's possible with more finagling of the specific macro expansion emitted.

Indeed, as you say, the approach I have in mind is about ways to deliberately add extra trys—this would (potentially—or perhaps I am naive to think it) allow a syntactically valid expansion that unconditionally emits a try without regard to whether there is an outer one—although obviously this could have other consequences that are undesirable that I haven't worked through. Specifically, consider the following:

// This wrapper function allows one to write an extraneous `try` because it unconditionally throws.
func _wrapper<T>(_ f: @autoclosure () throws -> T) throws -> T { try f() }

func foo() throws -> Int { 42 }
func bar() -> Int { 42 }

try bar() // Warning: no calls to throwing functions...

// Look! With `_wrapper`, I can (and must) write an extra `try` and it's fine, without checking whether `bar` throws.
try _wrapper(bar())

// And it's fine to have as many outer `try`s as I want.
try { try _wrapper(bar()) }()
try { try { try _wrapper(bar()) }()

I know the idea is half-baked. However, with more elbow grease, is it fully bakeable or are we truly stuck...

(Part of the reason I ask is, while swift-syntax API changes are outside the scope of Swift Evolution, those changes that add/change what macros are capable of doing are considered to be language features that need review.)

@grynspan
Copy link
Contributor

grynspan commented Jul 11, 2024

The problem is ultimately that we can only see the syntax nodes inside the macro. Unconditionally adding a try or await ahead of the expression would change the semantics of the expression and require the calling context to be throwing and/or async.

You'll note we do unconditionally add try and await when we initialize suite types, because in the context where we do so, we're already async throws so it doesn't matter.

@xwu
Copy link
Contributor

xwu commented Jul 11, 2024

Yeah, I see your point.

Going in the other direction, how confident can we be that await and try are the only or even most of the cases now that have this contextual issue for freestanding macros?

Should we instead consider some sort of "whole-statement freestanding macro" that always provides the entire statement as read-only context, such that the parens don't delimit what's visible to the macro but only marks the span that will be expanded?

@grynspan
Copy link
Contributor

grynspan commented Jul 11, 2024

Mutating member function calls on value types also don't compile correctly and they are syntactically identical to non-mutating ones. There is no readily available solution, but those are relatively rare so it's not as big of a problem.

New kinds of macros are beyond the purview of Swift Testing—you may want to bring your idea to the forums to discuss with other stakeholders. 🙂

@grynspan
Copy link
Contributor

@swift-ci please test

Copy link
Member

@hborla hborla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to have a design discussion about whether try and await really belong in the lexical context - probably on the forums instead of here. There are also real problems caused by the fact that the parser cannot represent the scope covered by try and await because it's only known after type checking because of things like operator precedence and sequence folding. I recently had to add a heuristic to the C++ parser implementation because of this, and I'm not sure how the Swift parser implementation handles it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Macros Issues in the SwiftSyntaxMacro… modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants