Merge pull request github#12348 from github/alexdenisov/extract-emission-body-decisions

Swift: move decision making out of dispatcher. NFC

Author: AlexDenisov

swift/extractor/SwiftExtractor.cpp

@@ -13,6 +13,7 @@
 #include "swift/extractor/SwiftBuiltinSymbols.h"
 #include "swift/extractor/infra/file/Path.h"
 #include "swift/extractor/infra/SwiftLocationExtractor.h"
+#include "swift/extractor/infra/SwiftBodyEmissionStrategy.h"
 
 using namespace codeql;
 using namespace std::string_literals;
@@ -142,7 +143,8 @@ static std::unordered_set<swift::ModuleDecl*> extractDeclarations(
 
   SwiftLocationExtractor locationExtractor(*trap);
   locationExtractor.emitFile(primaryFile);
-  SwiftVisitor visitor(compiler.getSourceMgr(), *trap, locationExtractor, module, primaryFile);
+  SwiftBodyEmissionStrategy bodyEmissionStrategy(module, primaryFile);
+  SwiftVisitor visitor(compiler.getSourceMgr(), *trap, locationExtractor, bodyEmissionStrategy);
   auto topLevelDecls = getTopLevelDecls(module, primaryFile);
   for (auto decl : topLevelDecls) {
     visitor.extract(decl);

swift/extractor/infra/SwiftBodyEmissionStrategy.cpp

@@ -0,0 +1,28 @@
+#include "swift/extractor/infra/SwiftBodyEmissionStrategy.h"
+
+using namespace codeql;
+
+// In order to not emit duplicated entries for declarations, we restrict emission to only
+// Decls declared within the current "scope".
+// Depending on the whether we are extracting a primary source file or not the scope is defined as
+// follows:
+//  - not extracting a primary source file (`currentPrimarySourceFile == nullptr`): the current
+//    scope means the current module. This is used in the case of system or builtin modules.
+//  - extracting a primary source file: in this mode, we extract several files belonging to the
+//    same module one by one. In this mode, we restrict emission only to the same file ignoring
+//    all the other files.
+bool SwiftBodyEmissionStrategy::shouldEmitDeclBody(const swift::Decl& decl) {
+  auto module = decl.getModuleContext();
+  if (module != &currentModule) {
+    return false;
+  }
+  // ModuleDecl is a special case: if it passed the previous test, it is the current module
+  // but it never has a source file, so we short circuit to emit it in any case
+  if (!currentPrimarySourceFile || decl.getKind() == swift::DeclKind::Module) {
+    return true;
+  }
+  if (auto context = decl.getDeclContext()) {
+    return currentPrimarySourceFile == context->getParentSourceFile();
+  }
+  return false;
+}

swift/extractor/infra/SwiftBodyEmissionStrategy.h

@@ -0,0 +1,20 @@
+#pragma once
+
+#include <swift/AST/SourceFile.h>
+#include <swift/AST/Module.h>
+
+namespace codeql {
+
+class SwiftBodyEmissionStrategy {
+ public:
+  SwiftBodyEmissionStrategy(swift::ModuleDecl& currentModule,
+                            swift::SourceFile* currentPrimarySourceFile)
+      : currentModule(currentModule), currentPrimarySourceFile(currentPrimarySourceFile) {}
+  bool shouldEmitDeclBody(const swift::Decl& decl);
+
+ private:
+  swift::ModuleDecl& currentModule;
+  swift::SourceFile* currentPrimarySourceFile;
+};
+
+}  // namespace codeql

swift/extractor/infra/SwiftDispatcher.h

@@ -11,6 +11,7 @@
 #include "swift/extractor/infra/SwiftTagTraits.h"
 #include "swift/extractor/trap/generated/TrapClasses.h"
 #include "swift/extractor/infra/SwiftLocationExtractor.h"
+#include "swift/extractor/infra/SwiftBodyEmissionStrategy.h"
 
 namespace codeql {
 
@@ -46,13 +47,11 @@ class SwiftDispatcher {
   SwiftDispatcher(const swift::SourceManager& sourceManager,
                   TrapDomain& trap,
                   SwiftLocationExtractor& locationExtractor,
-                  swift::ModuleDecl& currentModule,
-                  swift::SourceFile* currentPrimarySourceFile = nullptr)
+                  SwiftBodyEmissionStrategy& bodyEmissionStrategy)
       : sourceManager{sourceManager},
         trap{trap},
         locationExtractor{locationExtractor},
-        currentModule{currentModule},
-        currentPrimarySourceFile{currentPrimarySourceFile} {}
+        bodyEmissionStrategy{bodyEmissionStrategy} {}
 
   const std::unordered_set<swift::ModuleDecl*> getEncounteredModules() && {
     return std::move(encounteredModules);
@@ -238,36 +237,9 @@ class SwiftDispatcher {
     trap.debug(args...);
   }
 
-  // In order to not emit duplicated entries for declarations, we restrict emission to only
-  // Decls declared within the current "scope".
-  // Depending on the whether we are extracting a primary source file or not the scope is defined as
-  // follows:
-  //  - not extracting a primary source file (`currentPrimarySourceFile == nullptr`): the current
-  //    scope means the current module. This is used in the case of system or builtin modules.
-  //  - extracting a primary source file: in this mode, we extract several files belonging to the
-  //    same module one by one. In this mode, we restrict emission only to the same file ignoring
-  //    all the other files.
-  // This is also used to register the modules we encounter.
-  // TODO calls to this function should be taken away from `DeclVisitor` and moved around with a
-  // clearer separation between naming entities (some decls, all types), deciding whether to emit
-  // them and finally visiting emitting the contents of the entity (which should remain in the
-  // visitors). Then this double responsibility (carrying out the test and registering encountered
-  // modules) should also be cleared out
   bool shouldEmitDeclBody(const swift::Decl& decl) {
-    auto module = decl.getModuleContext();
-    if (module != &currentModule) {
-      encounteredModules.insert(module);
-      return false;
-    }
-    // ModuleDecl is a special case: if it passed the previous test, it is the current module
-    // but it never has a source file, so we short circuit to emit it in any case
-    if (!currentPrimarySourceFile || decl.getKind() == swift::DeclKind::Module) {
-      return true;
-    }
-    if (auto context = decl.getDeclContext()) {
-      return currentPrimarySourceFile == context->getParentSourceFile();
-    }
-    return false;
+    encounteredModules.insert(decl.getModuleContext());
+    return bodyEmissionStrategy.shouldEmitDeclBody(decl);
   }
 
   void emitComment(swift::Token& comment) {
@@ -333,9 +305,8 @@ class SwiftDispatcher {
   TrapDomain& trap;
   Store store;
   SwiftLocationExtractor& locationExtractor;
+  SwiftBodyEmissionStrategy& bodyEmissionStrategy;
   Store::Handle waitingForNewLabel{std::monostate{}};
-  swift::ModuleDecl& currentModule;
-  swift::SourceFile* currentPrimarySourceFile;
   std::unordered_set<swift::ModuleDecl*> encounteredModules;
 };
 

swift/extractor/invocation/SwiftInvocationExtractor.cpp

@@ -87,7 +87,10 @@ void emitSourceObjectDependencies(const SwiftExtractorState& state,
     object->emitObject(id);
     for (auto encounteredModule : state.encounteredModules) {
       if (auto depHash = getModuleHash(encounteredModule)) {
-        object->emitObjectDependency(getModuleId(encounteredModule, *depHash));
+        auto encounteredModuleId = getModuleId(encounteredModule, *depHash);
+        if (encounteredModuleId != id) {
+          object->emitObjectDependency(encounteredModuleId);
+        }
       }
     }
     for (const auto& requestedTrap : state.traps) {