Skip to content

Commit e4f4dfc

Browse files
aschwaighoferaschwaighofer
committed
Address feedback
1 parent 963c64e commit e4f4dfc

File tree

8 files changed

+52
-54
lines changed

8 files changed

+52
-54
lines changed

‎include/swift/AST/Module.h

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -856,22 +856,22 @@ class SourceFile final : public FileUnit {
856856
Testable = 0x2,
857857

858858
/// This source file has access to private declarations in the imported
859-
/// moduled.
859+
/// module.
860860
PrivateImport = 0x4,
861861
};
862862

863863
/// \see ImportFlags
864864
using ImportOptions = OptionSet<ImportFlags>;
865865

866+
typedef std::pair<ImportOptions, StringRef> ImportOptionsAndFilename;
866867
private:
867868
std::unique_ptr<LookupCache> Cache;
868869
LookupCache &getCache() const;
869870

870871
/// This is the list of modules that are imported by this module.
871872
///
872873
/// This is filled in by the Name Binding phase.
873-
ArrayRef<std::pair<ModuleDecl::ImportedModule,
874-
std::pair<ImportOptions, StringRef>>>
874+
ArrayRef<std::pair<ModuleDecl::ImportedModule, ImportOptionsAndFilename>>
875875
Imports;
876876

877877
/// A unique identifier representing this file; used to mark private decls
@@ -976,9 +976,9 @@ class SourceFile final : public FileUnit {
976976
ImplicitModuleImportKind ModImpKind, bool KeepParsedTokens = false,
977977
bool KeepSyntaxTree = false);
978978

979-
void addImports(ArrayRef<std::pair<ModuleDecl::ImportedModule,
980-
std::pair<ImportOptions, StringRef>>>
981-
IM);
979+
void addImports(
980+
ArrayRef<std::pair<ModuleDecl::ImportedModule, ImportOptionsAndFilename>>
981+
IM);
982982

983983
bool hasTestableImport(const ModuleDecl *module) const;
984984

@@ -1262,7 +1262,7 @@ class LoadedFile : public FileUnit {
12621262
auto it = FilenameForPrivateDecls.find(decl);
12631263
if (it == FilenameForPrivateDecls.end())
12641264
return StringRef();
1265-
else return it->second.str();
1265+
return it->second.str();
12661266
}
12671267

12681268
/// Look up an operator declaration.

‎include/swift/Frontend/FrontendOptions.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ class FrontendOptions {
192192
/// \see ModuleDecl::isTestingEnabled
193193
bool EnableTesting = false;
194194

195-
/// Indicates whether we are compiling for testing.
195+
/// Indicates whether we are compiling for private imports.
196196
///
197197
/// \see ModuleDecl::arePrivateImportsEnabled
198198
bool EnablePrivateImports = false;

‎lib/AST/Decl.cpp

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2418,7 +2418,7 @@ bool ValueDecl::isUsableFromInline() const {
24182418

24192419
/// Return the access level of an internal or public declaration
24202420
/// that's been testably imported.
2421-
static AccessLevel getTestableAccess(const ValueDecl *decl) {
2421+
static AccessLevel getTestableOrPrivateImportsAccess(const ValueDecl *decl) {
24222422
// Non-final classes are considered open to @testable importers.
24232423
if (auto cls = dyn_cast<ClassDecl>(decl)) {
24242424
if (!cls->isFinal())
@@ -2435,22 +2435,6 @@ static AccessLevel getTestableAccess(const ValueDecl *decl) {
24352435
return AccessLevel::Public;
24362436
}
24372437

2438-
static AccessLevel getPrivateImportsAccess(const ValueDecl *decl) {
2439-
// Non-final classes are considered open to @_private(sourceFile:) importers.
2440-
if (auto cls = dyn_cast<ClassDecl>(decl)) {
2441-
if (!cls->isFinal())
2442-
return AccessLevel::Open;
2443-
2444-
// Non-final overridable class members are considered open to
2445-
// @_private(sourceFile:) importers.
2446-
} else if (decl->isPotentiallyOverridable()) {
2447-
if (!cast<ValueDecl>(decl)->isFinal())
2448-
return AccessLevel::Open;
2449-
}
2450-
2451-
return AccessLevel::Public;
2452-
}
2453-
24542438
/// Adjust \p access based on whether \p VD is \@usableFromInline or has been
24552439
/// testably imported from \p useDC.
24562440
///
@@ -2467,18 +2451,16 @@ static AccessLevel getAdjustedFormalAccess(const ValueDecl *VD,
24672451
}
24682452

24692453
if (useDC) {
2470-
// Check whether we need to modific the access level based on
2454+
// Check whether we need to modify the access level based on
24712455
// @testable/@_private import attributes.
24722456
auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext());
24732457
if (!useSF) return access;
2474-
if (access == AccessLevel::Internal || access == AccessLevel::Public) {
2475-
if (useSF->hasTestableImport(VD->getModuleContext())){
2476-
return getTestableAccess(VD);
2477-
} else if (useSF->hasPrivateImport(access, VD)) {
2478-
return getPrivateImportsAccess(VD);
2479-
}
2480-
} else if (useSF->hasPrivateImport(access, VD))
2481-
return getPrivateImportsAccess(VD);
2458+
if (useSF->hasPrivateImport(access, VD))
2459+
return getTestableOrPrivateImportsAccess(VD);
2460+
if ((access == AccessLevel::Internal || access == AccessLevel::Public) &&
2461+
useSF->hasTestableImport(VD->getModuleContext())) {
2462+
return getTestableOrPrivateImportsAccess(VD);
2463+
}
24822464
}
24832465

24842466
return access;
@@ -2504,19 +2486,18 @@ AccessLevel ValueDecl::getEffectiveAccess() const {
25042486
break;
25052487
case AccessLevel::Public:
25062488
case AccessLevel::Internal:
2507-
if (getModuleContext()->isTestingEnabled())
2508-
effectiveAccess = getTestableAccess(this);
2509-
if (getModuleContext()->arePrivateImportsEnabled())
2510-
effectiveAccess = getPrivateImportsAccess(this);
2489+
if (getModuleContext()->isTestingEnabled() ||
2490+
getModuleContext()->arePrivateImportsEnabled())
2491+
effectiveAccess = getTestableOrPrivateImportsAccess(this);
25112492
break;
25122493
case AccessLevel::FilePrivate:
25132494
if (getModuleContext()->arePrivateImportsEnabled())
2514-
effectiveAccess = getPrivateImportsAccess(this);
2495+
effectiveAccess = getTestableOrPrivateImportsAccess(this);
25152496
break;
25162497
case AccessLevel::Private:
25172498
effectiveAccess = AccessLevel::FilePrivate;
25182499
if (getModuleContext()->arePrivateImportsEnabled())
2519-
effectiveAccess = getPrivateImportsAccess(this);
2500+
effectiveAccess = getTestableOrPrivateImportsAccess(this);
25202501
break;
25212502
}
25222503

‎lib/AST/Module.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1410,9 +1410,9 @@ bool SourceFile::hasPrivateImport(AccessLevel accessLevel,
14101410
std::pair<ImportOptions, StringRef>>;
14111411

14121412
auto *module = ofDecl->getModuleContext();
1413-
1414-
if (accessLevel == AccessLevel::Internal ||
1415-
accessLevel == AccessLevel::Public) {
1413+
switch (accessLevel) {
1414+
case AccessLevel::Internal:
1415+
case AccessLevel::Public:
14161416
// internal/public access only needs an import marked as @_private. The
14171417
// filename does not need to match (and we don't serialize it for such
14181418
// decls).
@@ -1422,7 +1422,14 @@ bool SourceFile::hasPrivateImport(AccessLevel accessLevel,
14221422
importPair.second.first.contains(
14231423
ImportFlags::PrivateImport);
14241424
});
1425+
case AccessLevel::Open:
1426+
return true;
1427+
case AccessLevel::FilePrivate:
1428+
case AccessLevel::Private:
1429+
// Fallthrough.
1430+
break;
14251431
}
1432+
14261433
auto *DC = ofDecl->getDeclContext();
14271434
if (!DC)
14281435
return false;
@@ -1433,9 +1440,6 @@ bool SourceFile::hasPrivateImport(AccessLevel accessLevel,
14331440
StringRef filename;
14341441
if (auto *file = dyn_cast<LoadedFile>(scope)) {
14351442
filename = file->getFilenameForPrivateDecl(ofDecl);
1436-
} else if (auto *file = dyn_cast<SourceFile>(scope)) {
1437-
// Source files cannot be imported from another module.
1438-
return false;
14391443
} else
14401444
return false;
14411445

‎lib/Parse/ParseDecl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1439,7 +1439,7 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
14391439
auto ForLoc = consumeToken();
14401440

14411441
// Parse ':'.
1442-
if (Tok.getText() != ":") {
1442+
if (Tok.getKind() != tok::colon) {
14431443
diagnose(ForLoc, diag::attr_private_import_expected_colon);
14441444
return false;
14451445
}

‎test/Serialization/Inputs/private_import_other.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
private struct Base {
2+
private func shouldNotBeVisible() {}
3+
}

‎test/Serialization/Inputs/private_import_other_2.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
private struct Base {
2+
private func member() {}
3+
}
4+
5+
private struct Other {
6+
}

‎test/Serialization/private_import.swift

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
// RUN: llvm-bcanalyzer -dump %t/private_import.swiftmodule > %t/private_import.dump.txt
88
// RUN: %FileCheck -check-prefix=CHECK -check-prefix=NO-PRIVATE-IMPORT %s < %t/private_import.dump.txt
99

10-
// RUN: %target-swift-frontend -emit-module -DBASE -o %t -enable-private-imports %s
10+
// RUN: %target-build-swift -module-name private_import -emit-module -o %t -enable-private-imports %S/Inputs/private_import_other.swift %S/Inputs/private_import_other_2.swift
1111
// RUN: llvm-bcanalyzer -dump %t/private_import.swiftmodule > %t/private_import.dump.txt
1212
// RUN: %FileCheck -check-prefix=CHECK -check-prefix=PRIVATE-IMPORT %s < %t/private_import.dump.txt
1313
// RUN: %FileCheck -check-prefix=NEGATIVE %s < %t/private_import.dump.txt
@@ -24,22 +24,26 @@
2424
// NEGATIVE-NOT: UnknownCode
2525

2626
#if BASE
27-
28-
private struct Base {
29-
}
30-
3127
#elseif CLIENT
3228

33-
@_private(sourceFile: "private_import.swift") import private_import
29+
@_private(sourceFile: "private_import_other.swift") import private_import
3430

3531
extension Base {
3632
private func foo() {}
3733
}
34+
35+
extension Base {
36+
// This should not cause a failure.
37+
private func shouldNotBeVisible() {}
38+
}
39+
3840
public func unreleated() {}
3941

42+
// This should not conflict with Other from private_import_other_2.swift.
43+
struct Other {}
4044
#elseif MAIN
4145

42-
@_private(sourceFile: "private_import.swift") import private_import
46+
@_private(sourceFile: "private_import_other.swift") import private_import
4347
@_private(sourceFile: "private_import.swift") import client
4448

4549
Base().foo()

0 commit comments

Comments
 (0)