Parse: Mark declarations as hoisted in DebuggerContextChange

Expression evaluation in lldb wraps the entire user-written expression
in a new function body, which puts any new declarations written by the
user in local context.

There is a mechanism where declarations can get moved to the top level,
if they're only valid at the top level (imports, extensions etc), or
if the name of the declaration begins with '$'. This mechanism used to
actually add the declaration to the SourceFile's TopLevelDecls list,
which would break ASTScope invariants about source ranges being
monotonically increasing and non-overlapping.

Instead, we use the new 'hoisted' flag to mark the declarations as
hoisted, which leaves them syntactically in their original location
in the AST, but treats them as top level in SILGen and IRGen.

Part of <rdar://problem/53971116>.

Author: slavapestov

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/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;
   }