Skip to content

Commit 3753d77

Browse files
DougGregorDougGregor
committed
[Type checker] Be more rigorous about extracting argument labels from calls.
Whenever we have a call, retrieve the argument labels from the argument structurally and associate them with the callee. We were previously doing this as a separate AST walk (which was unnecessary), so fold that into constraint generation for a CallExpr. We were also allowing weird ASTs to effectively disable this information: tighten that up and require that CallExprs always have a ParenExpr, TupleExpr, or (as a temporary hack) a TypeExpr whose representation is a TupleTypeRepr as their argument prior to type checking. This gives us a more sane AST to work with, and guarantees that we aren't losing label information. From the user perspective, this should be NFC, because it's mostly AST cleanup and staging.
1 parent fb6fb95 commit 3753d77

File tree

12 files changed

+163
-112
lines changed

12 files changed

+163
-112
lines changed

‎include/swift/AST/Expr.h

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1621,6 +1621,9 @@ class ParenExpr : public IdentityExpr {
16211621
/// \brief Whether this expression has a trailing closure as its argument.
16221622
bool hasTrailingClosure() const { return HasTrailingClosure; }
16231623

1624+
/// Create a new, implicit parenthesized expression.
1625+
static ParenExpr *createImplicit(ASTContext &ctx, Expr *expr);
1626+
16241627
static bool classof(const Expr *E) { return E->getKind() == ExprKind::Paren; }
16251628
};
16261629

@@ -3195,7 +3198,15 @@ class CallExpr : public ApplyExpr {
31953198
SourceLoc FnLoc = getFn()->getLoc();
31963199
return FnLoc.isValid() ? FnLoc : getArg()->getLoc();
31973200
}
3198-
3201+
3202+
/// Retrieve the expression that direct represents the callee.
3203+
///
3204+
/// The "direct" callee is the expression representing the callee
3205+
/// after looking through top-level constructs that don't affect the
3206+
/// identity of the callee, e.g., extra parentheses, optional
3207+
/// unwrapping (?)/forcing (!), etc.
3208+
Expr *getDirectCallee() const;
3209+
31993210
static bool classof(const Expr *E) { return E->getKind() == ExprKind::Call; }
32003211
};
32013212

‎lib/AST/Expr.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -922,6 +922,14 @@ SequenceExpr *SequenceExpr::create(ASTContext &ctx, ArrayRef<Expr*> elements) {
922922
return ::new(Buffer) SequenceExpr(elements);
923923
}
924924

925+
ParenExpr *ParenExpr::createImplicit(ASTContext &ctx, Expr *expr) {
926+
ParenExpr *result = new (ctx) ParenExpr(SourceLoc(), expr, SourceLoc(),
927+
/*hasTrailingClosure=*/false,
928+
expr->getType());
929+
result->setImplicit();
930+
return result;
931+
}
932+
925933
SourceLoc TupleExpr::getStartLoc() const {
926934
if (LParenLoc.isValid()) return LParenLoc;
927935
if (getNumElements() == 0) return SourceLoc();
@@ -1033,6 +1041,25 @@ ValueDecl *ApplyExpr::getCalledValue() const {
10331041
return ::getCalledValue(Fn);
10341042
}
10351043

1044+
Expr *CallExpr::getDirectCallee() const {
1045+
auto fn = getFn();
1046+
while (true) {
1047+
fn = fn->getSemanticsProvidingExpr();
1048+
1049+
if (auto force = dyn_cast<ForceValueExpr>(fn)) {
1050+
fn = force->getSubExpr();
1051+
continue;
1052+
}
1053+
1054+
if (auto bind = dyn_cast<BindOptionalExpr>(fn)) {
1055+
fn = bind->getSubExpr();
1056+
continue;
1057+
}
1058+
1059+
return fn;
1060+
}
1061+
}
1062+
10361063
RebindSelfInConstructorExpr::RebindSelfInConstructorExpr(Expr *SubExpr,
10371064
VarDecl *Self)
10381065
: Expr(ExprKind::RebindSelfInConstructor, /*Implicit=*/true,

‎lib/ClangImporter/ImportDecl.cpp

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -380,8 +380,11 @@ makeEnumRawValueConstructor(ClangImporter::Implementation &Impl,
380380
return ctorDecl;
381381

382382
auto selfRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(), /*implicit*/true);
383-
auto paramRef = new (C) DeclRefExpr(param, DeclNameLoc(),
384-
/*implicit*/ true);
383+
Expr *paramRef = new (C) DeclRefExpr(param, DeclNameLoc(),
384+
/*implicit*/ true);
385+
paramRef = ParenExpr::createImplicit(C, paramRef);
386+
paramRef->setImplicit();
387+
385388
auto reinterpretCast
386389
= cast<FuncDecl>(getBuiltinValueDecl(C,C.getIdentifier("reinterpretCast")));
387390
auto reinterpretCastRef
@@ -440,7 +443,11 @@ static FuncDecl *makeEnumRawValueGetter(ClangImporter::Implementation &Impl,
440443
if (Impl.hasFinishedTypeChecking())
441444
return getterDecl;
442445

443-
auto selfRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(), /*implicit*/true);
446+
Expr *selfRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(),
447+
/*implicit*/true);
448+
selfRef = ParenExpr::createImplicit(C, selfRef);
449+
selfRef->setImplicit();
450+
444451
auto reinterpretCast
445452
= cast<FuncDecl>(getBuiltinValueDecl(C, C.getIdentifier("reinterpretCast")));
446453
auto reinterpretCastRef
@@ -616,8 +623,11 @@ makeUnionFieldAccessors(ClangImporter::Implementation &Impl,
616623
{
617624
auto selfDecl = getterDecl->getImplicitSelfDecl();
618625

619-
auto selfRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(),
620-
/*implicit*/ true);
626+
Expr *selfRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(),
627+
/*implicit*/ true);
628+
selfRef = ParenExpr::createImplicit(C, selfRef);
629+
selfRef->setImplicit();
630+
621631
auto reinterpretCast = cast<FuncDecl>(getBuiltinValueDecl(
622632
C, C.getIdentifier("reinterpretCast")));
623633
auto reinterpretCastRef
@@ -638,8 +648,10 @@ makeUnionFieldAccessors(ClangImporter::Implementation &Impl,
638648

639649
auto inoutSelfRef = new (C) DeclRefExpr(inoutSelfDecl, DeclNameLoc(),
640650
/*implicit*/ true);
641-
auto inoutSelf = new (C) InOutExpr(SourceLoc(), inoutSelfRef,
651+
Expr *inoutSelf = new (C) InOutExpr(SourceLoc(), inoutSelfRef,
642652
InOutType::get(importedUnionDecl->getType()), /*implicit*/ true);
653+
inoutSelf = ParenExpr::createImplicit(C, inoutSelf);
654+
inoutSelf->setImplicit();
643655

644656
auto newValueDecl = setterDecl->getParameterList(1)->get(0);
645657

‎lib/Sema/CSApply.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4766,6 +4766,10 @@ Expr *ExprRewriter::coerceCallArguments(Expr *arg, Type paramType,
47664766
arg = fromTupleExpr[0];
47674767
} else if (argParen) {
47684768
// If the element changed, rebuild a new ParenExpr.
4769+
if (!(fromTupleExpr.size() == 1 && fromTupleExpr[0])) {
4770+
arg->dump();
4771+
}
4772+
47694773
assert(fromTupleExpr.size() == 1 && fromTupleExpr[0]);
47704774
if (fromTupleExpr[0] != argParen->getSubExpr()) {
47714775
bool argParenImplicit = argParen->isImplicit();
@@ -5254,6 +5258,8 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
52545258
Expr *fnRef = new (tc.Context) DeclRefExpr(fnDeclRef,
52555259
DeclNameLoc(expr->getLoc()),
52565260
/*Implicit=*/true);
5261+
expr = ParenExpr::createImplicit(tc.Context, expr);
5262+
52575263
fnRef->setType(fn->getInterfaceType());
52585264
Expr *call = new (tc.Context) CallExpr(fnRef, expr,
52595265
/*implicit*/ true);
@@ -6593,6 +6599,8 @@ Expr *TypeChecker::callWitness(Expr *base, DeclContext *dc,
65936599
argumentNamesMatch(arguments[0],
65946600
witness->getFullName().getArgumentNames()))) {
65956601
arg = arguments[0];
6602+
if (!isa<TupleExpr>(arg) && !isa<ParenExpr>(arg))
6603+
arg = ParenExpr::createImplicit(Context, arg);
65966604
} else {
65976605
SmallVector<TupleTypeElt, 4> elementTypes;
65986606
auto names = witness->getFullName().getArgumentNames();
@@ -6840,6 +6848,7 @@ Expr *Solution::convertOptionalToBool(Expr *expr,
68406848
fnRef->setType(fn->getInterfaceType().subst(
68416849
constraintSystem->DC->getParentModule(), subMap, None));
68426850

6851+
expr = ParenExpr::createImplicit(ctx, expr);
68436852
Expr *call = new (ctx) CallExpr(fnRef, expr, /*Implicit=*/true);
68446853

68456854
bool failed = tc.typeCheckExpressionShallow(call, cs.DC);

‎lib/Sema/CSGen.cpp

Lines changed: 62 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -2429,11 +2429,71 @@ namespace {
24292429
return expr->getType();
24302430
}
24312431

2432+
using ArgumentLabelState = ConstraintSystem::ArgumentLabelState;
2433+
2434+
/// Extract argument labels from the given call argument.
2435+
ArgumentLabelState getArgumentLabelsForCallArgument(Expr *arg) {
2436+
// Parentheses.
2437+
if (auto paren = dyn_cast<ParenExpr>(arg)) {
2438+
return ArgumentLabelState{
2439+
CS.allocateCopy(llvm::makeArrayRef(Identifier())),
2440+
paren->hasTrailingClosure() };
2441+
}
2442+
2443+
// Type representations.
2444+
// FIXME: This should never happen, but
2445+
// PreCheckExpression::simplifyTypeExpr() tends to pull in the
2446+
// parentheses of call arguments overly eagerly, and the fix
2447+
// implies enforcing ".self" more rigorously than we do now,
2448+
// which is at odds with the expected direction of SE-0090.
2449+
if (auto typeExpr = dyn_cast<TypeExpr>(arg)) {
2450+
auto typeRepr = typeExpr->getTypeRepr();
2451+
auto tupleType = cast<TupleTypeRepr>(typeRepr);
2452+
2453+
// Collect the names.
2454+
SmallVector<Identifier, 4> names;
2455+
names.reserve(tupleType->getElements().size());
2456+
for (auto elt : tupleType->getElements()) {
2457+
if (auto named = dyn_cast<NamedTypeRepr>(elt))
2458+
names.push_back(named->getName());
2459+
else
2460+
names.push_back(Identifier());
2461+
}
2462+
2463+
return ArgumentLabelState{CS.allocateCopy(names), false};
2464+
}
2465+
2466+
if (!isa<TupleExpr>(arg)) arg->dump();
2467+
2468+
// Tuples.
2469+
auto tuple = cast<TupleExpr>(arg);
2470+
2471+
// If we have element names, use 'em.
2472+
if (tuple->hasElementNames()) {
2473+
return ArgumentLabelState{tuple->getElementNames(),
2474+
tuple->hasTrailingClosure()};
2475+
}
2476+
2477+
// Otherwise, there are no argument labels.
2478+
llvm::SmallVector<Identifier, 4> names(tuple->getNumElements(),
2479+
Identifier());
2480+
return ArgumentLabelState{CS.allocateCopy(names),
2481+
tuple->hasTrailingClosure()};
2482+
}
2483+
24322484
Type visitApplyExpr(ApplyExpr *expr) {
24332485
Type outputTy;
24342486

24352487
auto fnExpr = expr->getFn();
2436-
2488+
2489+
// Identify and record the argument labels for a call.
2490+
if (auto call = dyn_cast<CallExpr>(expr)) {
2491+
auto argumentLabels = getArgumentLabelsForCallArgument(expr->getArg());
2492+
auto callee = call->getDirectCallee();
2493+
CS.CalleeArgumentLabels[CS.getConstraintLocator(callee)] =
2494+
argumentLabels;
2495+
}
2496+
24372497
if (isa<DeclRefExpr>(fnExpr)) {
24382498
if (auto fnType = fnExpr->getType()->getAs<AnyFunctionType>()) {
24392499
outputTy = fnType->getResult();
@@ -2536,7 +2596,7 @@ namespace {
25362596
FunctionType::ExtInfo extInfo;
25372597
if (isa<ClosureExpr>(fnExpr->getSemanticsProvidingExpr()))
25382598
extInfo = extInfo.withNoEscape();
2539-
2599+
25402600
auto funcTy = FunctionType::get(expr->getArg()->getType(), outputTy,
25412601
extInfo);
25422602

@@ -2999,87 +3059,12 @@ namespace {
29993059
/// \brief Ignore declarations.
30003060
bool walkToDeclPre(Decl *decl) override { return false; }
30013061
};
3002-
3003-
/// AST walker that records the keyword arguments provided at each
3004-
/// call site.
3005-
class ArgumentLabelWalker : public ASTWalker {
3006-
ConstraintSystem &CS;
3007-
llvm::DenseMap<Expr *, Expr *> ParentMap;
3008-
3009-
public:
3010-
ArgumentLabelWalker(ConstraintSystem &cs, Expr *expr)
3011-
: CS(cs), ParentMap(expr->getParentMap()) { }
3012-
3013-
using State = ConstraintSystem::ArgumentLabelState;
3014-
3015-
void associateArgumentLabels(Expr *arg, State labels,
3016-
bool labelsArePermanent) {
3017-
// Our parent must be a call.
3018-
auto call = dyn_cast_or_null<CallExpr>(ParentMap[arg]);
3019-
if (!call)
3020-
return;
3021-
3022-
// We must have originated at the call argument.
3023-
if (arg != call->getArg())
3024-
return;
3025-
3026-
// Dig out the function, looking through, parentheses, ?, and !.
3027-
auto fn = call->getFn();
3028-
do {
3029-
fn = fn->getSemanticsProvidingExpr();
3030-
3031-
if (auto force = dyn_cast<ForceValueExpr>(fn)) {
3032-
fn = force->getSubExpr();
3033-
continue;
3034-
}
3035-
3036-
if (auto bind = dyn_cast<BindOptionalExpr>(fn)) {
3037-
fn = bind->getSubExpr();
3038-
continue;
3039-
}
3040-
3041-
break;
3042-
} while (true);
3043-
3044-
// Record the labels.
3045-
if (!labelsArePermanent)
3046-
labels.Labels = CS.allocateCopy(labels.Labels);
3047-
CS.ArgumentLabels[CS.getConstraintLocator(fn)] = labels;
3048-
}
3049-
3050-
std::pair<bool, Expr *> walkToExprPre(Expr *expr) override {
3051-
if (auto tuple = dyn_cast<TupleExpr>(expr)) {
3052-
if (tuple->hasElementNames())
3053-
associateArgumentLabels(expr,
3054-
{ tuple->getElementNames(),
3055-
tuple->hasTrailingClosure() },
3056-
/*labelsArePermanent*/ true);
3057-
else {
3058-
llvm::SmallVector<Identifier, 4> names(tuple->getNumElements(),
3059-
Identifier());
3060-
associateArgumentLabels(expr, { names, tuple->hasTrailingClosure() },
3061-
/*labelsArePermanent*/ false);
3062-
}
3063-
} else if (auto paren = dyn_cast<ParenExpr>(expr)) {
3064-
associateArgumentLabels(paren,
3065-
{ { Identifier() },
3066-
paren->hasTrailingClosure() },
3067-
/*labelsArePermanent*/ false);
3068-
}
3069-
3070-
return { true, expr };
3071-
}
3072-
};
3073-
30743062
} // end anonymous namespace
30753063

30763064
Expr *ConstraintSystem::generateConstraints(Expr *expr) {
30773065
// Remove implicit conversions from the expression.
30783066
expr = expr->walk(SanitizeExpr(getTypeChecker()));
30793067

3080-
// Walk the expression to associate labeled arguments.
3081-
expr->walk(ArgumentLabelWalker(*this, expr));
3082-
30833068
// Walk the expression, generating constraints.
30843069
ConstraintGenerator cg(*this);
30853070
ConstraintWalker cw(cg);

‎lib/Sema/CSSimplify.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2612,8 +2612,8 @@ getArgumentLabels(ConstraintSystem &cs, ConstraintLocatorBuilder locator) {
26122612
if (!parts.empty())
26132613
return None;
26142614

2615-
auto known = cs.ArgumentLabels.find(cs.getConstraintLocator(anchor));
2616-
if (known == cs.ArgumentLabels.end())
2615+
auto known = cs.CalleeArgumentLabels.find(cs.getConstraintLocator(anchor));
2616+
if (known == cs.CalleeArgumentLabels.end())
26172617
return None;
26182618

26192619
return known->second;

‎lib/Sema/CodeSynthesis.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -359,16 +359,12 @@ static Expr *buildArgumentForwardingExpr(ArrayRef<ParamDecl*> params,
359359

360360
// A single unlabelled value is not a tuple.
361361
if (args.size() == 1 && labels[0].empty())
362-
return args[0];
362+
return ParenExpr::createImplicit(ctx, args[0]);
363363

364364
return TupleExpr::create(ctx, SourceLoc(), args, labels, labelLocs,
365365
SourceLoc(), false, IsImplicit);
366366
}
367367

368-
369-
370-
371-
372368
/// Build a reference to the subscript index variables for this subscript
373369
/// accessor.
374370
static Expr *buildSubscriptIndexReference(ASTContext &ctx, FuncDecl *accessor) {
@@ -919,8 +915,10 @@ void swift::synthesizeObservingAccessors(VarDecl *VD, TypeChecker &TC) {
919915
// (call_expr (decl_ref_expr(willSet)), (declrefexpr(value)))
920916
if (auto willSet = VD->getWillSetFunc()) {
921917
Expr *Callee = new (Ctx) DeclRefExpr(willSet, DeclNameLoc(), /*imp*/true);
922-
auto *ValueDRE = new (Ctx) DeclRefExpr(ValueDecl, DeclNameLoc(),
918+
Expr *ValueDRE = new (Ctx) DeclRefExpr(ValueDecl, DeclNameLoc(),
923919
/*imp*/true);
920+
ValueDRE = ParenExpr::createImplicit(Ctx, ValueDRE);
921+
924922
if (SelfDecl) {
925923
auto *SelfDRE = new (Ctx) DeclRefExpr(SelfDecl, DeclNameLoc(),
926924
/*imp*/true);
@@ -945,8 +943,10 @@ void swift::synthesizeObservingAccessors(VarDecl *VD, TypeChecker &TC) {
945943
// or:
946944
// (call_expr (decl_ref_expr(didSet)), (decl_ref_expr(tmp)))
947945
if (auto didSet = VD->getDidSetFunc()) {
948-
auto *OldValueExpr = new (Ctx) DeclRefExpr(OldValue, DeclNameLoc(),
946+
Expr *OldValueExpr = new (Ctx) DeclRefExpr(OldValue, DeclNameLoc(),
949947
/*impl*/true);
948+
OldValueExpr = ParenExpr::createImplicit(Ctx, OldValueExpr);
949+
950950
Expr *Callee = new (Ctx) DeclRefExpr(didSet, DeclNameLoc(), /*imp*/true);
951951
if (SelfDecl) {
952952
auto *SelfDRE = new (Ctx) DeclRefExpr(SelfDecl, DeclNameLoc(),
@@ -1231,7 +1231,7 @@ void TypeChecker::completePropertyBehaviorStorage(VarDecl *VD,
12311231
// Type-check the expression.
12321232
typeCheckExpression(InitValue, DC);
12331233

1234-
InitStorageParam = InitValue;
1234+
InitStorageParam = ParenExpr::createImplicit(Context, InitValue);
12351235
} else {
12361236
InitStorageParam = TupleExpr::createEmpty(Context,
12371237
SourceLoc(), SourceLoc(),

0 commit comments

Comments
 (0)