[ASTScope] Re-enable checkSourceRangeBeforeAddingChild

include/swift/AST/ASTScope.h

@@ -239,6 +239,9 @@ class ASTScopeImpl : public ASTAllocated<ASTScopeImpl> {
   NullablePtr<Stmt> getStmtIfAny() const;
   NullablePtr<Expr> getExprIfAny() const;
 
+  /// Whether this scope is for a decl attribute.
+  bool isDeclAttribute() const;
+
 #pragma mark - debugging and printing
 
 public:
@@ -264,7 +267,9 @@ class ASTScopeImpl : public ASTAllocated<ASTScopeImpl> {
   void dumpOneScopeMapLocation(std::pair<unsigned, unsigned> lineColumn);
 
 private:
-  llvm::raw_ostream &verificationError() const;
+  [[noreturn]]
+  void abortWithVerificationError(
+      llvm::function_ref<void(llvm::raw_ostream &)> messageFn) const;
 
 #pragma mark - Scope tree creation
 public:

lib/AST/ASTScope.cpp

@@ -297,6 +297,20 @@ NullablePtr<Expr> ASTScopeImpl::getExprIfAny() const {
   }
 }
 
+bool ASTScopeImpl::isDeclAttribute() const {
+  switch (getKind()) {
+#define DECL_ATTRIBUTE_SCOPE_NODE(Name) \
+    case ScopeKind::Name: return true;
+#define SCOPE_NODE(Name)
+#include "swift/AST/ASTScopeNodes.def"
+
+#define DECL_ATTRIBUTE_SCOPE_NODE(Name)
+#define SCOPE_NODE(Name) case ScopeKind::Name:
+#include "swift/AST/ASTScopeNodes.def"
+      return false;
+  }
+}
+
 SourceManager &ASTScopeImpl::getSourceManager() const {
   return getASTContext().SourceMgr;
 }

lib/AST/ASTScopeCreation.cpp

@@ -610,6 +610,12 @@ ASTScopeImpl *ScopeCreator::addToScopeTreeAndReturnInsertionPoint(
 void ScopeCreator::addChildrenForParsedAccessors(
     AbstractStorageDecl *asd, ASTScopeImpl *parent) {
   asd->visitParsedAccessors([&](AccessorDecl *ad) {
+    // Ignore accessors added by macro expansions.
+    // TODO: This ought to be the default behavior of `visitParsedAccessors`,
+    // we ought to have a different entrypoint for clients that care about
+    // the semantic set of "explicit" accessors.
+    if (ad->isInMacroExpansionInContext())
+      return;
     assert(asd == ad->getStorage());
     this->addToScopeTree(ad, parent);
   });
@@ -666,11 +672,16 @@ ScopeCreator::addPatternBindingToScopeTree(PatternBindingDecl *patternBinding,
   if (auto *var = patternBinding->getSingleVar())
     addChildrenForKnownAttributes(var, parentScope);
 
+  // Check to see if we have a local binding. Note we need to exclude bindings
+  // in `@abi` attributes here since they may be in a local DeclContext, but
+  // their scope shouldn't extend past the attribute.
   bool isLocalBinding = false;
-  for (auto i : range(patternBinding->getNumPatternEntries())) {
-    if (auto *varDecl = patternBinding->getAnchoringVarDecl(i)) {
-      isLocalBinding = varDecl->getDeclContext()->isLocalContext();
-      break;
+  if (!isa<ABIAttributeScope>(parentScope)) {
+    for (auto i : range(patternBinding->getNumPatternEntries())) {
+      if (auto *varDecl = patternBinding->getAnchoringVarDecl(i)) {
+        isLocalBinding = varDecl->getDeclContext()->isLocalContext();
+        break;
+      }
     }
   }
 
@@ -703,7 +714,7 @@ void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) {
   child->parentAndWasExpanded.setPointer(this);
 
 #ifndef NDEBUG
-  // checkSourceRangeBeforeAddingChild(child, ctx);
+  checkSourceRangeBeforeAddingChild(child, ctx);
 #endif
 
   // If this is the first time we've added children, notify the ASTContext

lib/AST/ASTScopePrinting.cpp

@@ -69,9 +69,17 @@ void ASTScopeImpl::dumpOneScopeMapLocation(
   }
 }
 
-llvm::raw_ostream &ASTScopeImpl::verificationError() const {
-  return llvm::errs() << "ASTScopeImpl verification error in source file '"
-                      << getSourceFile()->getFilename() << "': ";
+void ASTScopeImpl::abortWithVerificationError(
+    llvm::function_ref<void(llvm::raw_ostream &)> messageFn) const {
+  llvm::SmallString<0> errorStr;
+  llvm::raw_svector_ostream out(errorStr);
+
+  out << "ASTScopeImpl verification error in source file '"
+      << getSourceFile()->getFilename() << "':\n";
+  messageFn(out);
+
+  llvm::PrettyStackTraceString trace(errorStr.c_str());
+  abort();
 }
 
 #pragma mark printing

lib/AST/ASTScopeSourceRange.cpp

@@ -39,16 +39,15 @@ using namespace ast_scope;
 
 static SourceLoc getLocAfterExtendedNominal(const ExtensionDecl *);
 
-/// Retrieve the character-based source range for the given source range.
-static SourceRange getCharSourceRange(
-    SourceManager &sourceMgr, SourceRange range
-){
-  range.End = Lexer::getLocForEndOfToken(sourceMgr, range.End);
-  return range;
-}
-
 void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
                                                      const ASTContext &ctx) const {
+  // Ignore attributes on extensions, currently they exist outside of the
+  // extension's source range due to the way we've setup the scope for
+  // extension binding.
+  // FIXME: We ought to fix the source range for extension scopes.
+  if (isa<ExtensionScope>(this) && child->isDeclAttribute())
+    return;
+
   // Ignore debugger bindings - they're a special mix of user code and implicit
   // wrapper code that is too difficult to check for consistency.
   if (auto d = getDeclIfAny().getPtrOrNull())
@@ -60,14 +59,14 @@ void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
 
   auto range = getCharSourceRangeOfScope(sourceMgr);
 
-  std::function<bool(SourceRange)> containedInParent;
-  containedInParent = [&](SourceRange childCharRange) {
+  auto containedInParent = [&](SourceRange childCharRange) {
     // HACK: For code completion. Handle replaced range.
+    // Note that the replaced SourceRanges here are already disguised
+    // CharSourceRanges, we don't need to adjust them. We use `rangeContains`
+    // since we're only interested in comparing within a single buffer.
     for (const auto &pair : sourceMgr.getReplacedRanges()) {
-      auto originalRange = getCharSourceRange(sourceMgr, pair.first);
-      auto newRange = getCharSourceRange(sourceMgr, pair.second);
-      if (sourceMgr.encloses(range, originalRange) &&
-          sourceMgr.encloses(newRange, childCharRange))
+      if (sourceMgr.rangeContains(range, pair.first) &&
+          sourceMgr.rangeContains(pair.second, childCharRange))
         return true;
     }
 
@@ -77,11 +76,12 @@ void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
   auto childCharRange = child->getCharSourceRangeOfScope(sourceMgr);
 
   if (!containedInParent(childCharRange)) {
-    auto &out = verificationError() << "child not contained in its parent:\n";
-    child->print(out);
-    out << "\n***Parent node***\n";
-    this->print(out);
-    abort();
+    abortWithVerificationError([&](llvm::raw_ostream &out) {
+      out << "child not contained in its parent:\n";
+      child->print(out);
+      out << "\n***Parent node***\n";
+      this->print(out);
+    });
   }
 
   if (!storedChildren.empty()) {
@@ -90,13 +90,14 @@ void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
         sourceMgr).End;
 
     if (!sourceMgr.isAtOrBefore(endOfPreviousChild, childCharRange.Start)) {
-      auto &out = verificationError() << "child overlaps previous child:\n";
-      child->print(out);
-      out << "\n***Previous child\n";
-      previousChild->print(out);
-      out << "\n***Parent node***\n";
-      this->print(out);
-      abort();
+      abortWithVerificationError([&](llvm::raw_ostream &out) {
+        out << "child overlaps previous child:\n";
+        child->print(out);
+        out << "\n***Previous child\n";
+        previousChild->print(out);
+        out << "\n***Parent node***\n";
+        this->print(out);
+      });
     }
   }
 }

lib/AST/Decl.cpp

@@ -1007,11 +1007,21 @@ SourceRange Decl::getSourceRangeIncludingAttrs() const {
 }
 
 bool Decl::isInMacroExpansionInContext() const {
-  auto *dc = getDeclContext();
-  auto parentFile = dc->getParentSourceFile();
   auto *mod = getModuleContext();
   auto *file = mod->getSourceFileContainingLocation(getStartLoc());
 
+  auto parentFile = [&]() {
+    // For accessors, the storage decl is the more accurate thing to check
+    // since the entire property/subscript could be macro-generated, in which
+    // case the accessor shouldn't be considered "added by macro expansion".
+    if (auto *accessor = dyn_cast<AccessorDecl>(this)) {
+      auto storageLoc = accessor->getStorage()->getLoc();
+      if (storageLoc.isValid())
+        return mod->getSourceFileContainingLocation(storageLoc);
+    }
+    return getDeclContext()->getParentSourceFile();
+  }();
+
   // Decls in macro expansions always have a source file. The source
   // file can be null if the decl is implicit or has an invalid
   // source location.

lib/Basic/SourceLoc.cpp

@@ -836,11 +836,11 @@ static bool isBeforeInSource(
   SourceLoc firstLocInLCA = firstMismatch == firstAncestors.end()
       ? firstLoc
       : sourceMgr.getGeneratedSourceInfo(*firstMismatch)
-          ->originalSourceRange.getEnd();
+          ->originalSourceRange.getStart();
   SourceLoc secondLocInLCA = secondMismatch == secondAncestors.end()
       ? secondLoc
       : sourceMgr.getGeneratedSourceInfo(*secondMismatch)
-          ->originalSourceRange.getEnd();
+          ->originalSourceRange.getStart();
   return sourceMgr.isBeforeInBuffer(firstLocInLCA, secondLocInLCA) ||
     (allowEqual && firstLocInLCA == secondLocInLCA);
 }

lib/Sema/TypeCheckMacros.cpp

@@ -984,6 +984,12 @@ static CharSourceRange getExpansionInsertionRange(MacroRole role,
                                  closure->getEndLoc()));
     }
 
+    // If the function has a body, that's what's being replaced.
+    auto *AFD = cast<AbstractFunctionDecl>(target.get<Decl *>());
+    if (auto range = AFD->getBodySourceRange())
+      return Lexer::getCharSourceRangeFromSourceRange(sourceMgr, range);
+
+    // Otherwise we have no body, just use the end of the decl.
     SourceLoc afterDeclLoc =
         Lexer::getLocForEndOfToken(sourceMgr, target.getEndLoc());
     return CharSourceRange(afterDeclLoc, 0);