[5.3][AST] Restore getSourceRange() on DefaultArgumentExpr #31873
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@swift-ci please test |
nathawes
changed the title
|
@swift-ci please nominate |
akyrtzi
approved these changes
May 19, 2020
tkremenek
approved these changes
May 19, 2020
AnthonyLatsis
added
swift 5.3
🍒 release cherry pick
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #31861
Explanation:
This restores getSourceRange() on DefaultArgumentExpr after it was removed in
[Sema] Maintain the implicitness of call argument tuple/parens in coerceCallArguments #31184. While source tooling and diagnostics don't care about default arg expression locations (nothing can reference them and any issues in them should be diagnosed on the callee rather than caller side), their locations are output in the debug info, so their removal resulted in a debug info regression (rdar://problem/63195504).
It was originally removed to solve the issues it was causing when computing the source range of TupleExprs containing defaulted arguments after a trailing closure. The implementation assumes its argument expressions with valid source locations appear in source order, which default argument expressions violate (they point to the start of their parent tuple expression). This change works around that issue with a more targeted fix instead by updating TupleExpr::getSourceRange() to simply ignore DefaultArgExprs.
Scope of issue: This results in having no debug info for frames corresponding to default argument expressions.
Origination: This regressed due to a part of the fix to stop tuple expressions being incorrectly flagged as implicit when they contained defaulted arguments: [Sema] Maintain the implicitness of call argument tuple/parens in coerceCallArguments #31184 (rdar 62118957)
Risk: Low. This restores the previous functionality and adds a more targeted fix for the original issue it was resolving.
Testing: Added a regression test to catch the debug info regression in future. All regression tests pass.
Reviewer: @xedin (on the master PR)
Resolves rdar://problem/63195504