Skip to content

swift-module-digester: diagnose adding/removing protocol conformances as API breakages. #19427

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

Merged
merged 2 commits into from
Sep 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsModuleDiffer.def
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ ERROR(decl_added,none,"%0 is added to a non-resilient type", (StringRef))

ERROR(default_arg_removed,none,"%0 has removed default argument from %1", (StringRef, StringRef))

ERROR(conformance_removed,none,"%0 has removed %select{conformance to|inherited protocol}2 %1", (StringRef, StringRef, bool))

ERROR(conformance_added,none,"%0 has added inherited protocol %1", (StringRef, StringRef))

#ifndef DIAG_NO_UNDEF
# if defined(DIAG)
# undef DIAG
Expand Down
2 changes: 2 additions & 0 deletions test/api-digester/Inputs/cake.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,5 @@ public struct fixedLayoutStruct {
private var b = 2
var c = 3
}

extension Int: P1 { public func bar() {} }
6 changes: 5 additions & 1 deletion test/api-digester/Inputs/cake1.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,8 @@ public enum FrozenKind {

public class C7 {
public func foo(_ a: Int = 1, _ b: Int = 2) {}
}
}

public protocol P3: P2, P1 {}

extension fixedLayoutStruct: P1 {}
10 changes: 8 additions & 2 deletions test/api-digester/Inputs/cake2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ public enum FrozenKind {
case AddedCase
}

public class C7 {
public class C7: P1 {
public func foo(_ a: Int, _ b: Int) {}
}
}

public protocol P3: P1, P4 {}

public protocol P4 {}

extension fixedLayoutStruct: P2 {}
8 changes: 7 additions & 1 deletion test/api-digester/Outputs/Cake-abi.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@

/* Generic Signature Changes */
cake1: Func P1.P1Constraint() has generic signature change from <τ_0_0 where τ_0_0 : P1, τ_0_0 : P2> to <τ_0_0 where τ_0_0 : P1>
cake1: Protocol P3 has generic signature change from <τ_0_0 : P1, τ_0_0 : P2> to <τ_0_0 : P1, τ_0_0 : P4>

/* RawRepresentable Changes */

Expand Down Expand Up @@ -40,4 +41,9 @@ cake1: Var fixedLayoutStruct.a in a non-resilient type changes position from 1 t
cake1: Var fixedLayoutStruct.b in a non-resilient type changes position from 0 to 1
cake2: EnumElement FrozenKind.AddedCase is added to a non-resilient type
cake2: Var fixedLayoutStruct.c is added to a non-resilient type
cake2: Var fixedLayoutStruct.lazy_d.storage is added to a non-resilient type
cake2: Var fixedLayoutStruct.lazy_d.storage is added to a non-resilient type

/* Conformance changes */
cake1: Protocol P3 has added inherited protocol P4
cake1: Protocol P3 has removed inherited protocol P2
cake1: Struct fixedLayoutStruct has removed conformance to P1
6 changes: 6 additions & 0 deletions test/api-digester/Outputs/Cake.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@

/* Generic Signature Changes */
cake1: Func P1.P1Constraint() has generic signature change from <Self where Self : P1, Self : P2> to <Self where Self : P1>
cake1: Protocol P3 has generic signature change from <Self : P1, Self : P2> to <Self : P1, Self : P4>

/* RawRepresentable Changes */

Expand All @@ -27,3 +28,8 @@ cake1: Func S1.foo1() is now mutating
cake1: Func S1.foo3() is now static
cake1: Var C1.CIIns1 changes from weak to strong
cake1: Var C1.CIIns2 changes from strong to weak

/* Conformance changes */
cake1: Protocol P3 has added inherited protocol P4
cake1: Protocol P3 has removed inherited protocol P2
cake1: Struct fixedLayoutStruct has removed conformance to P1
17 changes: 17 additions & 0 deletions test/api-digester/Outputs/cake-abi.json
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@
"Strideable",
"ExpressibleByIntegerLiteral",
"FixedWidthInteger",
"P1",
"Encodable",
"Decodable",
"Hashable",
Expand Down Expand Up @@ -1012,6 +1013,22 @@
"printedName": "()"
}
]
},
{
"kind": "Function",
"name": "bar",
"printedName": "bar()",
"declKind": "Func",
"usr": "s:Si4cakeE3baryyF",
"location": "",
"moduleName": "cake",
"children": [
{
"kind": "TypeNominal",
"name": "Void",
"printedName": "()"
}
]
}
]
}
Expand Down
17 changes: 17 additions & 0 deletions test/api-digester/Outputs/cake.json
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,7 @@
"Strideable",
"ExpressibleByIntegerLiteral",
"FixedWidthInteger",
"P1",
"Encodable",
"Decodable",
"Hashable",
Expand Down Expand Up @@ -888,6 +889,22 @@
"printedName": "()"
}
]
},
{
"kind": "Function",
"name": "bar",
"printedName": "bar()",
"declKind": "Func",
"usr": "s:Si4cakeE3baryyF",
"location": "",
"moduleName": "cake",
"children": [
{
"kind": "TypeNominal",
"name": "Void",
"printedName": "()"
}
]
}
]
}
Expand Down
56 changes: 46 additions & 10 deletions tools/swift-api-digester/ModuleAnalyzerNodes.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <ModuleAnalyzerNodes.h>
#include <algorithm>

using namespace swift;
using namespace ide;
Expand Down Expand Up @@ -642,6 +643,25 @@ bool SDKNode::hasSameChildren(const SDKNode &Other) const {
return true;
}

void swift::ide::api::stringSetDifference(ArrayRef<StringRef> Left,
ArrayRef<StringRef> Right,
std::vector<StringRef> &LeftMinusRight,
std::vector<StringRef> &RightMinusLeft) {
std::set<StringRef> LS(Left.begin(), Left.end());
std::set<StringRef> RS(Right.begin(), Right.end());
std::set_difference(LS.begin(), LS.end(), RS.begin(), RS.end(),
std::back_inserter(LeftMinusRight));
std::set_difference(RS.begin(), RS.end(), LS.begin(), LS.end(),
std::back_inserter(RightMinusLeft));
}

static bool hasSameContents(ArrayRef<StringRef> Left,
ArrayRef<StringRef> Right) {
std::vector<StringRef> LeftMinusRight, RightMinusLeft;
stringSetDifference(Left, Right, LeftMinusRight, RightMinusLeft);
return LeftMinusRight.empty() && RightMinusLeft.empty();
}

bool SDKNode::operator==(const SDKNode &Other) const {
auto *LeftAlias = dyn_cast<SDKNodeTypeAlias>(this);
auto *RightAlias = dyn_cast<SDKNodeTypeAlias>(&Other);
Expand Down Expand Up @@ -701,7 +721,16 @@ bool SDKNode::operator==(const SDKNode &Other) const {
}
LLVM_FALLTHROUGH;
}
case SDKNodeKind::DeclType:
case SDKNodeKind::DeclType: {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I'm adding a conformance in an extension, and the extended type is from a different module? You really should model conformances as top-level entities, and not as part of types. Then adding a new conformance is fine (just like adding a new top-level function), but removing one is not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the extension is for a type from a different module, the tool will synthesize the type definition in its module dump (here). So the standalone extension won't be missed.

auto *Left = dyn_cast<SDKNodeDeclType>(this);
auto *Right = dyn_cast<SDKNodeDeclType>(&Other);
if (Left && Right) {
if (!hasSameContents(Left->getAllProtocols(), Right->getAllProtocols())) {
return false;
}
}
LLVM_FALLTHROUGH;
}
case SDKNodeKind::DeclTypeAlias: {
auto Left = this->getAs<SDKNodeDecl>();
auto Right = (&Other)->getAs<SDKNodeDecl>();
Expand Down Expand Up @@ -1187,13 +1216,16 @@ static SDKNode *constructTypeDeclNode(SDKContext &Ctx, NominalTypeDecl *NTD,
/// synthesize this type node to include those extension members, since these
/// extension members are legit members of the module.
static SDKNode *constructExternalExtensionNode(SDKContext &Ctx, SDKNode *Root,
ExtensionDecl *Ext,
NominalTypeDecl *NTD,
ArrayRef<ExtensionDecl*> AllExts,
std::set<ExtensionDecl*> &HandledExts) {
auto *TypeNode = SDKNodeInitInfo(Ctx, Ext->getSelfNominalTypeDecl())
.createSDKNode(SDKNodeKind::DeclType);
auto *TypeNode = SDKNodeInitInfo(Ctx, NTD).createSDKNode(SDKNodeKind::DeclType);

// The members of the extension are the only members of this synthesized type.
addMembersToRoot(Ctx, TypeNode, Ext, HandledExts);
// The members of the extensions are the only members of this synthesized type.
for (auto *Ext: AllExts) {
HandledExts.insert(Ext);
addMembersToRoot(Ctx, TypeNode, Ext, HandledExts);
}
return TypeNode;
}

Expand Down Expand Up @@ -1273,16 +1305,20 @@ void SwiftDeclCollector::lookupVisibleDecls(ArrayRef<ModuleDecl *> Modules) {
for (auto *VD : ClangMacros)
processDecl(VD);

// For all known decls, collect those unhandled extensions and handle them
// separately.
// Collect extensions to types from other modules and synthesize type nodes
// for them.
llvm::MapVector<NominalTypeDecl*, llvm::SmallVector<ExtensionDecl*, 4>> ExtensionMap;
for (auto *D: KnownDecls) {
if (auto *Ext = dyn_cast<ExtensionDecl>(D)) {
if (HandledExtensions.find(Ext) == HandledExtensions.end()) {
RootNode->addChild(constructExternalExtensionNode(Ctx, RootNode, Ext,
HandledExtensions));
ExtensionMap[Ext->getExtendedNominal()].push_back(Ext);
}
}
}
for (auto Pair: ExtensionMap) {
RootNode->addChild(constructExternalExtensionNode(Ctx, RootNode,
Pair.first, Pair.second, HandledExtensions));
}
}

void SwiftDeclCollector::processDecl(ValueDecl *VD) {
Expand Down
5 changes: 4 additions & 1 deletion tools/swift-api-digester/ModuleAnalyzerNodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ class SwiftDeclCollector: public VisibleDeclConsumer {
SDKContext &Ctx;
std::vector<std::unique_ptr<llvm::MemoryBuffer>> OwnedBuffers;
SDKNode *RootNode;
llvm::DenseSet<Decl*> KnownDecls;
llvm::SetVector<Decl*> KnownDecls;
// Collected and sorted after we get all of them.
std::vector<ValueDecl *> ClangMacros;
std::set<ExtensionDecl*> HandledExtensions;
Expand Down Expand Up @@ -543,6 +543,9 @@ int deserializeSDKDump(StringRef dumpPath, StringRef OutputPath,

int findDeclUsr(StringRef dumpPath, CheckerOptions Opts);

void stringSetDifference(ArrayRef<StringRef> Left, ArrayRef<StringRef> Right,
std::vector<StringRef> &LeftMinusRight, std::vector<StringRef> &RightMinusLeft);

} // end of abi namespace
} // end of ide namespace
} // end of Swift namespace
Expand Down
3 changes: 3 additions & 0 deletions tools/swift-api-digester/ModuleDiagsConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ static StringRef getCategoryName(uint32_t ID) {
case LocalDiagID::decl_added:
case LocalDiagID::decl_reorder:
return "/* Fixed-layout Type Changes */";
case LocalDiagID::conformance_added:
case LocalDiagID::conformance_removed:
return "/* Protocol Conformance Change */";
default:
return StringRef();
}
Expand Down
28 changes: 28 additions & 0 deletions tools/swift-api-digester/swift-api-digester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,28 @@ static bool isOwnershipEquivalent(ReferenceOwnership Left,
return false;
}

static void diagnoseNominalTypeDeclChange(SDKNodeDeclType *L, SDKNodeDeclType *R) {
auto &Ctx = L->getSDKContext();
auto &Diags = Ctx.getDiags();
std::vector<StringRef> LeftMinusRight;
std::vector<StringRef> RightMinusLeft;
swift::ide::api::stringSetDifference(L->getAllProtocols(), R->getAllProtocols(),
LeftMinusRight, RightMinusLeft);
bool isProtocol = L->getDeclKind() == DeclKind::Protocol;
std::for_each(LeftMinusRight.begin(), LeftMinusRight.end(), [&](StringRef Name) {
Diags.diagnose(SourceLoc(), diag::conformance_removed, L->getScreenInfo(), Name,
isProtocol);
});

// Adding inherited protocols can be API breaking.
if (isProtocol) {
std::for_each(RightMinusLeft.begin(), RightMinusLeft.end(), [&](StringRef Name) {
Diags.diagnose(SourceLoc(), diag::conformance_added, L->getScreenInfo(),
Name);
});
}
}

static void detectDeclChange(NodePtr L, NodePtr R, SDKContext &Ctx) {
assert(L->getKind() == R->getKind());
auto &Diags = Ctx.getDiags();
Expand Down Expand Up @@ -727,6 +749,12 @@ static void detectDeclChange(NodePtr L, NodePtr R, SDKContext &Ctx) {
Diags.diagnose(SourceLoc(), diag::generic_sig_change, LD->getScreenInfo(),
LD->getGenericSignature(), RD->getGenericSignature());
}

if (auto *LDT = dyn_cast<SDKNodeDeclType>(L)) {
if (auto *RDT = dyn_cast<SDKNodeDeclType>(R)) {
diagnoseNominalTypeDeclChange(LDT, RDT);
}
}
detectRename(L, R);
}
}
Expand Down