[AST] [Sema] [IDE] Fix places where we set bare arguments #17987
Conversation
|
@swift-ci Please test |
beccadax
changed the title
|
I'm a bit surprised by the lack of @swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test |
|
Build failed |
|
That one failure looks like it was from the then-current master; rebasing and trying again. @swift-ci Please test |
|
Build failed |
|
Build failed |
|
🤷♂️ @swift-ci Please test |
beccadax
changed the title
|
@CodaFi Yes, we seem to have implemented similar fixes. Looks like the difference is that I effectively If #17696 gets merged, I can back out that particular change, and I think the rest of this will slot right in. |
|
This looks like a great idea. Eventually we want ApplyExpr to directly store call arguments and encode which ones are packed into variadics and defaulted, etc. Normalizing the AST in this manner is a good first step and also makes sense on its own. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
🙄 @swift-ci please test |
Most ApplyExpr subclasses will now require a ParenExpr, TupleExpr, or TupleShuffleExpr around their argument lists. The exceptions are BinaryExpr, which requires a TupleExpr, and SelfApplyExpr and its subclasses, which allow anything (and only ever have one argument). This change doesn’t fix any of the places where we actually generate these.
PreCheckExpression was not treating the implicit call parens on unary or binary operator applications like the explicit parens on CallExpr applications. Now it will. This prevents various forms of mischief, like having them subsumed into a TypeExpr.
|
@swift-ci please smoke test |
| else if (isa<BinaryExpr>(this)) | ||
| return isa<TupleExpr>(e); | ||
| else | ||
| return isa<ParenExpr>(e) || isa<TupleExpr>(e) || isa<TupleShuffleExpr>(e); |
There was a problem hiding this comment.
Is it worth to add PrefixUnaryExpr/PostfixUnaryExpr -> ParenExpr check here? cc: @xedin
There was a problem hiding this comment.
Since arguments of such expressions are now implicitly wrapped into parens it shouldn't be required.
There was a problem hiding this comment.
That's why I thought we can add the check for it.
There was a problem hiding this comment.
I don’t think we should. To be honest, I’d like to drop the BinaryExpr -> TupleExpr check too, because I want to support operator functions with extra defaulted parameters (for #line and friends), which will require TupleShuffleExpr args on BinaryExpr. But that will require other changes to the compiler, and I’d rather do one thing at a time.
|
@swift-ci please smoke test and merge |
|
It seems bot didn't responded. |
This change adds an assertion that, except on
SelfApplyExprs,Argmust always be aParenExpr,TupleExpr,TupleShuffleExpr,CodeCompletionExpr, orErrorExprAlong the way, I may have fixed rdar://41416911, but I'll need someone at Apple to confirm that for me.
I'm not sure if we should keep the assertions in the compiler or revert them and just merge the fixes, but I'm not at that stage yet anyway.
@CodaFi @slavapestov Probably relevant to your interests.