Skip to content

Commit 3ca701e

Browse files
scentiniscentini
committed
Change logic for visible decls lookup
1 parent 81c0721 commit 3ca701e

14 files changed

+215
-41
lines changed

‎lib/Sema/TypeCheckAccess.cpp

Lines changed: 68 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,17 @@
2222
#include "swift/AST/ASTWalker.h"
2323
#include "swift/AST/DiagnosticsSema.h"
2424
#include "swift/AST/ExistentialLayout.h"
25-
#include "swift/AST/ModuleNameLookup.h"
25+
#include "swift/AST/Import.h"
2626
#include "swift/AST/Pattern.h"
2727
#include "swift/AST/ParameterList.h"
28+
#include "swift/AST/SourceFile.h"
29+
#include "swift/AST/Type.h"
2830
#include "swift/AST/TypeCheckRequests.h"
31+
#include "clang/AST/Type.h"
32+
#include "clang/Basic/Module.h"
33+
#include "llvm/ADT/SmallPtrSet.h"
34+
#include "llvm/ADT/SmallVector.h"
35+
#include "llvm/Support/Debug.h"
2936

3037
using namespace swift;
3138

@@ -1441,19 +1448,60 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
14411448
}
14421449
};
14431450

1451+
void getVisibleModules(
1452+
llvm::SmallPtrSetImpl<const clang::Module *> &visibleModules,
1453+
swift::SourceFile *SF) {
1454+
llvm::SmallPtrSet<swift::ModuleDecl *, 4> seenModules;
1455+
llvm::SmallVector<swift::ModuleDecl *, 4> stack;
1456+
1457+
auto filter = ModuleDecl::ImportFilter(
1458+
{ModuleDecl::ImportFilterKind::Exported,
1459+
ModuleDecl::ImportFilterKind::Default,
1460+
ModuleDecl::ImportFilterKind::SPIAccessControl,
1461+
ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay});
1462+
1463+
SmallVector<ImportedModule, 4> sfImportedModules;
1464+
1465+
SF->getImportedModules(sfImportedModules, filter);
1466+
1467+
for (auto importedModule : sfImportedModules) {
1468+
stack.push_back(importedModule.importedModule);
1469+
seenModules.insert(importedModule.importedModule);
1470+
}
1471+
1472+
while (!stack.empty()) {
1473+
auto module = stack.pop_back_val();
1474+
if (auto clangModule = module->findUnderlyingClangModule()) {
1475+
visibleModules.insert(clangModule);
1476+
continue;
1477+
}
1478+
1479+
SmallVector<ImportedModule, 4> importedModules;
1480+
module->getImportedModules(importedModules, filter);
1481+
1482+
for (auto &importedModule : importedModules) {
1483+
auto moduleDecl = importedModule.importedModule;
1484+
if (!seenModules.contains(moduleDecl)) {
1485+
seenModules.insert(moduleDecl);
1486+
stack.push_back(moduleDecl);
1487+
}
1488+
}
1489+
}
1490+
}
14441491
} // end anonymous namespace
14451492

14461493
/// Returns the kind of origin, implementation-only import or SPI declaration,
14471494
/// that restricts exporting \p decl from the given file and context.
14481495
///
14491496
/// Local variant to swift::getDisallowedOriginKind for downgrade to warnings.
14501497
DisallowedOriginKind
1451-
swift::getDisallowedOriginKind(const Decl *decl,
1452-
ExportContext where,
1498+
swift::getDisallowedOriginKind(const Decl *decl, ExportContext where,
14531499
DowngradeToWarning &downgradeToWarning) {
14541500
downgradeToWarning = DowngradeToWarning::No;
14551501
ModuleDecl *M = decl->getModuleContext();
1502+
14561503
auto *SF = where.getDeclContext()->getParentSourceFile();
1504+
14571505
if (SF->isImportedImplementationOnly(M)) {
14581506
// Temporarily downgrade implementation-only exportability in SPI to
14591507
// a warning.
@@ -1462,31 +1510,28 @@ swift::getDisallowedOriginKind(const Decl *decl,
14621510

14631511
// Even if the current module is @_implementationOnly, Swift should
14641512
// not report an error in the cases where the decl is also exported from
1465-
// a non @_implementationOnly module. Thus, we look at all the imported
1513+
// a non @_implementationOnly module. Thus, we look at all the visible
14661514
// modules and see if we can find the decl in a non @_implementationOnly
14671515
// module.
1516+
llvm::SmallPtrSet<const clang::Module *, 4> visibleModules;
1517+
getVisibleModules(visibleModules, SF);
14681518

1469-
SmallVector<ImportedModule, 4> importedModules;
1470-
SF->getImportedModules(
1471-
importedModules,
1472-
ModuleDecl::ImportFilter(
1473-
{ModuleDecl::ImportFilterKind::Exported,
1474-
ModuleDecl::ImportFilterKind::Default,
1475-
ModuleDecl::ImportFilterKind::SPIAccessControl,
1476-
ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay}));
1477-
auto nlOptions = NL_QualifiedDefault | NL_IncludeUsableFromInline;
1478-
if (auto val = dyn_cast<ValueDecl>(decl)) {
1479-
for (auto &importedModule : importedModules) {
1480-
SmallVector<ValueDecl *, 4> candidateDecls;
1481-
namelookup::lookupInModule(
1482-
importedModule.importedModule, val->getName(), candidateDecls,
1483-
NLKind::UnqualifiedLookup, namelookup::ResolutionKind::Overloadable,
1484-
importedModule.importedModule, nlOptions);
1485-
for (auto *candidateDecl : candidateDecls) {
1486-
if (candidateDecl->getFormalAccess() >= AccessLevel::Public) {
1487-
return DisallowedOriginKind::None;
1519+
if (auto clangDecl = decl->getClangDecl()) {
1520+
for (auto redecl : clangDecl->redecls()) {
1521+
if (!visibleModules.contains(redecl->getOwningModule())) {
1522+
redecl->getOwningModule()->dump();
1523+
continue;
1524+
}
1525+
1526+
if (auto tagRedecl = dyn_cast<clang::TagDecl>(redecl)) {
1527+
// This is a forward declaration.
1528+
// We ignore visibility of these.
1529+
if (tagRedecl->getBraceRange().isInvalid()) {
1530+
continue;
14881531
}
14891532
}
1533+
1534+
return DisallowedOriginKind::None;
14901535
}
14911536
}
14921537
// Implementation-only imported, cannot be reexported.

‎test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313
// Swift should consider all sources for the decl and recognize that the
1414
// decl is not hidden behind @_implementationOnly in all modules.
1515

16-
// This test, as well as `lookup-visible-decls-recursively.swift` checks
17-
// that the `getFortyTwo` decl can be found when at least one of the
18-
// modules is not `@_implementationOnly`.
16+
// This test, as well as `lookup-visible-decls-recursively.swift`
17+
// ensures that Swift looks into the transitive visible modules as well
18+
// when looking for the `getFortyTwo` decl.
1919

2020
import UseModuleA
2121
@_implementationOnly import UseModuleB

‎test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
// decl is not hidden behind @_implementationOnly in all modules.
1515

1616
// This test, as well as `lookup-visible-decls-recursively-inversed.swift`
17-
// checks that the `getFortyTwo` decl can be found when at least one of the
18-
// modules is not `@_implementationOnly`.
17+
// ensures that Swift looks into the transitive visible modules as well
18+
// when looking for the `getFortyTwo` decl.
1919

2020
import UseModuleA
2121
@_implementationOnly import UseModuleB

‎test/Interop/Cxx/implementation-only-imports/Inputs/helper.h

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
11
#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H
22
#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H
33

4-
inline int getFortyTwo() { return 42; };
4+
inline int getFortyTwo() { return 42; }
55

66
class MagicWrapper {
7-
int _number;
8-
97
public:
10-
MagicWrapper(){};
8+
int _number;
9+
MagicWrapper(){_number = 2;};
1110
MagicWrapper(int number) : _number(number){};
11+
MagicWrapper operator - (MagicWrapper other) {
12+
return MagicWrapper{_number - other._number};
13+
}
1214
};
1315

16+
inline MagicWrapper operator + (MagicWrapper lhs, MagicWrapper rhs) {
17+
return MagicWrapper{lhs._number + rhs._number};
18+
}
19+
1420
#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H

‎test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,11 @@ module UserB {
88
export *
99
}
1010

11+
module UserC {
12+
header "user-c.h"
13+
export *
14+
}
15+
1116
module DeclA {
1217
header "decl-a.h"
1318
export *

‎test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H
2+
#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H
3+
4+
int getFortyTwo();
5+
6+
class MagicWrapper;
7+
8+
#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H

‎test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop
3+
4+
// If a decl comes from two modules, one of which is marked as
5+
// @_implementationOnly, Swift may choose the @_implementationOnly source
6+
// and then error out due to the decl being hidden.
7+
8+
// Swift should consider all sources for the decl and recognize that the
9+
// decl is not hidden behind @_implementationOnly in all modules.
10+
11+
// This test, as well as `check-constructor-visibility.swift` checks
12+
// that the constructor decl can be found when at least one of the
13+
// modules is not `@_implementationOnly`.
14+
15+
@_implementationOnly import UserA
16+
import UserB
17+
18+
@_inlineable
19+
public func createAWrapper() {
20+
let _ = MagicWrapper()
21+
}

‎test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,21 @@
11
// RUN: %empty-directory(%t)
22
// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop
33

4-
// REQUIRES: SR-13785
4+
// If a decl comes from two modules, one of which is marked as
5+
// @_implementationOnly, Swift may choose the @_implementationOnly source
6+
// and then error out due to the decl being hidden.
57

6-
@_implementationOnly import UserA
7-
import UserB
8+
// Swift should consider all sources for the decl and recognize that the
9+
// decl is not hidden behind @_implementationOnly in all modules.
10+
11+
// This test, as well as `check-constructor-visibility-inversed.swift` checks
12+
// that the constructor decl can be found when at least one of the
13+
// modules is not `@_implementationOnly`.
14+
15+
import UserA
16+
@_implementationOnly import UserB
817

918
@_inlineable
1019
public func createAWrapper() {
11-
let _wrapper = MagicWrapper()
20+
let _ = MagicWrapper()
1221
}

‎test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
// RUN: %empty-directory(%t)
22
// RUN: not %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s 2>&1 | %FileCheck %s
33

4+
// This test check that Swift recognizes that the DeclA and DeclB provide
5+
// different implementations for `getFortySomething()`
6+
47
@_implementationOnly import DeclA
58
import DeclB
69

‎test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop
3+
4+
// If a decl comes from two modules, one of which is marked as
5+
// @_implementationOnly, Swift may choose the @_implementationOnly source
6+
// and then error out due to the decl being hidden.
7+
8+
// Swift should consider all sources for the decl and recognize that the
9+
// decl is not hidden behind @_implementationOnly in all modules.
10+
11+
// This test, as well as `check-operator-visibility.swift` checks
12+
// that the operator decl can be found when at least one of the
13+
// modules is not `@_implementationOnly`.
14+
15+
16+
import UserA
17+
@_implementationOnly import UserB
18+
19+
@_inlineable
20+
public func addWrappers() {
21+
let wrapperA = MagicWrapper()
22+
let wrapperB = MagicWrapper()
23+
let _ = wrapperA + wrapperB
24+
}
25+
26+
@_inlineable
27+
public func subtractWrappers() {
28+
var wrapperA = MagicWrapper()
29+
let wrapperB = MagicWrapper()
30+
let _ = wrapperA - wrapperB
31+
}

0 commit comments

Comments
 (0)