Merge pull request #59144 from CodaFi/sunkissed-you-are-not

Patch Out a Source of Iterator Invalidation

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

@@ -3064,6 +3064,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

@@ -1168,8 +1168,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;
       }