Merge pull request #33976 from slavapestov/remove-astscope-starting-dc-hack

Remove ASTScope starting DeclContext hack

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -411,8 +411,7 @@ class ASTScopeImpl {
 
   /// Entry point into ASTScopeImpl-land for lookups
   static llvm::SmallVector<const ASTScopeImpl *, 0>
-  unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc,
-                    const DeclContext *startingContext, DeclConsumer);
+  unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc, DeclConsumer);
 
   /// Entry point into ASTScopeImpl-land for labeled statement lookups.
   static llvm::SmallVector<LabeledStmt *, 4>
@@ -429,11 +428,7 @@ class ASTScopeImpl {
 private:
   static const ASTScopeImpl *findStartingScopeForLookup(SourceFile *,
                                                         const DeclNameRef name,
-                                                        const SourceLoc where,
-                                                        const DeclContext *ctx);
-
-protected:
-  virtual bool doesContextMatchStartingContext(const DeclContext *) const;
+                                                        const SourceLoc where);
 
 protected:
   /// Not const because may reexpand some scopes.
@@ -1006,7 +1001,6 @@ class GenericParamScope final : public ASTScopeImpl {
 protected:
   bool lookupLocalsOrMembers(ArrayRef<const ASTScopeImpl *>,
                              DeclConsumer) const override;
-  bool doesContextMatchStartingContext(const DeclContext *) const override;
   Optional<bool>
   resolveIsCascadingUseForThisScope(Optional<bool>) const override;
 };
@@ -1622,7 +1616,6 @@ class DifferentiableAttributeScope final : public ASTScopeImpl {
   ASTScopeImpl *expandSpecifically(ScopeCreator &) override;
   bool lookupLocalsOrMembers(ArrayRef<const ASTScopeImpl *>,
                              DeclConsumer) const override;
-  bool doesContextMatchStartingContext(const DeclContext *) const override;
 };
 
 class SubscriptDeclScope final : public ASTScopeImpl {

include/swift/AST/NameLookup.h

@@ -691,7 +691,6 @@ class ASTScope {
   /// \return the scopes traversed
   static llvm::SmallVector<const ast_scope::ASTScopeImpl *, 0>
   unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc,
-                    const DeclContext *startingContext,
                     namelookup::AbstractASTScopeDeclConsumer &);
 
   static Optional<bool>

lib/AST/ASTScope.cpp

@@ -40,12 +40,10 @@ using namespace ast_scope;
 
 llvm::SmallVector<const ASTScopeImpl *, 0> ASTScope::unqualifiedLookup(
     SourceFile *SF, DeclNameRef name, SourceLoc loc,
-    const DeclContext *startingContext,
     namelookup::AbstractASTScopeDeclConsumer &consumer) {
   if (auto *s = SF->getASTContext().Stats)
     ++s->getFrontendCounters().NumASTScopeLookups;
-  return ASTScopeImpl::unqualifiedLookup(SF, name, loc, startingContext,
-                                         consumer);
+  return ASTScopeImpl::unqualifiedLookup(SF, name, loc, consumer);
 }
 
 Optional<bool> ASTScope::computeIsCascadingUse(

lib/AST/ASTScopeLookup.cpp

@@ -35,28 +35,19 @@ using namespace swift;
 using namespace namelookup;
 using namespace ast_scope;
 
-static bool isLocWithinAnInactiveClause(const SourceLoc loc, SourceFile *SF);
-
 llvm::SmallVector<const ASTScopeImpl *, 0> ASTScopeImpl::unqualifiedLookup(
     SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc,
-    const DeclContext *const startingContext, DeclConsumer consumer) {
+    DeclConsumer consumer) {
   SmallVector<const ASTScopeImpl *, 0> history;
   const auto *start =
-      findStartingScopeForLookup(sourceFile, name, loc, startingContext);
+      findStartingScopeForLookup(sourceFile, name, loc);
   if (start)
     start->lookup(history, nullptr, nullptr, consumer);
   return history;
 }
 
 const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
-    SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc,
-    const DeclContext *const startingContext) {
-  // At present, use legacy code in unqualifiedLookup.cpp to handle module-level
-  // lookups
-  // TODO: implement module scope someday
-  if (startingContext->getContextKind() == DeclContextKind::Module)
-    return nullptr;
-
+    SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc) {
   auto *const fileScope = sourceFile->getScope().impl;
   // Parser may have added decls to source file, since previous lookup
   if (name.isOperator())
@@ -66,63 +57,7 @@ const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
   ASTScopeAssert(innermost->getWasExpanded(),
                  "If looking in a scope, it must have been expanded.");
 
-  // The legacy lookup code gets passed both a SourceLoc and a starting context.
-  // However, our ultimate intent is for clients to not have to pass in a
-  // DeclContext at all, since the SourceLoc should be enough. While we are
-  // debugging the new ASTScope lookup code, we can catch bugs by comparing the
-  // DeclContext of the ASTScope found from the desired SourceLoc to the
-  // DeclContext passed in by the client.
-
-  const auto *startingScope = innermost;
-  for (; startingScope &&
-         !startingScope->doesContextMatchStartingContext(startingContext);
-       startingScope = startingScope->getParent().getPtrOrNull()) {
-  }
-  // Someday, just use the assertion below. For now, print out lots of info for
-  // debugging.
-  if (!startingScope) {
-
-    // Be lenient in code completion mode. There are cases where the decl
-    // context doesn't match with the ASTScope. e.g. dangling attributes.
-    // FIXME: Create ASTScope tree even for invalid code.
-    if (innermost &&
-        startingContext->getASTContext().SourceMgr.hasCodeCompletionBuffer()) {
-      return innermost;
-    }
-
-    llvm::errs() << "ASTScopeImpl: resorting to startingScope hack, file: "
-                 << sourceFile->getFilename() << "\n";
-    // The check is costly, and inactive lookups will end up here, so don't
-    // do the check unless we can't find the startingScope.
-    const bool isInInactiveClause =
-        isLocWithinAnInactiveClause(loc, sourceFile);
-    if (isInInactiveClause)
-      llvm::errs() << "  because location is within an inactive clause\n";
-    llvm::errs() << "'";
-    name.print(llvm::errs());
-    llvm::errs() << "' ";
-    llvm::errs() << "loc: ";
-    loc.print(llvm::errs(), sourceFile->getASTContext().SourceMgr);
-    llvm::errs() << "\nstarting context:\n ";
-    startingContext->printContext(llvm::errs());
-    //    llvm::errs() << "\ninnermost: ";
-    //    innermost->dump();
-    //    llvm::errs() << "in: \n";
-    //    fileScope->dump();
-    llvm::errs() << "\n\n";
-
-    // Might distort things
-    //    if (fileScope->crossCheckWithAST())
-    //      llvm::errs() << "Tree creation missed some DeclContexts.\n";
-
-    // Crash compilation even if NDEBUG
-    if (isInInactiveClause)
-      llvm::report_fatal_error(
-          "A lookup was attempted into an inactive clause");
-  }
-
-  ASTScopeAssert(startingScope, "ASTScopeImpl: could not find startingScope");
-  return startingScope;
+  return innermost;
 }
 
 ASTScopeImpl *
@@ -189,53 +124,6 @@ ASTScopeImpl::findChildContaining(SourceLoc loc,
   return nullptr;
 }
 
-#pragma mark doesContextMatchStartingContext
-// Match existing UnqualifiedLookupBehavior
-
-bool ASTScopeImpl::doesContextMatchStartingContext(
-    const DeclContext *context) const {
-  // Why are we not checking the loc for this--because already did binary search
-  // on loc to find the start First, try MY DeclContext
-  if (auto myDCForL = getDeclContext())
-    return myDCForL == context;
-  // If I don't have one, ask my parent.
-  // (Choose innermost scope with matching loc & context.)
-  if (auto p = getParent())
-    return p.get()->doesContextMatchStartingContext(context);
-  // Topmost scope always has a context, the SourceFile.
-  ASTScope_unreachable("topmost scope always has a context, the SourceFile");
-}
-
-// For a SubscriptDecl with generic parameters, the call tries to do lookups
-// with startingContext equal to either the get or set subscript
-// AbstractFunctionDecls. Since the generic parameters are in the
-// SubscriptDeclScope, and not the AbstractFunctionDecl scopes (after all how
-// could one parameter be in two scopes?), GenericParamScope intercepts the
-// match query here and tests against the accessor DeclContexts.
-bool GenericParamScope::doesContextMatchStartingContext(
-    const DeclContext *context) const {
-  if (auto *asd = dyn_cast<AbstractStorageDecl>(holder)) {
-    for (auto accessor : asd->getAllAccessors()) {
-      if (up_cast<DeclContext>(accessor) == context)
-        return true;
-    }
-  }
-  return false;
-}
-
-bool DifferentiableAttributeScope::doesContextMatchStartingContext(
-    const DeclContext *context) const {
-  // Need special logic to handle case where `attributedDeclaration` is an
-  // `AbstractStorageDecl` (`SubscriptDecl` or `VarDecl`). The initial starting
-  // context in `ASTScopeImpl::findStartingScopeForLookup` will be an accessor
-  // of the `attributedDeclaration`.
-  if (auto *asd = dyn_cast<AbstractStorageDecl>(attributedDeclaration))
-    for (auto accessor : asd->getAllAccessors())
-      if (up_cast<DeclContext>(accessor) == context)
-        return true;
-  return false;
-}
-
 #pragma mark lookup methods that run once per scope
 
 void ASTScopeImpl::lookup(SmallVectorImpl<const ASTScopeImpl *> &history,
@@ -841,39 +729,6 @@ Optional<bool> PatternEntryInitializerScope::resolveIsCascadingUseForThisScope(
   return isCascadingUse;
 }
 
-bool isLocWithinAnInactiveClause(const SourceLoc loc, SourceFile *SF) {
-  class InactiveClauseTester : public ASTWalker {
-    const SourceLoc loc;
-    const SourceManager &SM;
-
-  public:
-    bool wasFoundWithinInactiveClause = false;
-
-    InactiveClauseTester(const SourceLoc loc, const SourceManager &SM)
-        : loc(loc), SM(SM) {}
-
-    bool walkToDeclPre(Decl *D) override {
-      if (const auto *ifc = dyn_cast<IfConfigDecl>(D)) {
-        for (const auto &clause : ifc->getClauses()) {
-          if (clause.isActive)
-            continue;
-          for (const auto &n : clause.Elements) {
-            SourceRange sr = n.getSourceRange();
-            if (sr.isValid() && SM.rangeContainsTokenLoc(sr, loc)) {
-              wasFoundWithinInactiveClause = true;
-              return false;
-            }
-          }
-        }
-      }
-      return ASTWalker::walkToDeclPre(D);
-    }
-  };
-  InactiveClauseTester tester(loc, SF->getASTContext().SourceMgr);
-  SF->walk(tester);
-  return tester.wasFoundWithinInactiveClause;
-}
-
 #pragma mark isLabeledStmtLookupTerminator implementations
 bool ASTScopeImpl::isLabeledStmtLookupTerminator() const {
   return true;

lib/AST/ASTScopeSourceRange.cpp

@@ -456,8 +456,7 @@ SourceRange ClosureParametersScope::getSourceRangeOfThisASTNode(
   if (!omitAssertions)
     ASTScopeAssert(closureExpr->getInLoc().isValid(),
                    "We don't create these if no in loc");
-  return SourceRange(getStartOfFirstParam(closureExpr),
-                     closureExpr->getInLoc());
+  return SourceRange(closureExpr->getInLoc(), closureExpr->getEndLoc());
 }
 
 SourceRange

lib/AST/NameLookup.cpp

@@ -1946,7 +1946,33 @@ static DirectlyReferencedTypeDecls
 directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
                                          SourceLoc loc, DeclContext *dc,
                                          LookupOuterResults lookupOuter) {
+  // In a protocol or protocol extension, the 'where' clause can refer to
+  // associated types without 'Self' qualification:
+  //
+  // protocol MyProto where AssocType : Q { ... }
+  //
+  // extension MyProto where AssocType == Int { ... }
+  //
+  // For this reason, ASTScope maps source locations inside the 'where'
+  // clause to a scope that performs the lookup into the protocol or
+  // protocol extension.
+  //
+  // However, protocol and protocol extensions can also put bounds on 'Self',
+  // for example:
+  //
+  // protocol MyProto where Self : MyClass { ... }
+  //
+  // We must start searching for 'MyClass' at the top level, otherwise
+  // we end up with a cycle, because qualified lookup wants to resolve
+  // 'Self' bounds to build the set of declarations to search inside of.
+  //
+  // To make this work, we handle the top-level lookup case explicitly
+  // here, bypassing unqualified lookup and ASTScope altogether.
+  if (dc->isModuleScopeContext())
+    loc = SourceLoc();
+
   DirectlyReferencedTypeDecls results;
+
   UnqualifiedLookupOptions options =
       UnqualifiedLookupFlags::TypeLookup |
       UnqualifiedLookupFlags::AllowProtocolMembers;
@@ -1958,8 +1984,8 @@ directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
   auto lookup = evaluateOrDefault(ctx.evaluator,
                                   UnqualifiedLookupRequest{descriptor}, {});
   for (const auto &result : lookup.allResults()) {
-    if (auto typeDecl = dyn_cast<TypeDecl>(result.getValueDecl()))
-      results.push_back(typeDecl);
+    auto typeDecl = cast<TypeDecl>(result.getValueDecl());
+    results.push_back(typeDecl);
   }
 
   return results;

lib/AST/UnqualifiedLookup.cpp

@@ -544,7 +544,7 @@ void UnqualifiedLookupFactory::lookInASTScopes() {
 #endif
 
   ASTScope::unqualifiedLookup(DC->getParentSourceFile(),
-                              Name, Loc, DC, consumer);
+                              Name, Loc, consumer);
 }
 
 bool ASTScopeDeclConsumerForUnqualifiedLookup::consume(

lib/Sema/CSSimplify.cpp

@@ -7289,8 +7289,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
 
           auto descriptor = UnqualifiedLookupDescriptor(
               DeclNameRef(param->getName()),
-              paramDecl->getDeclContext()->getParentForLookup(),
-              paramDecl->getStartLoc(),
+              paramDecl->getDeclContext()->getParentSourceFile(),
+              SourceLoc(),
               UnqualifiedLookupFlags::KnownPrivate |
                   UnqualifiedLookupFlags::TypeLookup);
           auto lookup = evaluateOrDefault(