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.

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

[NFC] [PrintAsClang] Add tiebreaking rule to sort #74597

Merged
merged 1 commit into from
Jun 22, 2024
Merged

[NFC] [PrintAsClang] Add tiebreaking rule to sort #74597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
92 changes: 74 additions & 18 deletions lib/PrintAsClang/ModuleContentsWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,20 +680,21 @@ class ModuleWriter {
// Sort top level functions with the same C++ name by their location to
// have stable sorting that depends on users source but not on the
// compiler invocation.
// FIXME: This is pretty suspect; PrintAsClang sometimes operates on
// serialized modules which don't have SourceLocs, so this sort
// rule may be applied in some steps of a build but not others.
if ((*rhs)->getLoc().isValid() && (*lhs)->getLoc().isValid()) {
std::string rhsLoc, lhsLoc;
auto getLocText = [](const AbstractFunctionDecl *afd) {
auto getLocText = [](const Decl *afd) {
std::string res;
llvm::raw_string_ostream os(res);
afd->getLoc().print(os, afd->getASTContext().SourceMgr);
return std::move(os.str());
};
if (getLocText(cast<AbstractFunctionDecl>(*lhs)) <
getLocText(cast<AbstractFunctionDecl>(*rhs)))
return Descending;
return Ascending;

result = getLocText(*rhs).compare(getLocText(*lhs));
if (result != 0)
return result;
}
return result;
}

// A function and a global variable can have the same name in C++,
Expand All @@ -705,12 +706,35 @@ class ModuleWriter {
return -1;

// Prefer value decls to extensions.
assert(!(isa<ValueDecl>(*lhs) && isa<ValueDecl>(*rhs)));
if (isa<ValueDecl>(*lhs) && !isa<ValueDecl>(*rhs))
return Descending;
if (!isa<ValueDecl>(*lhs) && isa<ValueDecl>(*rhs))
return Ascending;

// Last-ditch ValueDecl tiebreaker: Compare mangled names. This captures
// *tons* of context and detail missed by the previous checks, but the
// resulting sort makes little sense to humans.
// FIXME: It'd be nice to share the mangler or even memoize mangled names,
// but we'd have to stop using `llvm::array_pod_sort()` so that we
// could capture some outside state.
Mangle::ASTMangler mangler;
auto getMangledName = [&](const Decl *D) {
auto VD = dyn_cast<ValueDecl>(D);
if (!VD && isa<ExtensionDecl>(D))
VD = cast<ExtensionDecl>(D)->getExtendedNominal();
if (!VD)
return std::string();
return mangler.mangleAnyDecl(VD, /*prefix=*/true,
/*respectOriginallyDefinedIn=*/true);
};
result = getMangledName(*rhs).compare(getMangledName(*lhs));
if (result != 0)
return result;

// Mangled names ought to distinguish all value decls, leaving only
// extensions of the same nominal type beyond this point.
assert(isa<ExtensionDecl>(*lhs) && isa<ExtensionDecl>(*rhs));

// Break ties in extensions by putting smaller extensions last (in reverse
// order).
// FIXME: This will end up taking linear time.
Expand All @@ -731,16 +755,48 @@ class ModuleWriter {

// If that fails, arbitrarily pick the extension whose protocols are
// alphabetically first.
auto mismatch =
std::mismatch(lhsProtos.begin(), lhsProtos.end(), rhsProtos.begin(),
[] (const ProtocolDecl *nextLHSProto,
const ProtocolDecl *nextRHSProto) {
return nextLHSProto->getName() != nextRHSProto->getName();
});
if (mismatch.first == lhsProtos.end())
return Equivalent;
StringRef lhsProtoName = (*mismatch.first)->getName().str();
return lhsProtoName.compare((*mismatch.second)->getName().str());
{
auto mismatch =
std::mismatch(lhsProtos.begin(), lhsProtos.end(), rhsProtos.begin(),
[] (const ProtocolDecl *nextLHSProto,
const ProtocolDecl *nextRHSProto) {
return nextLHSProto->getName() != nextRHSProto->getName();
});
if (mismatch.first != lhsProtos.end()) {
StringRef lhsProtoName = (*mismatch.first)->getName().str();
return lhsProtoName.compare((*mismatch.second)->getName().str());
}
}

// Still nothing? Fine, we'll pick the one with the alphabetically first
// member instead.
{
auto mismatch =
std::mismatch(cast<ExtensionDecl>(*lhs)->getMembers().begin(),
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about std::mismatch

Copy link
Contributor Author

@beccadax beccadax Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was news to me too!

cast<ExtensionDecl>(*lhs)->getMembers().end(),
cast<ExtensionDecl>(*rhs)->getMembers().begin(),
[] (const Decl *nextLHSDecl, const Decl *nextRHSDecl) {
if (isa<ValueDecl>(nextLHSDecl) && isa<ValueDecl>(nextRHSDecl)) {
return cast<ValueDecl>(nextLHSDecl)->getName() !=
cast<ValueDecl>(nextRHSDecl)->getName();
}
return isa<ValueDecl>(nextLHSDecl) != isa<ValueDecl>(nextRHSDecl);
});
if (mismatch.first != cast<ExtensionDecl>(*lhs)->getMembers().end()) {
auto *lhsMember = dyn_cast<ValueDecl>(*mismatch.first),
*rhsMember = dyn_cast<ValueDecl>(*mismatch.second);
if (!rhsMember && lhsMember)
return Descending;
if (lhsMember && !rhsMember)
return Ascending;
if (lhsMember && rhsMember)
return rhsMember->getName().compare(lhsMember->getName());
}
}

// Hopefully two extensions with identical conformances and member names
// will be interchangeable enough not to matter.
return Equivalent;
});

assert(declsToWrite.empty());
Expand Down
26 changes: 24 additions & 2 deletions test/PrintAsObjC/classes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

// RUN: %target-swift-frontend(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t) -emit-module -I %S/Inputs/custom-modules -o %t %s -disable-objc-attr-requires-foundation-module
// RUN: %target-swift-frontend(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t) -parse-as-library %t/classes.swiftmodule -typecheck -I %S/Inputs/custom-modules -emit-objc-header-path %t/classes.h -import-objc-header %S/../Inputs/empty.h -disable-objc-attr-requires-foundation-module
// RUN: %FileCheck %s < %t/classes.h
// RUN: %FileCheck --check-prefix=NEGATIVE %s < %t/classes.h
// RUN: %FileCheck %s --input-file %t/classes.h
// RUN: %FileCheck --check-prefix=NEGATIVE %s --input-file %t/classes.h
// RUN: %check-in-clang -I %S/Inputs/custom-modules/ %t/classes.h
// RUN: not %check-in-clang -I %S/Inputs/custom-modules/ -fno-modules -Qunused-arguments %t/classes.h
// RUN: %check-in-clang -I %S/Inputs/custom-modules/ -fno-modules -Qunused-arguments %t/classes.h -include CoreFoundation.h -include objc_generics.h -include SingleGenericClass.h -include CompatibilityAlias.h
Expand Down Expand Up @@ -427,6 +427,28 @@ class MyObject : NSObject {}
class ImplicitObjCInner : A1 {}
}

// CHECK-LABEL: @interface NestedCollision1Identical
// CHECK-NEXT: - (void)before
// CHECK-NEXT: init
// CHECK-NEXT: @end
// CHECK: @interface NestedCollision2Identical
// CHECK-NEXT: - (void)after
// CHECK-NEXT: init
// CHECK-NEXT: @end

// We're intentionally declaring NestedCollision2 before NestedCollision1 to
// make sure they're being sorted based on their names, not their source order.
@objc @objcMembers class NestedCollision2 {
@objc(NestedCollision2Identical) @objcMembers class Identical: NSObject {
@objc func after() {}
}
}
@objc @objcMembers class NestedCollision1 {
@objc(NestedCollision1Identical) @objcMembers class Identical: NSObject {
@objc func before() {}
}
}

// CHECK-LABEL: @class Inner2;
// CHECK-LABEL: @interface NestedMembers
// CHECK-NEXT: @property (nonatomic, strong) Inner2 * _Nullable ref2;
Expand Down
21 changes: 21 additions & 0 deletions test/PrintAsObjC/extensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,27 @@ extension A5 {
var notObjC: NotObjC { return NotObjC() }
}

// Check that two otherwise tied extensions will print in alphabetical
// order by first member with a differing Swift name.

// CHECK-LABEL: @interface A6
@objc class A6 {}

extension A6 {
@objc(skippedBool:) func skipped(_: Bool) {}
@objc func def() {}
}
extension A6 {
@objc(skippedInt:) func skipped(_: Int) {}
@objc func abc() {}
}
// CHECK: @interface A6 (SWIFT_EXTENSION(extensions))
// CHECK: - (void)skippedInt:
// CHECK: - (void)abc
// CHECK: @interface A6 (SWIFT_EXTENSION(extensions))
// CHECK: - (void)skippedBool:
// CHECK: - (void)def

// CHECK-LABEL: @interface CustomName{{$}}
// CHECK-NEXT: init
// CHECK-NEXT: @end
Expand Down