Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 15, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59d2fea
Improve strange-interpolation test
beccadax May 2, 2019
a4e5bce
Move strange interpolation fix to PreCheckExpression
beccadax May 3, 2019
da74978
[Parse] Parse string interpolations as args
beccadax May 6, 2019
4f1e05c
[AST] Give appendInterpolation refs a SourceLoc
beccadax May 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions include/swift/AST/Expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2377,14 +2377,30 @@ class UnresolvedDotExpr : public Expr {
: FunctionRefKind::Unapplied);
}

SourceLoc getLoc() const { return NameLoc.getBaseNameLoc(); }
SourceLoc getLoc() const {
if (NameLoc.isValid())
return NameLoc.getBaseNameLoc();
else if (DotLoc.isValid())
return DotLoc;
else
return SubExpr->getEndLoc();
}

SourceLoc getStartLoc() const {
return (DotLoc.isInvalid() ? NameLoc.getSourceRange().End
: SubExpr->getStartLoc());
if (SubExpr->getStartLoc().isValid())
return SubExpr->getStartLoc();
else if (DotLoc.isValid())
return DotLoc;
else
return NameLoc.getSourceRange().Start;
}
SourceLoc getEndLoc() const {
return NameLoc.getSourceRange().End;
if (NameLoc.isValid())
return NameLoc.getSourceRange().End;
else if (DotLoc.isValid())
return DotLoc;
else
return SubExpr->getEndLoc();
}

SourceLoc getDotLoc() const { return DotLoc; }
Expand Down
85 changes: 15 additions & 70 deletions lib/Parse/ParseExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1863,8 +1863,8 @@ parseStringSegments(SmallVectorImpl<Lexer::StringSegment> &Segments,
SyntaxKind::ExpressionSegment);

// Backslash is part of an expression segment.
Token BackSlash(tok::backslash,
CharSourceRange(Segment.Loc.getAdvancedLoc(-1), 1).str());
SourceLoc BackSlashLoc = Segment.Loc.getAdvancedLoc(-1);
Token BackSlash(tok::backslash, CharSourceRange(BackSlashLoc, 1).str());
ExprContext.addToken(BackSlash, EmptyTrivia, EmptyTrivia);
// Create a temporary lexer that lexes from the body of the string.
LexerState BeginState =
Expand All @@ -1888,30 +1888,12 @@ parseStringSegments(SmallVectorImpl<Lexer::StringSegment> &Segments,
TokReceiver->registerTokenKindChange(Tok.getLoc(),
tok::string_interpolation_anchor);

SourceLoc lParen, rParen;
SmallVector<Expr *, 4> args;
SmallVector<Identifier, 4> argLabels;
SmallVector<SourceLoc, 4> argLabelLocs;
Expr *trailingClosureNeverPresent;
ParserStatus S =
parseExprList(tok::l_paren, tok::r_paren,
/*isPostfix=*/false, /*isExprBasic=*/true,
lParen, args, argLabels, argLabelLocs, rParen,
trailingClosureNeverPresent,
SyntaxKind::FunctionCallArgumentList);
assert(!trailingClosureNeverPresent);

Status |= S;
// If there was an error parsing a parameter, add an ErrorExpr to
// represent it. Prevents spurious errors about a nonexistent
// appendInterpolation() overload.
if (S.isError() && args.size() == 0)
args.push_back(new (Context) ErrorExpr(SourceRange(lParen, rParen)));

while (argLabels.size() < args.size())
argLabels.push_back(Identifier());
while (argLabelLocs.size() < args.size())
argLabelLocs.push_back(SourceLoc());
auto callee = new (Context) UnresolvedDotExpr(InterpolationVarRef,
/*dotloc=*/BackSlashLoc,
appendInterpolation,
/*nameloc=*/DeclNameLoc(),
/*Implicit=*/true);
auto S = parseExprCallSuffix(makeParserResult(callee), true);

// If we stopped parsing the expression before the expression segment is
// over, eat the remaining tokens into a token list
Expand All @@ -1925,51 +1907,14 @@ parseStringSegments(SmallVectorImpl<Lexer::StringSegment> &Segments,
L->getLocForEndOfToken(SourceMgr, Tok.getLoc()));
}

Expr *call = S.getPtrOrNull();
if (!call)
call = new (Context) ErrorExpr(SourceRange(Segment.Loc,
Segment.getEndLoc()));

InterpolationCount += 1;

// In Swift 4.2 and earlier, a single argument with a label would ignore
// the label (at best), and multiple arguments would form a tuple.
if (!Context.isSwiftVersionAtLeast(5)) {
if (args.size() > 1) {
diagnose(args[1]->getLoc(), diag::string_interpolation_list_changing)
.highlightChars(args[1]->getLoc(), rParen);
diagnose(args[1]->getLoc(),
diag::string_interpolation_list_insert_parens)
.fixItInsertAfter(lParen, "(")
.fixItInsert(rParen, ")");

args = {
TupleExpr::create(Context,
lParen, args, argLabels, argLabelLocs, rParen,
/*hasTrailingClosure=*/false,
/*Implicit=*/false)
};
argLabels = { Identifier() };
argLabelLocs = { SourceLoc() };
}
else if (args.size() == 1 && argLabels[0] != Identifier()) {
diagnose(argLabelLocs[0], diag::string_interpolation_label_changing)
.highlightChars(argLabelLocs[0], args[0]->getStartLoc());
diagnose(argLabelLocs[0], diag::string_interpolation_remove_label,
argLabels[0])
.fixItRemoveChars(argLabelLocs[0], args[0]->getStartLoc());

argLabels[0] = Identifier();
}
}

auto AppendInterpolationRef =
new (Context) UnresolvedDotExpr(InterpolationVarRef,
/*dotloc=*/SourceLoc(),
appendInterpolation,
/*nameloc=*/DeclNameLoc(),
/*Implicit=*/true);
auto AppendInterpolationCall =
CallExpr::create(Context, AppendInterpolationRef,
lParen, args, argLabels, argLabelLocs, rParen,
/*trailingClosure=*/nullptr, /*implicit=*/false);

Stmts.push_back(AppendInterpolationCall);
Stmts.push_back(call);
Status |= S;

if (!Tok.is(tok::eof)) {
diagnose(Tok, diag::string_interpolation_extra);
Expand Down
104 changes: 104 additions & 0 deletions lib/Sema/TypeCheckConstraints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,107 @@ namespace {
/// the type conforms to the expected literal protocol.
Expr *simplifyTypeConstructionWithLiteralArg(Expr *E);

/// In Swift < 5, diagnose and correct invalid multi-argument or
/// argument-labeled interpolations.
void correctInterpolationIfStrange(InterpolatedStringLiteralExpr *ISLE) {
// These expressions are valid in Swift 5+.
if (TC.Context.isSwiftVersionAtLeast(5))
return;

/// Diagnoses appendInterpolation(...) calls with multiple
/// arguments or argument labels and corrects them.
class StrangeInterpolationRewriter : public ASTWalker {
TypeChecker &TC;

public:
StrangeInterpolationRewriter(TypeChecker &TC) : TC(TC) { }

virtual bool walkToDeclPre(Decl *D) {
// We don't want to look inside decls.
return false;
}

virtual std::pair<bool, Expr *> walkToExprPre(Expr *E) {
// One InterpolatedStringLiteralExpr should never be nested inside
// another except as a child of a CallExpr, and we don't recurse into
// the children of CallExprs.
assert(!isa<InterpolatedStringLiteralExpr>(E) &&
"StrangeInterpolationRewriter found nested interpolation?");
Copy link
Member

@rintaro rintaro May 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could you add some nested test cases? like:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// We only care about CallExprs.
if (!isa<CallExpr>(E))
return { true, E };

auto call = cast<CallExpr>(E);
if (auto callee = dyn_cast<UnresolvedDotExpr>(call->getFn())) {
if (callee->getName().getBaseName() ==
TC.Context.Id_appendInterpolation) {
Expr *newArg = nullptr;
SourceLoc lParen, rParen;

if (call->getNumArguments() > 1) {
auto *args = cast<TupleExpr>(call->getArg());

lParen = args->getLParenLoc();
rParen = args->getRParenLoc();
Expr *secondArg = args->getElement(1);

TC.diagnose(secondArg->getLoc(),
diag::string_interpolation_list_changing)
.highlightChars(secondArg->getLoc(), rParen);
TC.diagnose(secondArg->getLoc(),
diag::string_interpolation_list_insert_parens)
.fixItInsertAfter(lParen, "(")
.fixItInsert(rParen, ")");

newArg = args;
}
else if(call->getNumArguments() == 1 &&
call->getArgumentLabels().front() != Identifier()) {
auto *args = cast<TupleExpr>(call->getArg());
newArg = args->getElement(0);

lParen = args->getLParenLoc();
rParen = args->getRParenLoc();

SourceLoc argLabelLoc = call->getArgumentLabelLoc(0),
argLoc = newArg->getStartLoc();

TC.diagnose(argLabelLoc,
diag::string_interpolation_label_changing)
.highlightChars(argLabelLoc, argLoc);
TC.diagnose(argLabelLoc,
diag::string_interpolation_remove_label,
call->getArgumentLabels().front())
.fixItRemoveChars(argLabelLoc, argLoc);
}

// If newArg is no longer null, we need to build a new
// appendInterpolation(_:) call that takes it to replace the bad
// appendInterpolation(...) call.
if (newArg) {
auto newCallee = new (TC.Context) UnresolvedDotExpr(
callee->getBase(), /*dotloc=*/SourceLoc(),
DeclName(TC.Context.Id_appendInterpolation),
/*nameloc=*/DeclNameLoc(), /*Implicit=*/true);

E = CallExpr::create(TC.Context, newCallee, lParen,
{ newArg }, { Identifier() }, { SourceLoc() },
rParen, /*trailingClosure=*/nullptr, /*implicit=*/false);
}
}
}

// There is never a CallExpr between an InterpolatedStringLiteralExpr
// and an un-typechecked appendInterpolation(...) call, so whether we
// changed E or not, we don't need to recurse any deeper.
return { false, E };
}
};

ISLE->getAppendingExpr()->walk(StrangeInterpolationRewriter(TC));
}

public:
PreCheckExpression(TypeChecker &tc, DeclContext *dc, Expr *parent)
: TC(tc), DC(dc), ParentExpr(parent) {}
Expand Down Expand Up @@ -1086,6 +1187,9 @@ namespace {
return finish(false, nullptr);
}

if (auto *ISLE = dyn_cast<InterpolatedStringLiteralExpr>(expr))
correctInterpolationIfStrange(ISLE);

return finish(true, expr);
}

Expand Down
6 changes: 6 additions & 0 deletions test/IDE/complete_at_top_level.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ func fooFunc2(_ a: Int, _ b: Double) {}

func erroneous1(_ x: Undeclared) {}

// FIXME: Hides all other string interpolation completion.
//extension DefaultStringInterpolation {
// mutating func appendInterpolation(interpolate: Double) {}
//}

//===--- Test code completions of expressions that can be typechecked.

// Although the parser can recover in most of these test cases, we resync it
Expand Down Expand Up @@ -465,6 +470,7 @@ func resyncParserB14() {}
var stringInterp = "\(#^STRING_INTERP_3^#)"
_ = "" + "\(#^STRING_INTERP_4^#)" + ""
// STRING_INTERP: Begin completions
// STRING_INTERP-DAG: Decl[InstanceMethod]/CurrNominal: ['(']{#(value): _#}[')'][#Void#]; name=value: _
// STRING_INTERP-DAG: Decl[Struct]/CurrModule: FooStruct[#FooStruct#];
// STRING_INTERP-DAG: Decl[FreeFunction]/CurrModule/NotRecommended/TypeRelation[Invalid]: fooFunc1()[#Void#];
// STRING_INTERP-DAG: Decl[FreeFunction]/CurrModule: optStr()[#String?#];
Expand Down
101 changes: 57 additions & 44 deletions test/Parse/strange_interpolation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,47 +6,60 @@

// REQUIRES: executable_test

let x = 1

print("[\(x)]")
// CHECK: [1]

print("[\(foo: x)]")
// CHECK: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'foo' label to keep current behavior}} {{12-17=}}

print("[\(x, x)]")
// CHECK: [(1, 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{12-12=(}} {{16-16=)}}

print("[\(foo: x, x)]")
// CHECK: [(foo: 1, 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{12-12=(}} {{21-21=)}}

print("[\(x, foo: x)]")
// CHECK: [(1, foo: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {11-11(}} {{20-20=)}}

print("[\(foo: x, foo: x)]")
// CHECK: [(foo: 1, foo: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {11-11(}} {{25-25=)}}

print("[\(describing: x)]")
// CHECK: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'describing' label to keep current behavior}} {{12-24=}}

print("[\(x, radix: x)]")
// CHECK: [(1, radix: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {11-11(}} {{25-25=)}}

print("[\(stringInterpolationSegment: x)]")
// CHECK: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'stringInterpolationSegment' label to keep current behavior}} {{12-40=}}
print("Begin")
// CHECK: Begin

let x = 1

print("[\(x)]")
// CHECK-NEXT: [1]

print("[\(foo: x)]")
// CHECK-NEXT: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'foo' label to keep current behavior}} {{11-16=}}

print("[\(x, x)]")
// CHECK-NEXT: [(1, 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{15-15=)}}

print("[\(foo: x, x)]")
// CHECK-NEXT: [(foo: 1, 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{20-20=)}}

print("[\(x, foo: x)]")
// CHECK-NEXT: [(1, foo: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{20-20=)}}

print("[\(foo: x, foo: x)]")
// CHECK-NEXT: [(foo: 1, foo: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{25-25=)}}

print("[\(describing: x)]")
// CHECK-NEXT: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'describing' label to keep current behavior}} {{11-23=}}

print("[\(x, radix: x)]")
// CHECK-NEXT: [(1, radix: 1)]
// expected-warning@-2{{interpolating multiple values will not form a tuple in Swift 5}}
// expected-note@-3{{insert parentheses to keep current behavior}} {{11-11=(}} {{22-22=)}}

print("[\(stringInterpolationSegment: x)]")
// CHECK-NEXT: [1]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'stringInterpolationSegment' label to keep current behavior}} {{11-39=}}

print("[ \(foo: "[\(bar: x)]") ]")
// CHECK-NEXT: [ [1] ]
// expected-warning@-2{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-3{{remove 'foo' label to keep current behavior}} {{12-17=}}
// expected-warning@-4{{labeled interpolations will not be ignored in Swift 5}}
// expected-note@-5{{remove 'bar' label to keep current behavior}} {{21-26=}}

print("End")
// CHECK-NEXT: End
Loading