Patch Out a Source of Iterator Invalidation

Synthesized file units were designed for autodiff to emit synthesized declarations, and also to sidestep the design implications of doing so late in the compiler pipeline.

A call to materialize synthesized file units was added to the GetImplicitSendable request. This introduced a source of iterator invalidation into forEachFileToTypeCheck in whole-module builds. Any call to insert a new file into the module has the potential to cause the underlying SmallVector to reallocate.

This patch provides a narrow workaround that stops using iterators altogether in forEachFileToTypeCheck. However, this bug reveals a severe architectural flaw in the concept of a synthesized file unit. Iterating over the files in a module is an extremely common operation, and there now are myriad ways we could wind up calling a function that mutates the module's list of files in the process. This also means the number and kind of files being visited by compiler analyses is dependent upon whether a request that inserts these files has or has not been called.

This suggests the call to ModuleDecl::addFile in FileUnit::getOrCreateSynthesizedFile is deleterious and should be removed. Doing so will come as part of a larger refactoring.

rdar://94043340

Author: CodaFi

include/swift/AST/FileUnit.h

@@ -55,6 +55,12 @@ class FileUnit : public DeclContext, public ASTAllocated<FileUnit> {
   /// Returns the synthesized file for this source file, if it exists.
   SynthesizedFileUnit *getSynthesizedFile() const;
 
+  /// Returns the synthesized file for this source file, creating one and
+  /// inserting it into the module if it does not exist.
+  ///
+  /// \warning Because this function mutates the parent module's list of files,
+  ///          it will invalidate the iterators of any upstream callers of
+  ///          \c ModuleDecl::getFiles().
   SynthesizedFileUnit &getOrCreateSynthesizedFile();
 
   /// Look up a (possibly overloaded) value set at top-level scope

lib/AST/Module.cpp

@@ -3055,6 +3055,15 @@ SynthesizedFileUnit &FileUnit::getOrCreateSynthesizedFile() {
       return *thisSynth;
     SynthesizedFile = new (getASTContext()) SynthesizedFileUnit(*this);
     SynthesizedFileAndKind.setPointer(SynthesizedFile);
+    // FIXME: Mutating the module in-flight is not a good idea. Any
+    // callers above us in the stack that are iterating over
+    // the module's files will have their iterators invalidated. There's
+    // a strong chance that whatever analysis led to this function being
+    // called is doing just that!
+    //
+    // Instead we ought to just call ModuleDecl::clearLookupCache() here
+    // and patch out the places looking for synthesized files hanging off of
+    // source files.
     getParentModule()->addFile(*SynthesizedFile);
   }
   return *SynthesizedFile;

lib/Frontend/Frontend.cpp

@@ -1166,8 +1166,13 @@ bool CompilerInstance::loadPartialModulesAndImplicitImports(
 bool CompilerInstance::forEachFileToTypeCheck(
     llvm::function_ref<bool(SourceFile &)> fn) {
   if (isWholeModuleCompilation()) {
-    for (auto fileName : getMainModule()->getFiles()) {
-      auto *SF = dyn_cast<SourceFile>(fileName);
+    // FIXME: Do not refactor this to use an iterator as long as
+    // ModuleDecl::addFile is called during Sema. Synthesized files pushed
+    // during semantic analysis will cause iterator invalidation here.
+    // See notes in SourceFile::getOrCreateSynthesizedFile() for more.
+    unsigned i = 0;
+    while (i < getMainModule()->getFiles().size()) {
+      auto *SF = dyn_cast<SourceFile>(getMainModule()->getFiles()[i++]);
       if (!SF) {
         continue;
       }