Skip to content

Allow @objc protocols to declare async alternatives #59479

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 5 commits into from
Jun 17, 2022
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
2 changes: 1 addition & 1 deletion include/swift/AST/Decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -6753,7 +6753,7 @@ class AbstractFunctionDecl : public GenericContext, public ValueDecl {
bool hasDynamicSelfResult() const;

/// The async function marked as the alternative to this function, if any.
AbstractFunctionDecl *getAsyncAlternative() const;
AbstractFunctionDecl *getAsyncAlternative(bool isKnownObjC = false) const;

/// If \p asyncAlternative is set, then compare its parameters to this
/// (presumed synchronous) function's parameters to find the index of the
Expand Down
8 changes: 4 additions & 4 deletions include/swift/AST/ModuleLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,11 @@ class ModuleLoader {
virtual void loadExtensions(NominalTypeDecl *nominal,
unsigned previousGeneration) { }

/// Load the methods within the given class that produce
/// Load the methods within the given type that produce
/// Objective-C class or instance methods with the given selector.
///
/// \param classDecl The class in which we are searching for @objc methods.
/// The search only considers this class and its extensions; not any
/// \param typeDecl The type in which we are searching for @objc methods.
/// The search only considers this type and its extensions; not any
/// superclasses.
///
/// \param selector The selector to search for.
Expand All @@ -246,7 +246,7 @@ class ModuleLoader {
/// selector and are instance/class methods as requested. This list will be
/// extended with any methods found in subsequent generations.
virtual void loadObjCMethods(
ClassDecl *classDecl,
NominalTypeDecl *typeDecl,
ObjCSelector selector,
bool isInstanceMethod,
unsigned previousGeneration,
Expand Down
42 changes: 39 additions & 3 deletions include/swift/AST/SourceFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "swift/AST/Import.h"
#include "swift/AST/SynthesizedFileUnit.h"
#include "swift/Basic/Debug.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallPtrSet.h"

Expand Down Expand Up @@ -243,11 +244,20 @@ class SourceFile final : public FileUnit {
std::vector<ObjCUnsatisfiedOptReq> ObjCUnsatisfiedOptReqs;

/// A selector that is used by two different declarations in the same class.
/// Fields: typeDecl, selector, isInstanceMethod.
using ObjCMethodConflict = std::tuple<NominalTypeDecl *, ObjCSelector, bool>;
struct ObjCMethodConflict {
NominalTypeDecl *typeDecl;
ObjCSelector selector;
bool isInstanceMethod;

ObjCMethodConflict(NominalTypeDecl *typeDecl, ObjCSelector selector,
bool isInstanceMethod)
: typeDecl(typeDecl), selector(selector),
isInstanceMethod(isInstanceMethod)
{}
};

/// List of Objective-C member conflicts we have found during type checking.
std::vector<ObjCMethodConflict> ObjCMethodConflicts;
llvm::SetVector<ObjCMethodConflict> ObjCMethodConflicts;

/// List of attributes added by access notes, used to emit remarks for valid
/// ones.
Expand Down Expand Up @@ -636,4 +646,30 @@ inline void simple_display(llvm::raw_ostream &out, const SourceFile *SF) {
}
} // end namespace swift

namespace llvm {

template<>
struct DenseMapInfo<swift::SourceFile::ObjCMethodConflict> {
using ObjCMethodConflict = swift::SourceFile::ObjCMethodConflict;

static inline ObjCMethodConflict getEmptyKey() {
return ObjCMethodConflict(nullptr, {}, false);
}
static inline ObjCMethodConflict getTombstoneKey() {
return ObjCMethodConflict(nullptr, {}, true);
}
static inline unsigned getHashValue(ObjCMethodConflict a) {
return hash_combine(hash_value(a.typeDecl),
DenseMapInfo<swift::ObjCSelector>::getHashValue(a.selector),
hash_value(a.isInstanceMethod));
}
static bool isEqual(ObjCMethodConflict a, ObjCMethodConflict b) {
return a.typeDecl == b.typeDecl && a.selector == b.selector &&
a.isInstanceMethod == b.isInstanceMethod;
}
};

}


#endif
5 changes: 3 additions & 2 deletions include/swift/AST/TypeCheckRequests.h
Original file line number Diff line number Diff line change
Expand Up @@ -3441,7 +3441,8 @@ class ConditionalRequirementsRequest

class RenamedDeclRequest
: public SimpleRequest<RenamedDeclRequest,
ValueDecl *(const ValueDecl *, const AvailableAttr *),
ValueDecl *(const ValueDecl *, const AvailableAttr *,
bool isKnownObjC),
RequestFlags::Cached> {
public:
using SimpleRequest::SimpleRequest;
Expand All @@ -3450,7 +3451,7 @@ class RenamedDeclRequest
friend SimpleRequest;

ValueDecl *evaluate(Evaluator &evaluator, const ValueDecl *attached,
const AvailableAttr *attr) const;
const AvailableAttr *attr, bool isKnownObjC) const;

public:
bool isCached() const { return true; }
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/TypeCheckerTypeIDZone.def
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ SWIFT_REQUEST(TypeChecker, GetImplicitSendableRequest,
ProtocolConformance *(NominalTypeDecl *),
Cached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, RenamedDeclRequest,
ValueDecl *(const ValueDecl *),
ValueDecl *(const ValueDecl *, const AvailableAttr *, bool),
Cached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, ClosureEffectsRequest,
FunctionType::ExtInfo(ClosureExpr *),
Expand Down
2 changes: 1 addition & 1 deletion include/swift/ClangImporter/ClangImporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class ClangImporter final : public ClangModuleLoader {
unsigned previousGeneration) override;

virtual void loadObjCMethods(
ClassDecl *classDecl,
NominalTypeDecl *typeDecl,
ObjCSelector selector,
bool isInstanceMethod,
unsigned previousGeneration,
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Sema/SourceLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class SourceLoader : public ModuleLoader {
unsigned previousGeneration) override;

virtual void loadObjCMethods(
ClassDecl *classDecl,
NominalTypeDecl *typeDecl,
ObjCSelector selector,
bool isInstanceMethod,
unsigned previousGeneration,
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Serialization/SerializedModuleLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ class SerializedModuleLoaderBase : public ModuleLoader {
unsigned previousGeneration) override;

virtual void loadObjCMethods(
ClassDecl *classDecl,
NominalTypeDecl *typeDecl,
ObjCSelector selector,
bool isInstanceMethod,
unsigned previousGeneration,
Expand Down
14 changes: 1 addition & 13 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1919,24 +1919,12 @@ void ASTContext::loadObjCMethods(
PrettyStackTraceSelector stackTraceSelector("looking for", selector);
PrettyStackTraceDecl stackTraceDecl("...in", tyDecl);

// @objc protocols cannot have @objc extension members, so if we've recorded
// everything in the protocol definition, we've recorded everything. And we
// only ever use the ObjCSelector version of `NominalTypeDecl::lookupDirect()`
// on protocols in primary file typechecking, so we don't care about protocols
// that need to be loaded from files.
// TODO: Rework `ModuleLoader::loadObjCMethods()` to support protocols too if
// selector-based `NominalTypeDecl::lookupDirect()` ever needs to work
// in more situations.
ClassDecl *classDecl = dyn_cast<ClassDecl>(tyDecl);
if (!classDecl)
return;

for (auto &loader : getImpl().ModuleLoaders) {
// Ignore the Clang importer if we've been asked for Swift-only results.
if (swiftOnly && loader.get() == getClangModuleLoader())
continue;

loader->loadObjCMethods(classDecl, selector, isInstanceMethod,
loader->loadObjCMethods(tyDecl, selector, isInstanceMethod,
previousGeneration, methods);
}
}
Expand Down
6 changes: 4 additions & 2 deletions lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7627,7 +7627,8 @@ bool AbstractFunctionDecl::hasDynamicSelfResult() const {
return isa<ConstructorDecl>(this);
}

AbstractFunctionDecl *AbstractFunctionDecl::getAsyncAlternative() const {
AbstractFunctionDecl *
AbstractFunctionDecl::getAsyncAlternative(bool isKnownObjC) const {
// Async functions can't have async alternatives
if (hasAsync())
return nullptr;
Expand All @@ -7651,7 +7652,8 @@ AbstractFunctionDecl *AbstractFunctionDecl::getAsyncAlternative() const {
}

auto *renamedDecl = evaluateOrDefault(
getASTContext().evaluator, RenamedDeclRequest{this, avAttr}, nullptr);
getASTContext().evaluator, RenamedDeclRequest{this, avAttr, isKnownObjC},
nullptr);
auto *alternative = dyn_cast_or_null<AbstractFunctionDecl>(renamedDecl);
if (!alternative || !alternative->hasAsync())
return nullptr;
Expand Down
38 changes: 32 additions & 6 deletions lib/AST/NameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1310,7 +1310,24 @@ class swift::ObjCMethodLookupTable
: public llvm::DenseMap<std::pair<ObjCSelector, char>,
StoredObjCMethods>,
public ASTAllocated<ObjCMethodLookupTable>
{};
{
SWIFT_DEBUG_DUMP {
llvm::errs() << "ObjCMethodLookupTable:\n";
for (auto pair : *this) {
auto selector = pair.getFirst().first;
auto isInstanceMethod = pair.getFirst().second;
auto &methods = pair.getSecond();

llvm::errs() << " \"" << (isInstanceMethod ? "-" : "+") << selector
<< "\":\n";
for (auto method : methods.Methods) {
llvm::errs() << " - \"";
method->dumpRef(llvm::errs());
llvm::errs() << "\"\n";
}
}
}
};

MemberLookupTable::MemberLookupTable(ASTContext &ctx) {
// Register a cleanup with the ASTContext to call the lookup table
Expand Down Expand Up @@ -1699,6 +1716,16 @@ NominalTypeDecl::lookupDirect(ObjCSelector selector, bool isInstance) {
return stored.Methods;
}

/// Is the new method an async alternative of any existing method, or vice
/// versa?
static bool isAnAsyncAlternative(AbstractFunctionDecl *newDecl,
llvm::TinyPtrVector<AbstractFunctionDecl *> &vec) {
return llvm::any_of(vec, [&](AbstractFunctionDecl *oldDecl) {
return newDecl->getAsyncAlternative(/*isKnownObjC=*/true) == oldDecl
|| oldDecl->getAsyncAlternative(/*isKnownObjC=*/true) == newDecl;
});
}

void NominalTypeDecl::recordObjCMethod(AbstractFunctionDecl *method,
ObjCSelector selector) {
if (!ObjCMethodLookup && !createObjCMethodLookup())
Expand All @@ -1714,12 +1741,11 @@ void NominalTypeDecl::recordObjCMethod(AbstractFunctionDecl *method,
return;

if (auto *sf = method->getParentSourceFile()) {
if (vec.size() == 1) {
// We have a conflict.
sf->ObjCMethodConflicts.push_back(std::make_tuple(this, selector,
isInstanceMethod));
} if (vec.empty()) {
if (vec.empty()) {
sf->ObjCMethodList.push_back(method);
} else if (!isa<ProtocolDecl>(this) || !isAnAsyncAlternative(method, vec)) {
// We have a conflict.
sf->ObjCMethodConflicts.insert({ this, selector, isInstanceMethod });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For those following along at home, I believe that the behavior change addressed in #59743 was caused by us now inserting conflicts into ObjCMethodConflicts when vec.size() >= 1 instead of only when vec.size() == 1. When a method had both completion handler and async variants, vec.size() would start off at 2 and there would be no way for a user-defined conflicting method to ever add an entry to ObjCMethodConflicts.

}
}

Expand Down
7 changes: 6 additions & 1 deletion lib/ClangImporter/ClangImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3569,11 +3569,16 @@ void ClangImporter::loadExtensions(NominalTypeDecl *nominal,
}

void ClangImporter::loadObjCMethods(
ClassDecl *classDecl,
NominalTypeDecl *typeDecl,
ObjCSelector selector,
bool isInstanceMethod,
unsigned previousGeneration,
llvm::TinyPtrVector<AbstractFunctionDecl *> &methods) {
// TODO: We don't currently need to load methods from imported ObjC protocols.
auto classDecl = dyn_cast<ClassDecl>(typeDecl);
if (!classDecl)
return;

const auto *objcClass =
dyn_cast_or_null<clang::ObjCInterfaceDecl>(classDecl->getClangDecl());
if (!objcClass)
Expand Down
24 changes: 22 additions & 2 deletions lib/PrintAsClang/DeclAndTypePrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,8 @@ class DeclAndTypePrinter::Implementation
assert(!AvAttr->Rename.empty());

auto *renamedDecl = evaluateOrDefault(
getASTContext().evaluator, RenamedDeclRequest{D, AvAttr}, nullptr);
getASTContext().evaluator, RenamedDeclRequest{D, AvAttr, false},
nullptr);
if (renamedDecl) {
assert(shouldInclude(renamedDecl) &&
"ObjC printer logic mismatch with renamed decl");
Expand Down Expand Up @@ -2112,12 +2113,31 @@ auto DeclAndTypePrinter::getImpl() -> Implementation {
return Implementation(os, *this, outputLang);
}

static bool isAsyncAlternativeOfOtherDecl(const ValueDecl *VD) {
auto AFD = dyn_cast<AbstractFunctionDecl>(VD);
if (!AFD || !AFD->isAsyncContext() || !AFD->getObjCSelector())
return false;

auto type = AFD->getDeclContext()->getSelfNominalTypeDecl();
if (!type)
return false;
auto others = type->lookupDirect(AFD->getObjCSelector(),
AFD->isInstanceMember());

for (auto other : others)
if (other->getAsyncAlternative() == AFD)
return true;

return false;
}

bool DeclAndTypePrinter::shouldInclude(const ValueDecl *VD) {
return !VD->isInvalid() &&
(outputLang == OutputLanguageMode::Cxx
? cxx_translation::isVisibleToCxx(VD, minRequiredAccess)
: isVisibleToObjC(VD, minRequiredAccess)) &&
!VD->getAttrs().hasAttribute<ImplementationOnlyAttr>();
!VD->getAttrs().hasAttribute<ImplementationOnlyAttr>() &&
!isAsyncAlternativeOfOtherDecl(VD);
}

void DeclAndTypePrinter::print(const Decl *D) {
Expand Down
5 changes: 3 additions & 2 deletions lib/Sema/TypeCheckAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6218,7 +6218,8 @@ static bool parametersMatch(const AbstractFunctionDecl *a,

ValueDecl *RenamedDeclRequest::evaluate(Evaluator &evaluator,
const ValueDecl *attached,
const AvailableAttr *attr) const {
const AvailableAttr *attr,
bool isKnownObjC) const {
if (!attached || !attr)
return nullptr;

Expand Down Expand Up @@ -6249,7 +6250,7 @@ ValueDecl *RenamedDeclRequest::evaluate(Evaluator &evaluator,
auto minAccess = AccessLevel::Private;
if (attached->getModuleContext()->isExternallyConsumed())
minAccess = AccessLevel::Public;
bool attachedIsObjcVisible =
bool attachedIsObjcVisible = isKnownObjC ||
objc_translation::isVisibleToObjC(attached, minAccess);

SmallVector<ValueDecl *, 4> lookupResults;
Expand Down
17 changes: 6 additions & 11 deletions lib/Sema/TypeCheckDeclObjC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2422,11 +2422,8 @@ bool swift::diagnoseUnintendedObjCMethodOverrides(SourceFile &sf) {
/// Retrieve the source file for the given Objective-C member conflict.
static TinyPtrVector<AbstractFunctionDecl *>
getObjCMethodConflictDecls(const SourceFile::ObjCMethodConflict &conflict) {
NominalTypeDecl *typeDecl = std::get<0>(conflict);
ObjCSelector selector = std::get<1>(conflict);
bool isInstanceMethod = std::get<2>(conflict);

return typeDecl->lookupDirect(selector, isInstanceMethod);
return conflict.typeDecl->lookupDirect(conflict.selector,
conflict.isInstanceMethod);
}

static ObjCAttr *getObjCAttrIfFromAccessNote(ValueDecl *VD) {
Expand All @@ -2452,7 +2449,8 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
// Sort the set of conflicts so we get a deterministic order for
// diagnostics. We use the first conflicting declaration in each set to
// perform the sort.
auto localConflicts = sf.ObjCMethodConflicts;
llvm::SmallVector<SourceFile::ObjCMethodConflict, 4> localConflicts;
llvm::copy(sf.ObjCMethodConflicts, std::back_inserter(localConflicts));
std::sort(localConflicts.begin(), localConflicts.end(),
[&](const SourceFile::ObjCMethodConflict &lhs,
const SourceFile::ObjCMethodConflict &rhs) {
Expand All @@ -2463,9 +2461,6 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
// Diagnose each conflict.
bool anyConflicts = false;
for (const auto &conflict : localConflicts) {
NominalTypeDecl *tyDecl = std::get<0>(conflict);
ObjCSelector selector = std::get<1>(conflict);

auto methods = getObjCMethodConflictDecls(conflict);

// Erase any invalid or stub declarations. We don't want to complain about
Expand Down Expand Up @@ -2545,10 +2540,10 @@ bool swift::diagnoseObjCMethodConflicts(SourceFile &sf) {
: diag::objc_redecl,
diagInfo.first, diagInfo.second,
origDiagInfo.first, origDiagInfo.second,
selector);
conflict.selector);

// Protocols weren't checked for selector conflicts in 5.0.
diag.warnUntilSwiftVersionIf(!isa<ClassDecl>(tyDecl), 6);
diag.warnUntilSwiftVersionIf(!isa<ClassDecl>(conflict.typeDecl), 6);

auto objcAttr = getObjCAttrIfFromAccessNote(conflictingDecl);
swift::softenIfAccessNote(conflictingDecl, objcAttr, diag);
Expand Down
Loading