[Sema] Maintain the implicitness of call argument tuple/parens in coerceCallArguments #31184
Conversation
|
@swift-ci please test |
lib/AST/Expr.cpp
Outdated
| @@ -1231,6 +1231,9 @@ SourceRange TupleExpr::getSourceRange() const { | |||
| } else { | |||
| // Scan forward for the first valid source loc. | |||
| for (Expr *expr : getElements()) { | |||
| // Skip default arguments - they inherit the location of their parent. | |||
| if (isa<DefaultArgumentExpr>(expr)) | |||
There was a problem hiding this comment.
I see that in coerceCallArguments when we create DefaultArgumentExpr we give it arg->getStartLoc() which is supposed to be a location of "argument list". I think DefaultArgumentExpr should implement getStartLoc/getEndLoc and return {} from it to indicate that it has to source location. This way we wouldn't need to touch TupleExpr, WDYT?
There was a problem hiding this comment.
I'm not even sure why would it implement getSourceRange with a single location, maybe we should just remove Loc from DefaultArgumentExpr?
There was a problem hiding this comment.
Yeah, makes sense – thanks!
lib/Sema/CSApply.cpp
Outdated
| arg = new (ctx) | ||
| ParenExpr(lParenLoc, newArgs[0], rParenLoc, hasTrailingClosure); | ||
| arg->setImplicit(); | ||
| if (isImplicit) | ||
| arg->setImplicit(); |
There was a problem hiding this comment.
FYI Expr::setImplicit accepts an argument so I believe arg->setImplicit(isImplicit); should work.
nathawes
force-pushed
the
defaulted-arg-list-implicitness
branch
from
29cf3d8 to
3a5460a
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
…rceCallArguments If any arguments were defaulted the tuple/paren was made implicit, even though the original tuple/paren was present in the source. This prevented some sourcekit ASTWalkers from considering them, making refactorings, documentation info, jump-to-definition and other features unavailable when queried via their argument labels. Resolves rdar://problem/62118957
nathawes
force-pushed
the
defaulted-arg-list-implicitness
branch
from
3a5460a to
7c0a178
Compare
|
@swift-ci please test |
|
Build failed |
| @@ -788,7 +788,8 @@ Expr *CallerSideDefaultArgExprRequest::evaluate( | |||
| auto paramTy = defaultExpr->getType(); | |||
|
|
|||
| // Re-create the default argument using the location info of the call site. | |||
| auto *initExpr = synthesizeCallerSideDefault(param, defaultExpr->getLoc()); | |||
| auto *initExpr = | |||
| synthesizeCallerSideDefault(param, defaultExpr->getArgumentListLoc()); | |||
There was a problem hiding this comment.
@xedin this is the bit I was missing that caused the test failures previously. getLoc() was based on getSourceRange().
|
Build failed |
This restores getSourceRange() on DefaultArgumentExpr after it was removed in swiftlang#31184. It was originally removed to solve the issues it was causing when computing the source range of its parent TupleExpr. To account for trailing closures we walk back through the tuple's arguments until one with a valid location is found, which we use as the end location. If the last argument was a DefaultArgumentExpr though that meant the end loc would end up being the tuple's start location, so none of the tuple's other arguments were contained in its range, triggering an ASTVerifier assertion. Source tooling and diagnostics don't care about default arg expression locations as nothing can reference them, but their locations are output in the debug info. Added a regression test to catch that in future, and updated TupleExpr::getSourceRange() to ignore them when computing the end loc. Resolves rdar://problem/63195504.
This restores getSourceRange() on DefaultArgumentExpr after it was removed in swiftlang#31184. It was originally removed to solve the issues it was causing when computing the source range of its parent TupleExpr. To account for trailing closures we walk back through the tuple's arguments until one with a valid location is found, which we use as the end location. If the last argument was a DefaultArgumentExpr though that meant the end loc would end up being the tuple's start location, so none of the tuple's other arguments were contained in its range, triggering an ASTVerifier assertion. Source tooling and diagnostics don't care about default arg expression locations as nothing can reference them, but their locations are output in the debug info. Added a regression test to catch that in future, and updated TupleExpr::getSourceRange() to ignore them when computing the end loc. Resolves rdar://problem/63195504.
If any arguments were defaulted the tuple/paren was made implicit, even though the original tuple/paren was present in the source.
This prevented some sourcekit ASTWalkers from considering them, making refactorings, documentation info, jump-to-definition and other features unavailable when queried via their argument labels.
Resolves rdar://problem/62118957