Skip to content

Further loosen selector conflict checks #60081

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 1 commit into from
Jul 20, 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
35 changes: 28 additions & 7 deletions lib/AST/NameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "swift/Basic/Statistic.h"
#include "swift/ClangImporter/ClangImporterRequests.h"
#include "swift/Parse/Lexer.h"
#include "swift/Strings.h"
#include "clang/AST/DeclObjC.h"
#include "clang/Basic/Specifiers.h"
#include "llvm/ADT/DenseMap.h"
Expand Down Expand Up @@ -1716,14 +1717,34 @@ 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) {
/// If there is an apparent conflict between \p newDecl and one of the methods
/// in \p vec, should we diagnose it?
static bool
shouldDiagnoseConflict(NominalTypeDecl *ty, AbstractFunctionDecl *newDecl,
llvm::TinyPtrVector<AbstractFunctionDecl *> &vec) {
// Are all conflicting methods imported from ObjC and in our ObjC half or a
// bridging header? Some code bases implement ObjC methods in Swift even
// though it's not exactly supported.
auto newDeclModuleName = newDecl->getModuleContext()->getName();
if (llvm::all_of(vec, [&](AbstractFunctionDecl *oldDecl) {
if (!oldDecl->hasClangNode())
return false;
auto oldDeclModuleName = oldDecl->getModuleContext()->getName();
return oldDeclModuleName == newDeclModuleName
|| oldDeclModuleName.str() == CLANG_HEADER_MODULE_NAME;
}))
return false;

// If we're looking at protocol requirements, is the new method an async
// alternative of any existing method, or vice versa?
if (isa<ProtocolDecl>(ty) &&
llvm::any_of(vec, [&](AbstractFunctionDecl *oldDecl) {
return newDecl->getAsyncAlternative(/*isKnownObjC=*/true) == oldDecl
|| oldDecl->getAsyncAlternative(/*isKnownObjC=*/true) == newDecl;
});
}))
return false;

return true;
}

void NominalTypeDecl::recordObjCMethod(AbstractFunctionDecl *method,
Expand All @@ -1743,7 +1764,7 @@ void NominalTypeDecl::recordObjCMethod(AbstractFunctionDecl *method,
if (auto *sf = method->getParentSourceFile()) {
if (vec.empty()) {
sf->ObjCMethodList.push_back(method);
} else if (!isa<ProtocolDecl>(this) || !isAnAsyncAlternative(method, vec)) {
} else if (shouldDiagnoseConflict(this, method, vec)) {
// We have a conflict.
sf->ObjCMethodConflicts.insert({ this, selector, isInstanceMethod });
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckDeclObjC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2269,7 +2269,7 @@ namespace {
// location within that source file.
SourceFile *lhsSF = lhs->getDeclContext()->getParentSourceFile();
SourceFile *rhsSF = rhs->getDeclContext()->getParentSourceFile();
if (lhsSF == rhsSF) {
if (lhsSF && lhsSF == rhsSF) {
// If only one location is valid, the valid location comes first.
if (lhs->getLoc().isValid() != rhs->getLoc().isValid()) {
return lhs->getLoc().isValid();
Expand Down
5 changes: 5 additions & 0 deletions test/ClangImporter/Inputs/custom-modules/module.map
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,8 @@ module CommonName {
header "CommonName.h"
export *
}

module objc_init_redundant {
header "objc_init_redundant.h"
export *
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#import <Foundation.h>

@interface MyObject : NSObject

- (void)implementedInSwift;

@end
7 changes: 7 additions & 0 deletions test/ClangImporter/Inputs/objc_init_redundant_bridging.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#import <Foundation.h>

@interface MyBridgedObject : NSObject

- (void)implementedInSwift;

@end
13 changes: 11 additions & 2 deletions test/ClangImporter/objc_init_redundant.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk -I %S/Inputs/custom-modules) -emit-sil %s -verify
// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk -I %S/Inputs/custom-modules) -emit-sil %s > %t.log 2>&1
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk -I %S/Inputs/custom-modules) -import-underlying-module -import-objc-header %S/Inputs/objc_init_redundant_bridging.h -emit-sil %s -verify
// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk -I %S/Inputs/custom-modules) -import-underlying-module -import-objc-header %S/Inputs/objc_init_redundant_bridging.h -emit-sil %s > %t.log 2>&1
// RUN: %FileCheck %s < %t.log

// REQUIRES: objc_interop
Expand All @@ -19,3 +19,12 @@ extension NSObject {
// CHECK: ObjectiveC.NSObjectProtocol:{{.*}}note: method 'class()' declared here
}

// rdar://96470068 - Don't want conflict diagnostics in the same module
extension MyObject {
@objc func implementedInSwift() {}
}

// ...or the bridging header
extension MyBridgedObject {
@objc func implementedInSwift() {}
}
1 change: 0 additions & 1 deletion test/Concurrency/objc_async_overload.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,4 @@ extension Delegate {

extension Delegate {
@objc public func makeRequest(fromSwift: Request, completionHandler: (() -> Void)?) {}
// expected-warning@-1 {{method 'makeRequest(fromSwift:completionHandler:)' with Objective-C selector 'makeRequestFromSwift:completionHandler:' conflicts with method 'makeRequest(fromSwift:)' with the same Objective-C selector; this is an error in Swift 6}}
}