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

[Parse][Sema] Improve interpolation parsing and construction #24464

Merged
merged 4 commits into from May 15, 2019

Conversation

Projects
None yet
2 participants
@brentdax
Copy link
Collaborator

commented May 3, 2019

In Swift 4.2 mode, we currently diagnose and correct multi-argument and labeled interpolations during parsing. This isn't great because there are various modes where we parse code that won't actually compile in Swift 4.2 mode, like code in #if swift(>=5) blocks.

This PR moves this diagnosis to Sema, specifically to PreCheckExpression. This then allows us to parse string interpolations literally as argument lists, which (with another small fix) is enough to let code completion offer appendInterpolation overload signatures in a string interpolation segment.

While I was there, I also cleaned up some issues with the strange interpolation warning test, such as fixits that weren't being checked properly and accidental indentation. It's best to review the commits separately so you can see which parts of the test changed.

Resolves SR-9937.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

@swift-ci please smoke test

@brentdax brentdax force-pushed the brentdax:charmed-interpolations branch from a52ffd8 to d77ea1a May 3, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

@swift-ci please smoke test

@brentdax brentdax requested a review from rintaro May 3, 2019

@rintaro

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Thank you for looking into this!
Instead of guarding the block in Parser, is it possible to move this diagnostics and transformation to Sema?

Improve strange-interpolation test
There were a number of mistakes in this test:

• The whole thing was indented one space.
• Some fix-it tests were malformed and therefore not being tested.
• The output checking could in theory allow content before or after the intended content.

@brentdax brentdax force-pushed the brentdax:charmed-interpolations branch from d77ea1a to d253999 May 6, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

@rintaro I've moved it to PreCheckExpression, which runs just before type checking, and modified the tests to simply check that we don't diagnose in a failing #if in parse mode without checking that we do diagnose outside of one. However, the obvious cleanup this enables—parsing string interpolations by calling parseExprCallSuffix()—changes code completion behavior in a way that mostly seems to be an improvement, but has some problems. Do you have any thoughts about how to fix the little bugs mentioned in the FIXMEs?

@rintaro

This comment has been minimized.

Copy link
Member

commented May 6, 2019

TypeRelation[Invalid] probably because the context type analysis in code completion is not working correctly. I need to look AST dump to investigate it.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

@swift-ci please smoke test

@rintaro

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Ah, I see that was caused by invalid SourceLocs. Thank you!


virtual std::pair<bool, Expr *> walkToExprPre(Expr *E) {
assert(!isa<InterpolatedStringLiteralExpr>(E) &&
"StrangeInterpolationRewriter found nested interpolation?");

This comment has been minimized.

Copy link
@rintaro

rintaro May 7, 2019

Member

Could you add some nested test cases? like:

"[\(foo: "=\(bar: 0)=")]"

This comment has been minimized.

Copy link
@brentdax

brentdax May 8, 2019

Author Collaborator

Will do. (But it's safe because the only place nested interpolations can appear is as one of the parameters of an appendInterpolation(_:) call, and we stop recursing when we see CallExpr.)

This comment has been minimized.

Copy link
@brentdax

brentdax May 9, 2019

Author Collaborator

Done.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

@swift-ci please build toolchain

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

So…I accidentally made appendInterpolation(…) overloads autocomplete their parameter lists correctly.

Cool.

@brentdax brentdax requested a review from rintaro May 9, 2019

brentdax added some commits May 3, 2019

Move strange interpolation fix to PreCheckExpression
This defers diagnosis until a stage where #if conditions have definitely been evaluated, at the cost of a slightly more complex implementation. We’ll gain some of that complexity back in a subsequent refactoring. Fixes SR-9937.
[Parse] Parse string interpolations as args
Now that we manipulate the argument list to correct strange interpolations in Sema, we can parse interpolations directly as argument lists, simplifying the parser.

By itself, this refactoring causes a code completion regression; a subsequent commit will fix that.
[AST] Give appendInterpolation refs a SourceLoc
This change permits UnresolvedDotExpr to have both a name and a base that are implicit, but a valid DotLoc, and to treat that DotLoc as the node’s location. It then changes the generation of string interpolation code so that `$stringInterpolation.appendInterpolation` references have a DotLoc corresponding to the backslash in the string literal.

This makes it possible for `ExprContextAnalyzer` in IDE to correctly detect when you are code-completing in a string interpolation and treat it as an `appendInterpolation` call.

@brentdax brentdax force-pushed the brentdax:charmed-interpolations branch from 0857b44 to 4f1e05c May 9, 2019

@brentdax brentdax changed the title [Parse] Suppress strange interpolation warnings in parse-only modes [Parse][Sema] Improve interpolation parsing and construction May 9, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

Squashed away the intermediate state.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

@swift-ci please smoke test

@brentdax brentdax requested a review from slavapestov May 9, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2019

@swift-ci please smoke test and merge

@brentdax brentdax merged commit 82928fd into apple:master May 15, 2019

2 of 3 checks passed

Test and Merge (smoke test) Build finished.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.