Merge pull request #33795 from slavapestov/hoisted-decls-for-lldb

Re-implement DebuggerContextChange using a new mechanism

Author: slavapestov

include/swift/AST/Decl.h

@@ -284,7 +284,7 @@ class alignas(1 << DeclAlignInBits) Decl {
 protected:
   union { uint64_t OpaqueBits;
 
-  SWIFT_INLINE_BITFIELD_BASE(Decl, bitmax(NumDeclKindBits,8)+1+1+1+1,
+  SWIFT_INLINE_BITFIELD_BASE(Decl, bitmax(NumDeclKindBits,8)+1+1+1+1+1,
     Kind : bitmax(NumDeclKindBits,8),
 
     /// Whether this declaration is invalid.
@@ -301,7 +301,13 @@ class alignas(1 << DeclAlignInBits) Decl {
 
     /// Whether this declaration was added to the surrounding
     /// DeclContext of an active #if config clause.
-    EscapedFromIfConfig : 1
+    EscapedFromIfConfig : 1,
+
+    /// Whether this declaration is syntactically scoped inside of
+    /// a local context, but should behave like a top-level
+    /// declaration for name lookup purposes. This is used by
+    /// lldb.
+    Hoisted : 1
   );
 
   SWIFT_INLINE_BITFIELD_FULL(PatternBindingDecl, Decl, 1+2+16,
@@ -690,6 +696,7 @@ class alignas(1 << DeclAlignInBits) Decl {
     Bits.Decl.Implicit = false;
     Bits.Decl.FromClang = false;
     Bits.Decl.EscapedFromIfConfig = false;
+    Bits.Decl.Hoisted = false;
   }
 
   /// Get the Clang node associated with this declaration.
@@ -837,6 +844,16 @@ class alignas(1 << DeclAlignInBits) Decl {
   /// Mark this declaration as implicit.
   void setImplicit(bool implicit = true) { Bits.Decl.Implicit = implicit; }
 
+  /// Determine whether this declaration is syntactically scoped inside of
+  /// a local context, but should behave like a top-level declaration
+  /// for name lookup purposes. This is used by lldb.
+  bool isHoisted() const { return Bits.Decl.Hoisted; }
+
+  /// Set whether this declaration should be syntactically scoped inside
+  /// of a local context, but should behave like a top-level declaration,
+  /// but should behave like a top-level declaration. This is used by lldb.
+  void setHoisted(bool hoisted = true) { Bits.Decl.Hoisted = hoisted; }
+
 public:
   bool escapedFromIfConfig() const {
     return Bits.Decl.EscapedFromIfConfig;

include/swift/AST/SourceFile.h

@@ -187,6 +187,10 @@ class SourceFile final : public FileUnit {
   /// have been removed, this can become an optional ArrayRef.
   Optional<std::vector<Decl *>> Decls;
 
+  /// The list of hoisted declarations. See Decl::isHoisted().
+  /// This is only used by lldb.
+  std::vector<Decl *> Hoisted;
+
   using SeparatelyImportedOverlayMap =
     llvm::SmallDenseMap<ModuleDecl *, llvm::SmallPtrSet<ModuleDecl *, 1>>;
 
@@ -231,9 +235,18 @@ class SourceFile final : public FileUnit {
     Decls->insert(Decls->begin(), d);
   }
 
+  /// Add a hoisted declaration. See Decl::isHoisted().
+  void addHoistedDecl(Decl *d) {
+    Hoisted.push_back(d);
+  }
+
   /// Retrieves an immutable view of the list of top-level decls in this file.
   ArrayRef<Decl *> getTopLevelDecls() const;
 
+  /// Retrieves an immutable view of the list of hoisted decls in this file.
+  /// See Decl::isHoisted().
+  ArrayRef<Decl *> getHoistedDecls() const;
+
   /// Retrieves an immutable view of the top-level decls if they have already
   /// been parsed, or \c None if they haven't. Should only be used for dumping.
   Optional<ArrayRef<Decl *>> getCachedTopLevelDecls() const {

include/swift/Parse/Parser.h

@@ -125,9 +125,6 @@ class Parser {
   CodeCompletionCallbacks *CodeCompletion = nullptr;
   std::vector<Located<std::vector<ParamDecl*>>> AnonClosureVars;
 
-  /// Tracks parsed decls that LLDB requires to be inserted at the top-level.
-  std::vector<Decl *> ContextSwitchedTopLevelDecls;
-
   /// The current token hash, or \c None if the parser isn't computing a hash
   /// for the token stream.
   Optional<llvm::MD5> CurrentTokenHash;
@@ -146,7 +143,6 @@ class Parser {
   ArrayRef<VarDecl *> DisabledVars;
   Diag<> DisabledVarReason;
 
-  llvm::SmallPtrSet<Decl *, 2> AlreadyHandledDecls;
   enum {
     /// InVarOrLetPattern has this value when not parsing a pattern.
     IVOLP_NotInVarOrLet,
@@ -930,14 +926,8 @@ class Parser {
   void consumeDecl(ParserPosition BeginParserPosition, ParseDeclOptions Flags,
                    bool IsTopLevel);
 
-  // When compiling for the Debugger, some Decl's need to be moved from the
-  // current scope.  In which case although the Decl will be returned in the
-  // ParserResult, it should not be inserted into the Decl list for the current
-  // context.  markWasHandled asserts that the Decl is already where it
-  // belongs, and declWasHandledAlready is used to check this assertion.
-  // To keep the handled decl array small, we remove the Decl when it is
-  // checked, so you can only call declWasAlreadyHandled once for a given
-  // decl.
+  /// FIXME: Remove this, it's vestigial.
+  llvm::SmallPtrSet<Decl *, 2> AlreadyHandledDecls;
 
   void markWasHandled(Decl *D) {
     AlreadyHandledDecls.insert(D);

lib/AST/ASTDumper.cpp

@@ -585,6 +585,9 @@ namespace {
       if (D->isImplicit())
         PrintWithColorRAII(OS, DeclModifierColor) << " implicit";
 
+      if (D->isHoisted())
+        PrintWithColorRAII(OS, DeclModifierColor) << " hoisted";
+
       auto R = D->getSourceRange();
       if (R.isValid()) {
         PrintWithColorRAII(OS, RangeColor) << " range=";

lib/AST/Module.cpp

@@ -312,6 +312,7 @@ SourceLookupCache::SourceLookupCache(const SourceFile &SF) {
   FrontendStatsTracer tracer(SF.getASTContext().Stats,
                              "source-file-populate-cache");
   addToUnqualifiedLookupCache(SF.getTopLevelDecls(), false);
+  addToUnqualifiedLookupCache(SF.getHoistedDecls(), false);
 }
 
 SourceLookupCache::SourceLookupCache(const ModuleDecl &M) {
@@ -322,8 +323,9 @@ SourceLookupCache::SourceLookupCache(const ModuleDecl &M) {
       addToUnqualifiedLookupCache(SFU->getTopLevelDecls(), false);
       continue;
     }
-    auto &SF = *cast<SourceFile>(file);
-    addToUnqualifiedLookupCache(SF.getTopLevelDecls(), false);
+    auto *SF = cast<SourceFile>(file);
+    addToUnqualifiedLookupCache(SF->getTopLevelDecls(), false);
+    addToUnqualifiedLookupCache(SF->getHoistedDecls(), false);
   }
 }
 
@@ -2342,6 +2344,10 @@ ArrayRef<Decl *> SourceFile::getTopLevelDecls() const {
                            {}).TopLevelDecls;
 }
 
+ArrayRef<Decl *> SourceFile::getHoistedDecls() const {
+  return Hoisted;
+}
+
 bool FileUnit::walk(ASTWalker &walker) {
   SmallVector<Decl *, 64> Decls;
   getTopLevelDecls(Decls);

lib/IRGen/GenDecl.cpp

@@ -449,13 +449,15 @@ void IRGenModule::emitSourceFile(SourceFile &SF) {
   // Emit types and other global decls.
   for (auto *decl : SF.getTopLevelDecls())
     emitGlobalDecl(decl);
+  for (auto *decl : SF.getHoistedDecls())
+    emitGlobalDecl(decl);
   for (auto *localDecl : SF.LocalTypeDecls)
     emitGlobalDecl(localDecl);
   for (auto *opaqueDecl : SF.getOpaqueReturnTypeDecls())
     maybeEmitOpaqueTypeDecl(opaqueDecl);
 
   SF.collectLinkLibraries([this](LinkLibrary linkLib) {
-      this->addLinkLibrary(linkLib);
+    this->addLinkLibrary(linkLib);
   });
 
   if (ObjCInterop)

lib/Parse/ParseDecl.cpp

@@ -60,48 +60,34 @@ namespace {
   /// file scope level so it will be set up correctly for this purpose.
   ///
   /// Creating an instance of this object will cause it to figure out
-  /// whether we are in the debugger function, whether it needs to swap 
+  /// whether we are in the debugger function, and whether it needs to swap
   /// the Decl that is currently being parsed.
-  /// If you have created the object, instead of returning the result
-  /// with makeParserResult, use the object's fixupParserResult.  If
-  /// no swap has occurred, these methods will work the same.  
-  /// If the decl has been moved, then Parser::markWasHandled will be
-  /// called on the Decl, and you should call declWasHandledAlready
-  /// before you consume the Decl to see if you actually need to
-  /// consume it.
+  ///
   /// If you are making one of these objects to address issue 1, call
   /// the constructor that only takes a DeclKind, and it will be moved
   /// unconditionally.  Otherwise pass in the Name and DeclKind and the
   /// DebuggerClient will be asked whether to move it or not.
   class DebuggerContextChange {
   protected:
     Parser &P;
-    Identifier Name;
-    SourceFile *SF;
     Optional<Parser::ContextChange> CC;
+    SourceFile *SF;
   public:
-    DebuggerContextChange (Parser &P)
-      : P(P), SF(nullptr) {
+    DebuggerContextChange(Parser &P) : P(P), SF(nullptr) {
       if (!inDebuggerContext())
         return;
-      else
-        switchContext();
+
+      switchContext();
     }
 
-    DebuggerContextChange (Parser &P, Identifier &Name, DeclKind Kind)
-      : P(P), Name(Name), SF(nullptr) {
+    DebuggerContextChange(Parser &P, Identifier Name, DeclKind Kind)
+        : P(P), SF(nullptr) {
       if (!inDebuggerContext())
         return;
-      bool globalize = false;
-        
-      DebuggerClient *debug_client = getDebuggerClient();
-      if (!debug_client)
-        return;
-      
-      globalize = debug_client->shouldGlobalize(Name, Kind);
-        
-      if (globalize)
-        switchContext();
+
+      if (auto *client = getDebuggerClient())
+        if (client->shouldGlobalize(Name, Kind))
+          switchContext();
     }
 
     bool movedToTopLevel() {
@@ -118,19 +104,21 @@ namespace {
     template <typename T>
     ParserResult<T>
     fixupParserResult(T *D) {
-      if (CC.hasValue()) {
-        swapDecl(D);
+      if (movedToTopLevel()) {
+        D->setHoisted();
+        SF->addHoistedDecl(D);
+        getDebuggerClient()->didGlobalize(D);
       }
       return ParserResult<T>(D);
     }
 
     template <typename T>
     ParserResult<T>
     fixupParserResult(ParserStatus Status, T *D) {
-      if (CC.hasValue() && !Status.isError()) {
-        // If there is an error, don't do our splicing trick,
-        // just return the Decl and the status for reporting.
-        swapDecl(D);
+      if (movedToTopLevel()) {
+        D->setHoisted();
+        SF->addHoistedDecl(D);
+        getDebuggerClient()->didGlobalize(D);
       }
       return makeParserResult(Status, D);
     }
@@ -140,43 +128,29 @@ namespace {
     ~DebuggerContextChange () {}
   protected:
 
-    DebuggerClient *getDebuggerClient()
-    {
-      ModuleDecl *PM = P.CurDeclContext->getParentModule();
-      if (!PM)
-          return nullptr;
-      else
-           return PM->getDebugClient();
+    DebuggerClient *getDebuggerClient() {
+      ModuleDecl *M = P.CurDeclContext->getParentModule();
+      return M->getDebugClient();
     }
 
     bool inDebuggerContext() {
       if (!P.Context.LangOpts.DebuggerSupport)
         return false;
       if (!P.CurDeclContext)
         return false;
-      auto *func_decl = dyn_cast<FuncDecl>(P.CurDeclContext);
-      if (!func_decl)
+      auto *func = dyn_cast<FuncDecl>(P.CurDeclContext);
+      if (!func)
         return false;
-        
-      if (!func_decl->getAttrs().hasAttribute<LLDBDebuggerFunctionAttr>())
+
+      if (!func->getAttrs().hasAttribute<LLDBDebuggerFunctionAttr>())
         return false;
-      
+
       return true;
     }
 
-    void switchContext () {
+    void switchContext() {
       SF = P.CurDeclContext->getParentSourceFile();
-      CC.emplace (P, SF);
-    }
-    
-    void swapDecl (Decl *D)
-    {
-      assert (SF);
-      DebuggerClient *debug_client = getDebuggerClient();
-      assert (debug_client);
-      debug_client->didGlobalize(D);
-      P.ContextSwitchedTopLevelDecls.push_back(D);
-      P.markWasHandled(D);
+      CC.emplace(P, SF);
     }
   };
 } // end anonymous namespace
@@ -225,10 +199,6 @@ void Parser::parseTopLevel(SmallVectorImpl<Decl *> &decls) {
     }
   }
 
-  // First append any decls that LLDB requires be inserted at the top-level.
-  decls.append(ContextSwitchedTopLevelDecls.begin(),
-               ContextSwitchedTopLevelDecls.end());
-
   // Then append the top-level decls we parsed.
   for (auto item : items) {
     auto *decl = item.get<Decl *>();
@@ -7726,8 +7696,9 @@ Parser::parseDeclPrecedenceGroup(ParseDeclOptions flags,
   SourceLoc precedenceGroupLoc = consumeToken(tok::kw_precedencegroup);
   DebuggerContextChange DCC (*this);
 
-  if (!CodeCompletion && !DCC.movedToTopLevel() && !(flags & PD_AllowTopLevel))
-  {
+  if (!CodeCompletion &&
+      !DCC.movedToTopLevel() &&
+      !(flags & PD_AllowTopLevel)) {
     diagnose(precedenceGroupLoc, diag::decl_inner_scope);
     return nullptr;
   }

lib/SILGen/SILGen.cpp

@@ -1889,6 +1889,12 @@ class SILGenModuleRAII {
       SGM.visit(D);
     }
 
+    for (Decl *D : sf->getHoistedDecls()) {
+      FrontendStatsTracer StatsTracer(SGM.getASTContext().Stats,
+                                      "SILgen-decl", D);
+      SGM.visit(D);
+    }
+
     for (TypeDecl *TD : sf->LocalTypeDecls) {
       FrontendStatsTracer StatsTracer(SGM.getASTContext().Stats,
                                       "SILgen-tydecl", TD);

lib/SILGen/SILGenStmt.cpp

@@ -346,7 +346,13 @@ void StmtEmitter::visitBraceStmt(BraceStmt *S) {
     } else if (auto *E = ESD.dyn_cast<Expr*>()) {
       SGF.emitIgnoredExpr(E);
     } else {
-      SGF.visit(ESD.get<Decl*>());
+      auto *D = ESD.get<Decl*>();
+
+      // Hoisted declarations are emitted at the top level by emitSourceFile().
+      if (D->isHoisted())
+        continue;
+
+      SGF.visit(D);
     }
   }
 }

lib/Sema/ImportResolution.cpp

@@ -298,6 +298,8 @@ void swift::performImportResolution(SourceFile &SF) {
   // Resolve each import declaration.
   for (auto D : SF.getTopLevelDecls())
     resolver.visit(D);
+  for (auto D : SF.getHoistedDecls())
+    resolver.visit(D);
 
   SF.setImports(resolver.getFinishedImports());
 

lib/Sema/TypeChecker.cpp

@@ -215,11 +215,17 @@ void swift::bindExtensions(ModuleDecl &mod) {
     if (!SF)
       continue;
 
-    for (auto D : SF->getTopLevelDecls()) {
+    auto visitTopLevelDecl = [&](Decl *D) {
       if (auto ED = dyn_cast<ExtensionDecl>(D))
         if (!tryBindExtension(ED))
-          worklist.push_back(ED);
-    }
+          worklist.push_back(ED);;
+    };
+
+    for (auto *D : SF->getTopLevelDecls())
+      visitTopLevelDecl(D);
+
+    for (auto *D : SF->getHoistedDecls())
+      visitTopLevelDecl(D);
   }
 
   // Phase 2 - repeatedly go through the worklist and attempt to bind each