From 872a1c85924eba44d84f52533bcdd5ebf564378e Mon Sep 17 00:00:00 2001 From: Sean Callanan Date: Wed, 1 Jul 2015 17:53:06 +0000 Subject: [PATCH] Sequester expressions the PlaygroundTransform adds, isolating type-check errors. We had a bit of back-and-forth over how to handle type-check errors in added expressions. Obviously all added expressions *should* type-check -- but we don't want the compiler (and, by extension, the XPC playground service) crashing when they don't. So the ErrorTypes from failed type-checks must not leak into existing code. In this solution, we use a convenience class (Added) that wraps an expression. It marks expressions we've constructed -- and we're only allowed to type-check Addeds. (This isn't fully enforced -- you could still have Addeds that refer to existing Expr*s) but adding this and plumbing it through caught most of the problems. I also added checks to various places that weren't checking whether type checking succeeded. In one case where we were emitting a source location for one element in a TupleExpr but not emitting source locations for the others (which caused dumping to assert) I eliminated that source location. Finally I added several test cases. These cases used to crash; one of them works perfectly now and I've XFAILed the other two. Swift SVN r29842 --- lib/Sema/PlaygroundTransform.cpp | 219 +++++++++++------- test/PlaygroundTransform/array_did_set.swift | 26 +++ .../PlaygroundTransform/array_in_struct.swift | 17 ++ test/PlaygroundTransform/print_inout.swift | 13 ++ 4 files changed, 191 insertions(+), 84 deletions(-) create mode 100644 test/PlaygroundTransform/array_did_set.swift create mode 100644 test/PlaygroundTransform/array_in_struct.swift create mode 100644 test/PlaygroundTransform/print_inout.swift diff --git a/lib/Sema/PlaygroundTransform.cpp b/lib/Sema/PlaygroundTransform.cpp index c743f6cd75a89..95bcaece8b75a 100644 --- a/lib/Sema/PlaygroundTransform.cpp +++ b/lib/Sema/PlaygroundTransform.cpp @@ -40,6 +40,20 @@ using namespace swift; namespace { +template class Added { +private: + E Contents; +public: + Added() { } + Added(E &&NewContents) { Contents = NewContents; } + const Added &operator=(const Added &rhs) { + Contents = rhs.Contents; + return *this; + } + E &operator*() { return Contents; } + E &operator->() { return Contents; } +}; + class Instrumenter { private: std::mt19937_64 &RNG; @@ -120,7 +134,7 @@ class Instrumenter { break; } Elements.insert(Elements.begin() + EI, - buildScopeExit(BP.BraceRange)); + *buildScopeExit(BP.BraceRange)); ++EI; } return EI; @@ -330,17 +344,37 @@ class Instrumenter { return D; } - std::pair digForVariable(Expr *E) { + std::pair, ValueDecl *> digForVariable(Expr *E) { switch (E->getKind()) { default: if (auto *ICE = dyn_cast(E)) return digForVariable(ICE->getSubExpr()); - return std::make_pair(nullptr, nullptr); - case ExprKind::DeclRef: - return std::make_pair(E, cast(E)->getDecl()); - case ExprKind::MemberRef: - return std::make_pair( - E, cast(E)->getMember().getDecl()); + return std::make_pair(Added(nullptr), nullptr); + case ExprKind::DeclRef: { + ValueDecl *D = cast(E)->getDecl(); + Added DRE = + new (Context) DeclRefExpr(ConcreteDeclRef(D), + cast(E)->getLoc(), + true, // implicit + AccessSemantics::Ordinary, + cast(E)->getType()); + return std::make_pair(DRE, D); + } + case ExprKind::MemberRef: { + std::pair, ValueDecl *> BaseVariable = + digForVariable(cast(E)->getBase()); + if (!*BaseVariable.first || !BaseVariable.second) + return std::make_pair(nullptr, nullptr); + ValueDecl *M = cast(E)->getMember().getDecl(); + Added MRE( + new (Context) MemberRefExpr(*BaseVariable.first, + cast(E)->getDotLoc(), + ConcreteDeclRef(M), + cast(E)->getSourceRange(), + true, // implicit + AccessSemantics::Ordinary)); + return std::make_pair(MRE, M); + } case ExprKind::Load: return digForVariable(cast(E)->getSubExpr()); case ExprKind::ForceValue: @@ -351,7 +385,7 @@ class Instrumenter { } std::string digForName(Expr *E) { - Expr *RE = nullptr; + Added RE = nullptr; ValueDecl *VD = nullptr; std::tie(RE, VD) = digForVariable(E); if (VD) { @@ -439,16 +473,17 @@ class Instrumenter { } }; - bool doTypeCheck(ASTContext &Ctx, DeclContext *DC, Expr *&parsedExpr) { + bool doTypeCheck(ASTContext &Ctx, DeclContext *DC, + Added &parsedExpr) { DiagnosticEngine diags(Ctx.SourceMgr); ErrorGatherer errorGatherer(diags); const bool discardedExpr = false; TypeChecker TC(Ctx, diags); - TC.typeCheckExpression(parsedExpr, DC, Type(), Type(), discardedExpr, + TC.typeCheckExpression(*parsedExpr, DC, Type(), Type(), discardedExpr, FreeTypeVariableBinding::GenericParameters); - if (parsedExpr) { + if (*parsedExpr) { ErrorFinder errorFinder; parsedExpr->walk(errorFinder); if (!errorFinder.hadError() && !errorGatherer.hadError()) { @@ -478,14 +513,14 @@ class Instrumenter { if (auto *MRE = dyn_cast(AE->getDest())) { // an assignment to a property of an object counts as a mutation of // that object - Expr *Base_RE = nullptr; + Added Base_RE = nullptr; ValueDecl *BaseVD = nullptr; std::tie(Base_RE, BaseVD) = digForVariable(MRE->getBase()); - if (Base_RE) { - Expr *Log = logDeclOrMemberRef(Base_RE); - if (Log) { - Elements.insert(Elements.begin() + (EI + 1), Log); + if (*Base_RE) { + Added Log = logDeclOrMemberRef(Base_RE); + if (*Log) { + Elements.insert(Elements.begin() + (EI + 1), *Log); ++EI; } } @@ -506,18 +541,21 @@ class Instrumenter { AE->setImplicit(true); std::string Name = digForName(AE->getDest()); - Expr *Log = buildLoggerCall( + Added Log(buildLoggerCall( new (Context) DeclRefExpr(ConcreteDeclRef(PV.second), SourceLoc(), true, // implicit AccessSemantics::Ordinary, AE->getSrc()->getType()), - AE->getSrc()->getSourceRange(), Name.c_str()); - Elements[EI] = PV.first; - Elements.insert(Elements.begin() + (EI + 1), PV.second); - Elements.insert(Elements.begin() + (EI + 2), Log); - Elements.insert(Elements.begin() + (EI + 3), NAE); - EI += 3; + AE->getSrc()->getSourceRange(), Name.c_str())); + + if (*Log) { + Elements[EI] = PV.first; + Elements.insert(Elements.begin() + (EI + 1), PV.second); + Elements.insert(Elements.begin() + (EI + 2), *Log); + Elements.insert(Elements.begin() + (EI + 3), NAE); + EI += 3; + } } } else if (ApplyExpr *AE = dyn_cast(E)) { @@ -532,9 +570,10 @@ class Instrumenter { Expr *appendNewline = nullptr; if (ParenExpr *PE = dyn_cast(AE->getArg())) { object = PE->getSubExpr(); - appendNewline = - new (Context) BooleanLiteralExpr(true, SourceLoc(), true); - doTypeCheck(Context, TypeCheckDC, appendNewline); + Added appendNewlineAdded( + new (Context) BooleanLiteralExpr(true, SourceLoc(), true)); + doTypeCheck(Context, TypeCheckDC, appendNewlineAdded); + appendNewline = *appendNewlineAdded; } else if (TupleExpr *TE = dyn_cast(AE->getArg())) { if (TE->getNumElements() == 2) { object = TE->getElement(0); @@ -547,19 +586,22 @@ class Instrumenter { buildPatternAndVariable(object); std::pair appendNewlinePV = buildPatternAndVariable(appendNewline); - Expr *Log = logPrint(isDebugPrint, - objectPV.second, appendNewlinePV.second, - AE->getSourceRange()); - Elements[EI] = objectPV.first; - Elements.insert(Elements.begin() + (EI + 1), - objectPV.second); - Elements.insert(Elements.begin() + (EI + 2), - appendNewlinePV.first); - Elements.insert(Elements.begin() + (EI + 3), - appendNewlinePV.second); - Elements.insert(Elements.begin() + (EI + 4), - Log); - EI += 4; + Added Log = logPrint(isDebugPrint, + objectPV.second, + appendNewlinePV.second, + AE->getSourceRange()); + if (*Log) { + Elements[EI] = objectPV.first; + Elements.insert(Elements.begin() + (EI + 1), + objectPV.second); + Elements.insert(Elements.begin() + (EI + 2), + appendNewlinePV.first); + Elements.insert(Elements.begin() + (EI + 3), + appendNewlinePV.second); + Elements.insert(Elements.begin() + (EI + 4), + *Log); + EI += 4; + } } Handled = true; } @@ -570,22 +612,22 @@ class Instrumenter { Context.TheEmptyTupleType) { if (auto *DSCE = dyn_cast(AE->getFn())) { Expr *TargetExpr = DSCE->getArg(); - Expr *Target_RE = nullptr; + Added Target_RE; ValueDecl *TargetVD = nullptr; std::tie(Target_RE, TargetVD) = digForVariable(TargetExpr); if (TargetVD) { - Expr *Log = logDeclOrMemberRef(Target_RE); - if (Log) { - Elements.insert(Elements.begin() + (EI + 1), Log); + Added Log = logDeclOrMemberRef(Target_RE); + if (*Log) { + Elements.insert(Elements.begin() + (EI + 1), *Log); ++EI; } } } else if (DeclRefExpr *DRE = digForInoutDeclRef(AE->getArg())) { - Expr *Log = logDeclOrMemberRef(DRE); - if (Log) { - Elements.insert(Elements.begin() + (EI + 1), Log); + Added Log = logDeclOrMemberRef(DRE); + if (*Log) { + Elements.insert(Elements.begin() + (EI + 1), *Log); ++EI; } } @@ -595,17 +637,19 @@ class Instrumenter { // do the same as for all other expressions std::pair PV = buildPatternAndVariable(E); - Expr *Log = buildLoggerCall( + Added Log = buildLoggerCall( new (Context) DeclRefExpr(ConcreteDeclRef(PV.second), SourceLoc(), true, // implicit AccessSemantics::Ordinary, E->getType()), E->getSourceRange(), ""); - Elements[EI] = PV.first; - Elements.insert(Elements.begin() + (EI + 1), PV.second); - Elements.insert(Elements.begin() + (EI + 2), Log); - EI += 2; + if (*Log) { + Elements[EI] = PV.first; + Elements.insert(Elements.begin() + (EI + 1), PV.second); + Elements.insert(Elements.begin() + (EI + 2), *Log); + EI += 2; + } } } else { @@ -613,17 +657,19 @@ class Instrumenter { Context.TheEmptyTupleType) { std::pair PV = buildPatternAndVariable(E); - Expr *Log = buildLoggerCall( + Added Log = buildLoggerCall( new (Context) DeclRefExpr(ConcreteDeclRef(PV.second), SourceLoc(), true, // implicit AccessSemantics::Ordinary, E->getType()), E->getSourceRange(), ""); - Elements[EI] = PV.first; - Elements.insert(Elements.begin() + (EI + 1), PV.second); - Elements.insert(Elements.begin() + (EI + 2), Log); - EI += 2; + if (*Log) { + Elements[EI] = PV.first; + Elements.insert(Elements.begin() + (EI + 1), PV.second); + Elements.insert(Elements.begin() + (EI + 2), *Log); + EI += 2; + } } } } else if (Stmt *S = Element.dyn_cast()) { @@ -641,18 +687,20 @@ class Instrumenter { ReturnStmt *NRS = new (Context) ReturnStmt(SourceLoc(), DRE, true); // implicit - Expr *Log = buildLoggerCall( + Added Log = buildLoggerCall( new (Context) DeclRefExpr(ConcreteDeclRef(PV.second), SourceLoc(), true, // implicit AccessSemantics::Ordinary, RS->getResult()->getType()), RS->getResult()->getSourceRange(), ""); - Elements[EI] = PV.first; - Elements.insert(Elements.begin() + (EI + 1), PV.second); - Elements.insert(Elements.begin() + (EI + 2), Log); - Elements.insert(Elements.begin() + (EI + 3), NRS); - EI += 3; + if (*Log) { + Elements[EI] = PV.first; + Elements.insert(Elements.begin() + (EI + 1), PV.second); + Elements.insert(Elements.begin() + (EI + 2), *Log); + Elements.insert(Elements.begin() + (EI + 3), NRS); + EI += 3; + } } EI = escapeToTarget(BracePair::TargetKinds::Return, Elements, EI); } else { @@ -672,8 +720,9 @@ class Instrumenter { if (auto *PBD = dyn_cast(D)) { if (VarDecl *VD = PBD->getSingleVar()) { if (VD->getParentInitializer()) { - if (Expr *Log = logVarDecl(VD)) { - Elements.insert(Elements.begin() + (EI + 1), Log); + Added Log = logVarDecl(VD); + if (*Log) { + Elements.insert(Elements.begin() + (EI + 1), *Log); ++EI; } } @@ -685,8 +734,8 @@ class Instrumenter { } if (!TopLevel && !HighPerformance) { - Elements.insert(Elements.begin(), buildScopeEntry(BS->getSourceRange())); - Elements.insert(Elements.end(), buildScopeExit(BS->getSourceRange())); + Elements.insert(Elements.begin(), *buildScopeEntry(BS->getSourceRange())); + Elements.insert(Elements.end(), *buildScopeExit(BS->getSourceRange())); } // Remove null elements from the list. @@ -707,7 +756,7 @@ class Instrumenter { // log*() functions return a newly-created log expression to be inserted // after or instead of the expression they're looking at. Only call this // if the variable has an initializer. - Expr *logVarDecl(VarDecl *VD) { + Added logVarDecl(VarDecl *VD) { if (isa(TypeCheckDC) && VD->getNameStr().equals("self")) { // Don't log "self" in a constructor return nullptr; @@ -722,8 +771,8 @@ class Instrumenter { VD->getSourceRange(), VD->getName().str().str().c_str()); } - Expr *logDeclOrMemberRef(Expr *RE) { - if (DeclRefExpr *DRE = dyn_cast(RE)) { + Added logDeclOrMemberRef(Added RE) { + if (DeclRefExpr *DRE = dyn_cast(*RE)) { VarDecl *VD = cast(DRE->getDecl()); if (isa(TypeCheckDC) && VD->getNameStr().equals("self")){ @@ -738,7 +787,7 @@ class Instrumenter { AccessSemantics::Ordinary, Type()), DRE->getSourceRange(), VD->getName().str().str().c_str()); - } else if (MemberRefExpr *MRE = dyn_cast(RE)) { + } else if (MemberRefExpr *MRE = dyn_cast(*RE)) { Expr *B = MRE->getBase(); ConcreteDeclRef M = MRE->getMember(); @@ -761,8 +810,8 @@ class Instrumenter { } } - Expr *logPrint(bool isDebugPrint, VarDecl *object, VarDecl *appendNewline, - SourceRange SR) { + Added logPrint(bool isDebugPrint, VarDecl *object, + VarDecl *appendNewline, SourceRange SR) { const char *LoggerName = isDebugPrint ? "$builtin_debugPrint" : "$builtin_print"; DeclRefExpr *object_DRE = @@ -817,7 +866,8 @@ class Instrumenter { return std::make_pair(PBD, VD); } - Expr *buildLoggerCall(Expr *E, SourceRange SR, const char *Name) { + Added buildLoggerCall(Added E, SourceRange SR, + const char *Name) { assert(Name); std::string *NameInContext = Context.AllocateObjectCopy(std::string(Name)); @@ -831,10 +881,10 @@ class Instrumenter { const unsigned int id_num = Distribution(RNG); ::snprintf(id_buf, buf_size, "%u", id_num); Expr *IDExpr = new (Context) IntegerLiteralExpr(id_buf, - SR.End, true); + SourceLoc(), true); Expr *LoggerArgExprs[] = { - E, + *E, NameExpr, IDExpr }; @@ -844,15 +894,15 @@ class Instrumenter { SR); } - Expr *buildScopeEntry(SourceRange SR) { + Added buildScopeEntry(SourceRange SR) { return buildScopeCall(SR, false); } - Expr *buildScopeExit(SourceRange SR) { + Added buildScopeExit(SourceRange SR) { return buildScopeCall(SR, true); } - Expr *buildScopeCall(SourceRange SR, bool IsExit) { + Added buildScopeCall(SourceRange SR, bool IsExit) { const char *LoggerName = IsExit ? "$builtin_log_scope_exit" : "$builtin_log_scope_entry"; @@ -861,9 +911,9 @@ class Instrumenter { SR); } - Expr *buildLoggerCallWithArgs(const char *LoggerName, - MutableArrayRef Args, - SourceRange SR) { + Added buildLoggerCallWithArgs(const char *LoggerName, + MutableArrayRef Args, + SourceRange SR) { Expr *LoggerArgs = nullptr; if (Args.size() == 1) { @@ -932,8 +982,9 @@ class Instrumenter { SendDataRef->setImplicit(true); - Expr *SendDataCall = new (Context) CallExpr(SendDataRef, SendDataArgs, true, - Type()); + Added SendDataCall = new (Context) CallExpr(SendDataRef, + SendDataArgs, true, + Type()); if (!doTypeCheck(Context, TypeCheckDC, SendDataCall)) { return nullptr; diff --git a/test/PlaygroundTransform/array_did_set.swift b/test/PlaygroundTransform/array_did_set.swift new file mode 100644 index 0000000000000..82eac22413514 --- /dev/null +++ b/test/PlaygroundTransform/array_did_set.swift @@ -0,0 +1,26 @@ +// RUN: rm -rf %t && mkdir %t +// RUN: cp %s %t/main.swift +// RUN: %target-build-swift -Xfrontend -playground -Xfrontend -debugger-support -o %t/main %S/Inputs/PlaygroundsRuntime.swift %t/main.swift +// RUN: %target-run %t/main | FileCheck %s +// REQUIRES: executable_test +struct S { + var a : [Int] = [] { + didSet { + print("Set") + } + } +} + +var s = S() +s.a = [3,2] +s.a.append(300) + +// CHECK: [{{.*}}] $builtin_log[s='main.S(a: [])'] +// CHECK-NEXT: [{{.*}}] $builtin_log_scope_entry +// CHECK-NEXT: [{{.*}}] $builtin_print['Set'] +// CHECK-NEXT: [{{.*}}] $builtin_log_scope_exit +// CHECK-NEXT: [{{.*}}] $builtin_log[s='main.S(a: [3, 2])'] +// CHECK-NEXT: [{{.*}}] $builtin_log_scope_entry +// CHECK-NEXT: [{{.*}}] $builtin_print['Set'] +// CHECK-NEXT: [{{.*}}] $builtin_log_scope_exit +// CHECK-NEXT: [{{.*}}] $builtin_log[a='[3, 2, 300]'] diff --git a/test/PlaygroundTransform/array_in_struct.swift b/test/PlaygroundTransform/array_in_struct.swift new file mode 100644 index 0000000000000..bc8c63def9a2c --- /dev/null +++ b/test/PlaygroundTransform/array_in_struct.swift @@ -0,0 +1,17 @@ +// RUN: rm -rf %t && mkdir %t +// RUN: cp %s %t/main.swift +// RUN: %target-build-swift -Xfrontend -playground -Xfrontend -debugger-support -o %t/main %S/Inputs/PlaygroundsRuntime.swift %t/main.swift +// RUN: %target-run %t/main | FileCheck %s +// REQUIRES: executable_test + +// FIXME: We need to instrument mutations of objects that are accessed in +// arbitrary ways, not just successive member references. +// XFAIL: * + +struct S { + var a = [Int]() +} +var s = [S()] +s[0].a.append(3) +// CHECK: [{{.*}}] $builtin_log[s='[main.S(a: [])]'] +// CHECK-NEXT: [{{.*}}] $builtin_log[a='[3])]'] diff --git a/test/PlaygroundTransform/print_inout.swift b/test/PlaygroundTransform/print_inout.swift new file mode 100644 index 0000000000000..b6366fa240b4a --- /dev/null +++ b/test/PlaygroundTransform/print_inout.swift @@ -0,0 +1,13 @@ +// RUN: rm -rf %t && mkdir %t +// RUN: cp %s %t/main.swift +// RUN: %target-build-swift -Xfrontend -playground -Xfrontend -debugger-support -o %t/main %S/Inputs/PlaygroundsRuntime.swift %t/main.swift +// RUN: %target-run %t/main | FileCheck %s +// REQUIRES: executable_test + +// FIXME: deal with inout arguments to print +// XFAIL: * + +var s = "a" +print("b", &s) +// CHECK: [{{.*}}] $builtin_log[s='a'] +// CHECK-NEXT: [{{.*}}] $builtin_print['a']