Skip to content

Commit 23e3f5f

Browse files
hamishknighthamishknight
authored
Merge pull request #77666 from hamishknight/lets-try-this-again
[AST] Remove `ModuleDecl::addFile`
2 parents 3e95e81 + 4946c79 commit 23e3f5f

29 files changed

+300
-251
lines changed

‎include/swift/AST/FileUnit.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -486,9 +486,11 @@ void simple_display(llvm::raw_ostream &out, const FileUnit *file);
486486
inline FileUnit &ModuleDecl::getMainFile(FileUnitKind expectedKind) const {
487487
assert(expectedKind != FileUnitKind::Source &&
488488
"must use specific source kind; see getMainSourceFile");
489-
assert(!Files.empty() && "No files added yet");
490-
assert(Files.front()->getKind() == expectedKind);
491-
return *Files.front();
489+
490+
auto files = getFiles();
491+
assert(!files.empty() && "No files in module");
492+
assert(files.front()->getKind() == expectedKind);
493+
return *files.front();
492494
}
493495

494496
} // end namespace swift

‎include/swift/AST/Module.h

Lines changed: 47 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,9 @@ class ModuleDecl
314314
// FIXME: Do we really need to bloat all modules with this?
315315
DebuggerClient *DebugClient = nullptr;
316316

317-
SmallVector<FileUnit *, 2> Files;
317+
/// The list of files in the module. This is guaranteed to be set once module
318+
/// construction has completed. It must not be mutated afterwards.
319+
std::optional<SmallVector<FileUnit *, 2>> Files;
318320

319321
llvm::SmallDenseMap<Identifier, SmallVector<OverlayFile *, 1>>
320322
declaredCrossImports;
@@ -359,24 +361,55 @@ class ModuleDecl
359361
/// Used by the debugger to bypass resilient access to fields.
360362
bool BypassResilience = false;
361363

362-
ModuleDecl(Identifier name, ASTContext &ctx, ImplicitImportInfo importInfo);
364+
public:
365+
using PopulateFilesFn = llvm::function_ref<void(
366+
ModuleDecl *, llvm::function_ref<void(FileUnit *)>)>;
367+
368+
private:
369+
ModuleDecl(Identifier name, ASTContext &ctx, ImplicitImportInfo importInfo,
370+
PopulateFilesFn populateFiles, bool isMainModule);
363371

364372
public:
365373
/// Creates a new module with a given \p name.
366374
///
367375
/// \param importInfo Information about which modules should be implicitly
368376
/// imported by each file of this module.
369-
static ModuleDecl *
370-
create(Identifier name, ASTContext &ctx,
371-
ImplicitImportInfo importInfo = ImplicitImportInfo()) {
372-
return new (ctx) ModuleDecl(name, ctx, importInfo);
377+
/// \param populateFiles A function which populates the files for the module.
378+
/// Once called, the module's list of files may not change.
379+
static ModuleDecl *create(Identifier name, ASTContext &ctx,
380+
ImplicitImportInfo importInfo,
381+
PopulateFilesFn populateFiles) {
382+
return new (ctx) ModuleDecl(name, ctx, importInfo, populateFiles,
383+
/*isMainModule*/ false);
384+
}
385+
386+
/// Creates a new module with a given \p name.
387+
///
388+
/// \param populateFiles A function which populates the files for the module.
389+
/// Once called, the module's list of files may not change.
390+
static ModuleDecl *create(Identifier name, ASTContext &ctx,
391+
PopulateFilesFn populateFiles) {
392+
return new (ctx) ModuleDecl(name, ctx, ImplicitImportInfo(), populateFiles,
393+
/*isMainModule*/ false);
373394
}
374395

396+
/// Creates a new main module with a given \p name. The main module is the
397+
/// module being built by the compiler, containing the primary source files.
398+
///
399+
/// \param importInfo Information about which modules should be implicitly
400+
/// imported by each file of this module.
401+
/// \param populateFiles A function which populates the files for the module.
402+
/// Once called, the module's list of files may not change.
375403
static ModuleDecl *createMainModule(ASTContext &ctx, Identifier name,
376-
ImplicitImportInfo iinfo) {
377-
auto *Mod = ModuleDecl::create(name, ctx, iinfo);
378-
Mod->Bits.ModuleDecl.IsMainModule = true;
379-
return Mod;
404+
ImplicitImportInfo iinfo,
405+
PopulateFilesFn populateFiles) {
406+
return new (ctx) ModuleDecl(name, ctx, iinfo, populateFiles,
407+
/*isMainModule*/ true);
408+
}
409+
410+
/// Creates an empty module with a given \p name.
411+
static ModuleDecl *createEmpty(Identifier name, ASTContext &ctx) {
412+
return create(name, ctx, ImplicitImportInfo(), [](auto, auto) {});
380413
}
381414

382415
using Decl::getASTContext;
@@ -398,24 +431,11 @@ class ModuleDecl
398431
/// Only to be called by MemoryBufferSerializedModuleLoader.
399432
void setBypassResilience() { BypassResilience = true; }
400433

401-
ArrayRef<FileUnit *> getFiles() {
402-
ASSERT(!Files.empty() || failedToLoad());
403-
return Files;
434+
ArrayRef<FileUnit *> getFiles() const {
435+
ASSERT(Files.has_value() &&
436+
"Attempting to query files before setting them");
437+
return *Files;
404438
}
405-
ArrayRef<const FileUnit *> getFiles() const {
406-
return { Files.begin(), Files.size() };
407-
}
408-
409-
/// Add the given file to this module.
410-
///
411-
/// FIXME: Remove this function from public view. Modules never need to add
412-
/// files once they are created.
413-
///
414-
/// \warning There are very few safe points to call this function once a
415-
/// \c ModuleDecl has been created. If you find yourself needing to insert
416-
/// a file in the middle of e.g. semantic analysis, use a \c
417-
/// SynthesizedFileUnit instead.
418-
void addFile(FileUnit &newFile);
419439

420440
/// Produces the source file that contains the given source location, or
421441
/// \c nullptr if the source location isn't in this module.

‎include/swift/AST/SourceFile.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -823,8 +823,8 @@ inline SourceFile::ParsingOptions operator|(SourceFile::ParsingFlags lhs,
823823
}
824824

825825
inline SourceFile &ModuleDecl::getMainSourceFile() const {
826-
assert(!Files.empty() && "No files added yet");
827-
return *cast<SourceFile>(Files.front());
826+
assert(!getFiles().empty() && "No files in module");
827+
return *cast<SourceFile>(getFiles().front());
828828
}
829829

830830
inline FileUnit *ModuleDecl::EntryPointInfoTy::getEntryPointFile() const {

‎include/swift/IDETool/SyntacticMacroExpansion.h

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,35 +45,32 @@ class SyntacticMacroExpansionInstance {
4545
DiagnosticEngine Diags{SourceMgr};
4646
std::unique_ptr<ASTContext> Ctx;
4747
ModuleDecl *TheModule = nullptr;
48+
SourceFile *SF = nullptr;
4849
llvm::StringMap<MacroDecl *> MacroDecls;
4950

50-
/// Create 'SourceFile' using the buffer.
51-
swift::SourceFile *getSourceFile(llvm::MemoryBuffer *inputBuf);
52-
5351
/// Synthesize 'MacroDecl' AST object to use the expansion.
5452
swift::MacroDecl *
5553
getSynthesizedMacroDecl(swift::Identifier name,
5654
const MacroExpansionSpecifier &expansion);
5755

58-
/// Expand single 'expansion' in SF.
59-
void expand(swift::SourceFile *SF,
60-
const MacroExpansionSpecifier &expansion,
61-
SourceEditConsumer &consumer);
56+
/// Expand single 'expansion'.
57+
void expand(const MacroExpansionSpecifier &expansion,
58+
SourceEditConsumer &consumer);
6259

6360
public:
6461
SyntacticMacroExpansionInstance() {}
6562

66-
/// Setup the instance with \p args .
63+
/// Setup the instance with \p args and a given \p inputBuf.
6764
bool setup(StringRef SwiftExecutablePath, ArrayRef<const char *> args,
65+
llvm::MemoryBuffer *inputBuf,
6866
std::shared_ptr<PluginRegistry> plugins, std::string &error);
6967

7068
ASTContext &getASTContext() { return *Ctx; }
7169

72-
/// Expand all macros in \p inputBuf and send the edit results to \p consumer.
73-
/// Expansions are specified by \p expansions .
74-
void expandAll(llvm::MemoryBuffer *inputBuf,
75-
ArrayRef<MacroExpansionSpecifier> expansions,
76-
SourceEditConsumer &consumer);
70+
/// Expand all macros and send the edit results to \p consumer. Expansions are
71+
/// specified by \p expansions .
72+
void expandAll(ArrayRef<MacroExpansionSpecifier> expansions,
73+
SourceEditConsumer &consumer);
7774
};
7875

7976
/// Manager object to vend 'SyntacticMacroExpansionInstance'.
@@ -86,9 +83,11 @@ class SyntacticMacroExpansion {
8683
std::shared_ptr<PluginRegistry> Plugins)
8784
: SwiftExecutablePath(SwiftExecutablePath), Plugins(Plugins) {}
8885

89-
/// Get instance configured with the specified compiler arguments.
86+
/// Get instance configured with the specified compiler arguments and
87+
/// input buffer.
9088
std::shared_ptr<SyntacticMacroExpansionInstance>
91-
getInstance(ArrayRef<const char *> args, std::string &error);
89+
getInstance(ArrayRef<const char *> args, llvm::MemoryBuffer *inputBuf,
90+
std::string &error);
9291
};
9392

9493
} // namespace ide

‎include/swift/Subsystems.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,9 @@ namespace swift {
117117
bool TokenizeInterpolatedString = true,
118118
ArrayRef<Token> SplitTokens = ArrayRef<Token>());
119119

120+
/// Perform import resolution for the module.
121+
void performImportResolution(ModuleDecl *M);
122+
120123
/// This walks the AST to resolve imports.
121124
void performImportResolution(SourceFile &SF);
122125

‎lib/AST/ASTContext.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -734,8 +734,10 @@ ConstraintCheckerArenaRAII::~ConstraintCheckerArenaRAII() {
734734
}
735735

736736
static ModuleDecl *createBuiltinModule(ASTContext &ctx) {
737-
auto M = ModuleDecl::create(ctx.getIdentifier(BUILTIN_NAME), ctx);
738-
M->addFile(*new (ctx) BuiltinUnit(*M));
737+
auto *M = ModuleDecl::create(ctx.getIdentifier(BUILTIN_NAME), ctx,
738+
[&](ModuleDecl *M, auto addFile) {
739+
addFile(new (ctx) BuiltinUnit(*M));
740+
});
739741
M->setHasResolvedImports();
740742
return M;
741743
}

‎lib/AST/ASTDemangler.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,7 @@ ASTBuilder::getAcceptableTypeDeclCandidate(ValueDecl *decl,
11351135

11361136
DeclContext *ASTBuilder::getNotionalDC() {
11371137
if (!NotionalDC) {
1138-
NotionalDC = ModuleDecl::create(Ctx.getIdentifier(".RemoteAST"), Ctx);
1138+
NotionalDC = ModuleDecl::createEmpty(Ctx.getIdentifier(".RemoteAST"), Ctx);
11391139
NotionalDC = new (Ctx) TopLevelCodeDecl(NotionalDC);
11401140
}
11411141
return NotionalDC;

‎lib/AST/Module.cpp

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -737,7 +737,9 @@ void SourceLookupCache::lookupClassMember(ImportPath::Access accessPath,
737737
//===----------------------------------------------------------------------===//
738738

739739
ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx,
740-
ImplicitImportInfo importInfo)
740+
ImplicitImportInfo importInfo,
741+
PopulateFilesFn populateFiles,
742+
bool isMainModule)
741743
: DeclContext(DeclContextKind::Module, nullptr),
742744
TypeDecl(DeclKind::Module, &ctx, name, SourceLoc(), {}),
743745
ImportInfo(importInfo) {
@@ -756,7 +758,7 @@ ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx,
756758
Bits.ModuleDecl.ImplicitDynamicEnabled = 0;
757759
Bits.ModuleDecl.IsSystemModule = 0;
758760
Bits.ModuleDecl.IsNonSwiftModule = 0;
759-
Bits.ModuleDecl.IsMainModule = 0;
761+
Bits.ModuleDecl.IsMainModule = isMainModule;
760762
Bits.ModuleDecl.HasIncrementalInfo = 0;
761763
Bits.ModuleDecl.HasHermeticSealAtLink = 0;
762764
Bits.ModuleDecl.IsEmbeddedSwiftModule = 0;
@@ -766,6 +768,22 @@ ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx,
766768
Bits.ModuleDecl.CXXStdlibKind = 0;
767769
Bits.ModuleDecl.AllowNonResilientAccess = 0;
768770
Bits.ModuleDecl.SerializePackageEnabled = 0;
771+
772+
// Populate the module's files.
773+
SmallVector<FileUnit *, 2> files;
774+
populateFiles(this, [&](FileUnit *file) {
775+
// If this is a LoadedFile, make sure it loaded without error.
776+
assert(!(isa<LoadedFile>(file) &&
777+
cast<LoadedFile>(file)->hadLoadError()));
778+
779+
// Require Main and REPL files to be the first file added.
780+
assert(files.empty() ||
781+
!isa<SourceFile>(file) ||
782+
cast<SourceFile>(file)->Kind == SourceFileKind::Library ||
783+
cast<SourceFile>(file)->Kind == SourceFileKind::SIL);
784+
files.push_back(file);
785+
});
786+
Files.emplace(std::move(files));
769787
}
770788

771789
void ModuleDecl::setIsSystemModule(bool flag) {
@@ -788,20 +806,6 @@ ImplicitImportList ModuleDecl::getImplicitImports() const {
788806
{});
789807
}
790808

791-
void ModuleDecl::addFile(FileUnit &newFile) {
792-
// If this is a LoadedFile, make sure it loaded without error.
793-
assert(!(isa<LoadedFile>(newFile) &&
794-
cast<LoadedFile>(newFile).hadLoadError()));
795-
796-
// Require Main and REPL files to be the first file added.
797-
assert(Files.empty() ||
798-
!isa<SourceFile>(newFile) ||
799-
cast<SourceFile>(newFile).Kind == SourceFileKind::Library ||
800-
cast<SourceFile>(newFile).Kind == SourceFileKind::SIL);
801-
Files.push_back(&newFile);
802-
clearLookupCache();
803-
}
804-
805809
SourceFile *ModuleDecl::getSourceFileContainingLocation(SourceLoc loc) {
806810
if (loc.isInvalid())
807811
return nullptr;
@@ -1210,7 +1214,7 @@ void SourceFile::lookupObjCMethods(
12101214
}
12111215

12121216
bool ModuleDecl::shouldCollectDisplayDecls() const {
1213-
for (const FileUnit *file : Files) {
1217+
for (const FileUnit *file : getFiles()) {
12141218
if (!file->shouldCollectDisplayDecls())
12151219
return false;
12161220
}
@@ -1791,6 +1795,15 @@ void SourceFile::dumpSeparatelyImportedOverlays() const {
17911795

17921796
void ModuleDecl::getImportedModulesForLookup(
17931797
SmallVectorImpl<ImportedModule> &modules) const {
1798+
// FIXME: We can unfortunately end up in a cycle where we attempt to query
1799+
// the files for a serialized module while attempting to load its LoadedFile
1800+
// unit. This is because both SerializedModuleLoader and ClangImporter
1801+
// kick the computation of transitive module dependencies, which can end up
1802+
// pointing back to the original module in cases where it's an overlay. We
1803+
// ought to be more lazy there. This is also why
1804+
// `SourceFile::getImportedModules` cannot assume imports have been resolved.
1805+
if (!Files.has_value())
1806+
return;
17941807
FORWARD(getImportedModulesForLookup, (modules));
17951808
}
17961809

‎lib/ClangImporter/ClangImporter.cpp

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,11 +1583,14 @@ ClangImporter::create(ASTContext &ctx,
15831583
= clangContext.Selectors.getSelector(2, setObjectForKeyedSubscriptIdents);
15841584

15851585
// Set up the imported header module.
1586-
auto *importedHeaderModule =
1587-
ModuleDecl::create(ctx.getIdentifier(CLANG_HEADER_MODULE_NAME), ctx);
1588-
importer->Impl.ImportedHeaderUnit =
1589-
new (ctx) ClangModuleUnit(*importedHeaderModule, importer->Impl, nullptr);
1590-
importedHeaderModule->addFile(*importer->Impl.ImportedHeaderUnit);
1586+
auto *importedHeaderModule = ModuleDecl::create(
1587+
ctx.getIdentifier(CLANG_HEADER_MODULE_NAME), ctx,
1588+
[&](ModuleDecl *importedHeaderModule, auto addFile) {
1589+
importer->Impl.ImportedHeaderUnit = new (ctx)
1590+
ClangModuleUnit(*importedHeaderModule, importer->Impl, nullptr);
1591+
addFile(importer->Impl.ImportedHeaderUnit);
1592+
});
1593+
15911594
importedHeaderModule->setHasResolvedImports();
15921595
importedHeaderModule->setIsNonSwiftModule(true);
15931596

@@ -2807,17 +2810,19 @@ ClangModuleUnit *ClangImporter::Implementation::getWrapperForModule(
28072810
if (auto mainModule = SwiftContext.MainModule) {
28082811
implicitImportInfo = mainModule->getImplicitImportInfo();
28092812
}
2810-
auto wrapper = ModuleDecl::create(name, SwiftContext, implicitImportInfo);
2813+
ClangModuleUnit *file = nullptr;
2814+
auto wrapper = ModuleDecl::create(name, SwiftContext, implicitImportInfo,
2815+
[&](ModuleDecl *wrapper, auto addFile) {
2816+
file = new (SwiftContext) ClangModuleUnit(*wrapper, *this, underlying);
2817+
addFile(file);
2818+
});
28112819
wrapper->setIsSystemModule(underlying->IsSystem);
28122820
wrapper->setIsNonSwiftModule();
28132821
wrapper->setHasResolvedImports();
28142822
if (!underlying->ExportAsModule.empty())
28152823
wrapper->setExportAsName(
28162824
SwiftContext.getIdentifier(underlying->ExportAsModule));
28172825

2818-
auto file = new (SwiftContext) ClangModuleUnit(*wrapper, *this,
2819-
underlying);
2820-
wrapper->addFile(*file);
28212826
SwiftContext.getClangModuleLoader()->findOverlayFiles(diagLoc, wrapper, file);
28222827
cacheEntry.setPointer(file);
28232828

‎lib/ClangImporter/DWARFImporter.cpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -116,23 +116,25 @@ ModuleDecl *ClangImporter::Implementation::loadModuleDWARF(
116116
if (it != DWARFModuleUnits.end())
117117
return it->second->getParentModule();
118118

119-
auto *decl = ModuleDecl::create(name, SwiftContext);
120-
decl->setIsNonSwiftModule();
121-
decl->setHasResolvedImports();
122-
auto *wrapperUnit = new (SwiftContext) DWARFModuleUnit(*decl, *this);
123-
DWARFModuleUnits.insert({name, wrapperUnit});
124-
decl->addFile(*wrapperUnit);
119+
auto *M = ModuleDecl::create(name, SwiftContext,
120+
[&](ModuleDecl *M, auto addFile) {
121+
auto *wrapperUnit = new (SwiftContext) DWARFModuleUnit(*M, *this);
122+
DWARFModuleUnits.insert({name, wrapperUnit});
123+
addFile(wrapperUnit);
124+
});
125+
M->setIsNonSwiftModule();
126+
M->setHasResolvedImports();
125127

126128
// Force load overlay modules for all imported modules.
127-
assert(namelookup::getAllImports(decl).size() == 1 &&
128-
namelookup::getAllImports(decl).front().importedModule == decl &&
129+
assert(namelookup::getAllImports(M).size() == 1 &&
130+
namelookup::getAllImports(M).front().importedModule == M &&
129131
"DWARF module depends on additional modules?");
130132

131133
// Register the module with the ASTContext so it is available for lookups.
132134
if (!SwiftContext.getLoadedModule(name))
133-
SwiftContext.addLoadedModule(decl);
135+
SwiftContext.addLoadedModule(M);
134136

135-
return decl;
137+
return M;
136138
}
137139

138140
// This function exists to defeat the lazy member importing mechanism. The

0 commit comments

Comments
 (0)