Parse: Preserve source order when code completion adds delayed declarations

Author: slavapestov

include/swift/AST/DeclContext.h

@@ -782,9 +782,20 @@ class IterableDeclContext {
   /// abstractions on top of member loading, such as a name lookup table.
   DeclRange getCurrentMembersWithoutLoading() const;
 
-  /// Add a member to this context. If the hint decl is specified, the new decl
-  /// is inserted immediately after the hint.
-  void addMember(Decl *member, Decl *hint = nullptr);
+  /// Add a member to this context.
+  ///
+  /// If the hint decl is specified, the new decl is inserted immediately
+  /// after the hint.
+  ///
+  /// If insertAtHead is set, the new decl is inserted at the beginning of
+  /// the list.
+  ///
+  /// Otherwise, it is inserted at the end.
+  void addMember(Decl *member, Decl *hint = nullptr, bool insertAtHead = false);
+
+  /// Add a member in the right place to preserve source order. This should
+  /// only be called from the code completion delayed parsing path.
+  void addMemberPreservingSourceOrder(Decl *member);
 
   /// Check whether there are lazily-loaded members.
   bool hasLazyMembers() const {
@@ -862,10 +873,7 @@ class IterableDeclContext {
 private:
   /// Add a member to the list for iteration purposes, but do not notify the
   /// subclass that we have done so.
-  ///
-  /// This is used internally when loading members, because loading a
-  /// member is an invisible addition.
-  void addMemberSilently(Decl *member, Decl *hint = nullptr) const;
+  void addMemberSilently(Decl *member, Decl *hint, bool insertAtHead) const;
 };
 
 /// Define simple_display for DeclContexts but not for subclasses in order to

lib/AST/DeclContext.cpp

@@ -764,10 +764,33 @@ ArrayRef<Decl *> IterableDeclContext::getSemanticMembers() const {
       ArrayRef<Decl *>());
 }
 
+void IterableDeclContext::addMemberPreservingSourceOrder(Decl *member) {
+  auto &SM = getASTContext().SourceMgr;
+
+  SourceLoc start = member->getStartLoc();
+  Decl *hint = nullptr;
+
+  for (auto *existingMember : getMembers()) {
+    if (existingMember->isImplicit())
+      continue;
+
+    if (isa<EnumCaseDecl>(existingMember) ||
+        isa<IfConfigDecl>(existingMember))
+      continue;
+
+    if (!SM.isBeforeInBuffer(existingMember->getEndLoc(), start))
+      break;
+
+    hint = existingMember;
+  }
+
+  addMember(member, hint, /*insertAtHead=*/hint == nullptr);
+}
+
 /// Add a member to this context.
-void IterableDeclContext::addMember(Decl *member, Decl *Hint) {
+void IterableDeclContext::addMember(Decl *member, Decl *hint, bool insertAtHead) {
   // Add the member to the list of declarations without notification.
-  addMemberSilently(member, Hint);
+  addMemberSilently(member, hint, insertAtHead);
 
   // Notify our parent declaration that we have added the member, which can
   // be used to update the lookup tables.
@@ -790,29 +813,83 @@ void IterableDeclContext::addMember(Decl *member, Decl *Hint) {
   }
 }
 
-void IterableDeclContext::addMemberSilently(Decl *member, Decl *hint) const {
+void IterableDeclContext::addMemberSilently(Decl *member, Decl *hint,
+                                            bool insertAtHead) const {
   assert(!isa<AccessorDecl>(member) && "Accessors should not be added here");
   assert(!member->NextDecl && "Already added to a container");
 
-  // If there is a hint decl that specifies where to add this, just
-  // link into the chain immediately following it.
-  if (hint) {
+#ifndef NDEBUG
+  auto checkSourceRange = [&](Decl *prev, Decl *next) {
+    if (!member->getDeclContext()->getParentSourceFile())
+      return;
+
+    auto shouldSkip = [](Decl *d) {
+      if (isa<VarDecl>(d) || isa<EnumElementDecl>(d) || isa<IfConfigDecl>(d))
+        return true;
+
+      if (d->isImplicit())
+        return true;
+
+      return false;
+    };
+
+    if (shouldSkip(prev) || shouldSkip(next))
+      return;
+
+    SourceLoc prevEnd = prev->getEndLoc();
+    SourceLoc nextStart = next->getStartLoc();
+
+    if (!prevEnd.isValid() || !nextStart.isValid())
+      return;
+
+    if (getASTContext().SourceMgr.isBeforeInBuffer(prevEnd, nextStart))
+      return;
+
+    llvm::errs() << "Source ranges out of order in addMember():\n";
+    prev->dump(llvm::errs());
+    next->dump(llvm::errs());
+    abort();
+  };
+#endif
+
+  // Empty list.
+  if (!FirstDeclAndLazyMembers.getPointer()) {
+    assert(hint == nullptr);
+
+    FirstDeclAndLazyMembers.setPointer(member);
+    LastDeclAndKind.setPointer(member);
+
+  // Insertion at the head.
+  } else if (insertAtHead) {
+    assert(hint == nullptr);
+
+    member->NextDecl = FirstDeclAndLazyMembers.getPointer();
+    FirstDeclAndLazyMembers.setPointer(member);
+
+  // Insertion at the tail.
+  } else if (hint == nullptr) {
+    auto *last = LastDeclAndKind.getPointer();
+
+#ifndef NDEBUG
+    checkSourceRange(last, member);
+#endif
+
+    last->NextDecl = member;
+    LastDeclAndKind.setPointer(member);
+
+  // Insertion after 'hint' (which may be the tail).
+  } else {
+#ifndef NDEBUG
+    checkSourceRange(hint, member);
+#endif
+
     member->NextDecl = hint->NextDecl;
     hint->NextDecl = member;
 
-    // If the hint was the last in the parent context's chain, update it.
+    // Handle case where the 'hint' is the tail.
     if (LastDeclAndKind.getPointer() == hint)
       LastDeclAndKind.setPointer(member);
-    return;
-  }
-
-  if (auto last = LastDeclAndKind.getPointer()) {
-    last->NextDecl = member;
-    assert(last != member && "Simple cycle in decl list");
-  } else {
-    FirstDeclAndLazyMembers.setPointer(member);
   }
-  LastDeclAndKind.setPointer(member);
 }
 
 void IterableDeclContext::setMemberLoader(LazyMemberLoader *loader,

lib/Parse/Parser.cpp

@@ -189,9 +189,9 @@ void Parser::performCodeCompletionSecondPassImpl(
     parseDecl(ParseDeclOptions(info.Flags),
               /*IsAtStartOfLineOrPreviousHadSemi=*/true, [&](Decl *D) {
                 if (auto *NTD = dyn_cast<NominalTypeDecl>(DC)) {
-                  NTD->addMember(D);
+                  NTD->addMemberPreservingSourceOrder(D);
                 } else if (auto *ED = dyn_cast<ExtensionDecl>(DC)) {
-                  ED->addMember(D);
+                  ED->addMemberPreservingSourceOrder(D);
                 } else if (auto *SF = dyn_cast<SourceFile>(DC)) {
                   SF->addTopLevelDecl(D);
                 } else {