Merge pull request swiftlang#33805 from slavapestov/remove-enable-astscope-lookup-flag

Remove -enable-astscope-lookup flag

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -44,11 +44,10 @@
 /// try disabling it.
 /// \p message must be a string literal
 #define ASTScopeAssert(predicate, message)                                     \
-  assert((predicate) && message                                                \
-         " Try compiling with '-disable-astscope-lookup'.")
+  assert((predicate) && message)
 
 #define ASTScope_unreachable(message)                                          \
-  llvm_unreachable(message " Try compiling with '-disable-astscope-lookup'.")
+  llvm_unreachable(message)
 
 namespace swift {
 

include/swift/AST/SourceFile.h

@@ -236,9 +236,7 @@ class SourceFile final : public FileUnit {
   }
 
   /// Add a hoisted declaration. See Decl::isHoisted().
-  void addHoistedDecl(Decl *d) {
-    Hoisted.push_back(d);
-  }
+  void addHoistedDecl(Decl *d);
 
   /// Retrieves an immutable view of the list of top-level decls in this file.
   ArrayRef<Decl *> getTopLevelDecls() const;

include/swift/Basic/LangOptions.h

@@ -248,25 +248,12 @@ namespace swift {
     /// This is a staging flag; eventually it will be removed.
     bool EnableDeserializationRecovery = true;
 
-    /// Should we use \c ASTScope-based resolution for unqualified name lookup?
-    /// Default is in \c ParseLangArgs
-    ///
-    /// This is a staging flag; eventually it will be removed.
-    bool EnableASTScopeLookup = true;
-
     /// Someday, ASTScopeLookup will supplant lookup in the parser
     bool DisableParserLookup = false;
 
-    /// Should  we compare to ASTScope-based resolution for debugging?
-    bool CrosscheckUnqualifiedLookup = false;
-
     /// Should  we stress ASTScope-based resolution for debugging?
     bool StressASTScopeLookup = false;
 
-    /// Since some tests fail if the warning is output, use a flag to decide
-    /// whether it is. The warning is useful for testing.
-    bool WarnIfASTScopeLookup = false;
-
     /// Build the ASTScope tree lazily
     bool LazyASTScopes = true;
 

include/swift/Frontend/Frontend.h

@@ -328,11 +328,6 @@ class CompilerInvocation {
     return CodeCompletionOffset != ~0U;
   }
 
-  /// Called from lldb, see rdar://53971116
-  void disableASTScopeLookup() {
-    LangOpts.EnableASTScopeLookup = false;
-  }
-
   /// Retrieve a module hash string that is suitable for uniquely
   /// identifying the conditions under which the module was built, for use
   /// in generating a cached PCH file for the bridging header.

include/swift/Option/FrontendOptions.td

@@ -160,9 +160,6 @@ def crosscheck_unqualified_lookup : Flag<["-"], "crosscheck-unqualified-lookup">
 def stress_astscope_lookup : Flag<["-"], "stress-astscope-lookup">,
   HelpText<"Stress ASTScope-based unqualified name lookup (for testing)">;
 
-def warn_if_astscope_lookup : Flag<["-"], "warn-if-astscope-lookup">,
-  HelpText<"Print a warning if ASTScope lookup is used">;
-  
 def lazy_astscopes : Flag<["-"], "lazy-astscopes">,
   HelpText<"Build ASTScopes lazily">;
 

include/swift/Option/Options.td

@@ -1107,14 +1107,6 @@ def scan_clang_dependencies : Flag<["-"], "scan-clang-dependencies">,
   HelpText<"Scan dependencies of the given Clang module">, ModeOpt,
   Flags<[FrontendOption, NoInteractiveOption, DoesNotAffectIncrementalBuild]>;
 
-def enable_astscope_lookup : Flag<["-"], "enable-astscope-lookup">,
-  Flags<[FrontendOption]>,
-  HelpText<"Enable ASTScope-based unqualified name lookup">;
-
-def disable_astscope_lookup : Flag<["-"], "disable-astscope-lookup">,
-  Flags<[FrontendOption]>,
-  HelpText<"Disable ASTScope-based unqualified name lookup">;
-
 def disable_parser_lookup : Flag<["-"], "disable-parser-lookup">,
   Flags<[FrontendOption]>,
   HelpText<"Disable parser lookup & use ast scope lookup only (experimental)">;

lib/AST/ASTScopeCreation.cpp

@@ -1086,8 +1086,6 @@ void ASTScopeImpl::disownDescendants(ScopeCreator &scopeCreator) {
 
 ASTScopeImpl *
 ASTScopeImpl::expandAndBeCurrentDetectingRecursion(ScopeCreator &scopeCreator) {
-  assert(scopeCreator.getASTContext().LangOpts.EnableASTScopeLookup &&
-         "Should not be getting here if ASTScopes are disabled");
   return evaluateOrDefault(scopeCreator.getASTContext().evaluator,
                            ExpandASTScopeRequest{this, &scopeCreator}, nullptr);
 }

lib/AST/Module.cpp

@@ -2337,6 +2337,12 @@ bool SourceFile::hasDelayedBodyParsing() const {
   return true;
 }
 
+/// Add a hoisted declaration. See Decl::isHoisted().
+void SourceFile::addHoistedDecl(Decl *d) {
+  assert(d->isHoisted());
+  Hoisted.push_back(d);
+}
+
 ArrayRef<Decl *> SourceFile::getTopLevelDecls() const {
   auto &ctx = getASTContext();
   auto *mutableThis = const_cast<SourceFile *>(this);

lib/AST/UnqualifiedLookup.cpp

@@ -235,9 +235,6 @@ namespace {
 
     bool useASTScopesForLookup() const;
 
-    /// For testing, assume this lookup is enabled:
-    bool wouldUseASTScopesForLookupIfItWereEnabled() const;
-
     void lookUpTopLevelNamesInModuleScopeContext(DeclContext *);
 
     void lookInASTScopes();
@@ -399,13 +396,6 @@ namespace {
     void print(raw_ostream &OS) const;
     void printResults(raw_ostream &OS) const;
 
-    bool verifyEqualTo(const UnqualifiedLookupFactory &&, StringRef thisLabel,
-                       StringRef otherLabel) const;
-
-    /// Legacy lookup is wrong here; we should NOT find this symbol.
-    bool shouldDiffer() const;
-    StringRef getSourceFileName() const;
-
 #ifndef NDEBUG
     bool isTargetLookup() const;
     void stopForDebuggingIfStartingTargetLookup(bool isASTScopeLookup) const;
@@ -497,14 +487,7 @@ void UnqualifiedLookupFactory::performUnqualifiedLookup() {
 
   ContextAndUnresolvedIsCascadingUse contextAndIsCascadingUse{
       DC, initialIsCascadingUse};
-  const bool crosscheckUnqualifiedLookup =
-      Ctx.LangOpts.CrosscheckUnqualifiedLookup;
   if (useASTScopesForLookup()) {
-    static bool haveWarned = false;
-    if (!haveWarned && Ctx.LangOpts.WarnIfASTScopeLookup) {
-      haveWarned = true;
-      llvm::errs() << "WARNING: TRYING Scope exclusively\n";
-    }
     lookInASTScopes();
   } else {
 #ifndef NDEBUG
@@ -516,28 +499,6 @@ void UnqualifiedLookupFactory::performUnqualifiedLookup() {
     else
       lookupNamesIntroducedBy(contextAndIsCascadingUse, NULL);
   }
-
-  if (crosscheckUnqualifiedLookup &&
-      wouldUseASTScopesForLookupIfItWereEnabled()) {
-    ResultsVector results;
-    size_t indexOfFirstOuterResult = 0;
-    UnqualifiedLookupFactory altLookup(Name, DC, Loc, options, results,
-                                       indexOfFirstOuterResult);
-    if (!useASTScopesForLookup())
-      altLookup.lookInASTScopes();
-    else if (Name.isOperator())
-      altLookup.lookupOperatorInDeclContexts(contextAndIsCascadingUse);
-    else
-      altLookup.lookupNamesIntroducedBy(contextAndIsCascadingUse, NULL);
-
-    const auto *ASTScopeLabel = "ASTScope lookup";
-    const auto *contextLabel = "context-bsed lookup";
-    const auto *mainLabel =
-        useASTScopesForLookup() ? ASTScopeLabel : contextLabel;
-    const auto *alternateLabel =
-        useASTScopesForLookup() ? contextLabel : ASTScopeLabel;
-    assert(verifyEqualTo(std::move(altLookup), mainLabel, alternateLabel));
-  }
 }
 
 void UnqualifiedLookupFactory::lookUpTopLevelNamesInModuleScopeContext(
@@ -562,12 +523,6 @@ void UnqualifiedLookupFactory::lookUpTopLevelNamesInModuleScopeContext(
 }
 
 bool UnqualifiedLookupFactory::useASTScopesForLookup() const {
-  return Ctx.LangOpts.EnableASTScopeLookup &&
-         wouldUseASTScopesForLookupIfItWereEnabled();
-}
-
-bool UnqualifiedLookupFactory::wouldUseASTScopesForLookupIfItWereEnabled()
-    const {
   if (!Loc.isValid())
     return false;
   return (bool) DC->getParentSourceFile();
@@ -1335,124 +1290,6 @@ void UnqualifiedLookupFactory::print(raw_ostream &OS) const {
   OS << "\n";
 }
 
-#pragma mark debugging: output utilities for grepping
-
-static void writeLine(std::string s) {
-  llvm::errs() << "\n+-+-+-+-  " << s << "\n";
-}
-
-StringRef UnqualifiedLookupFactory::getSourceFileName() const {
-  return DC->getParentSourceFile()->getFilename();
-}
-
-static void writeFirstLine(const UnqualifiedLookupFactory &ul, llvm::Twine s) {
-  std::string line =
-      std::string("In file: ") + ul.getSourceFileName().str() + ", " + s.str();
-  writeLine(line);
-}
-
-static void writeInconsistent(const UnqualifiedLookupFactory &me,
-                              StringRef thisLabel,
-                              const UnqualifiedLookupFactory &other,
-                              StringRef otherLabel, llvm::Twine s) {
-  writeFirstLine(me, s);
-  other.print(llvm::errs());
-  llvm::errs() << "\n" << thisLabel << " Results:\n";
-  me.printResults(llvm::errs());
-  llvm::errs() << "\n" << otherLabel << " Results:\n";
-  other.printResults(llvm::errs());
-  me.printScopes(llvm::errs());
-}
-
-#pragma mark comparing results
-
-bool UnqualifiedLookupFactory::verifyEqualTo(
-    const UnqualifiedLookupFactory &&other, const StringRef thisLabel,
-    StringRef otherLabel) const {
-  if (shouldDiffer()) {
-     return true;
-  }
-  auto writeErr = [&](llvm::Twine s) {
-    writeInconsistent(*this, thisLabel, other, otherLabel, s);
-  };
-  if (Results.size() != other.Results.size()) {
-    writeErr(thisLabel + " found " + std::to_string(Results.size()) + " but " +
-             otherLabel + " found " + std::to_string(other.Results.size()));
-    assert(false && "mismatch in number of results");
-  }
-  for (size_t i : indices(Results)) {
-    const auto &e = Results[i];
-    const auto &oe = other.Results[i];
-    if (e.getValueDecl() != oe.getValueDecl()) {
-      // print_ast_tc_function_bodies.swift generic from subscript vs get fn
-      std::string a; llvm::raw_string_ostream as(a);
-      std::string b; llvm::raw_string_ostream bs(b);
-      e.getValueDecl()->print(as);
-      oe.getValueDecl()->print(bs);
-      if (a == b)
-        llvm::errs() << "ValueDecls differ but print same\n";
-      else {
-        writeErr(std::string( "ValueDecls differ at ") + std::to_string(i));
-        assert(false && "other lookup found different Decl");
-      }
-    }
-    if (e.getDeclContext() != oe.getDeclContext()) {
-      writeErr((std::string("Contexts differ at ")) + std::to_string(i));
-      assert(false && "ASTScopeImpl found different context");
-    }
-    // unsigned printContext(llvm::raw_ostream &OS, unsigned indent = 0,
-    // bool onlyAPartialLine = false) const;
-  }
-  if (IndexOfFirstOuterResult != other.IndexOfFirstOuterResult) {
-    writeErr( std::string("IndexOfFirstOuterResult differs, should be: ")
-             + std::to_string(IndexOfFirstOuterResult)
-             + std::string( ", is: ")
-             + std::to_string(other.IndexOfFirstOuterResult));
-    assert(false && "other lookup IndexOfFirstOuterResult differs");
-  }
-  if (recordedSF != other.recordedSF) {
-    writeErr(std::string("recordedSF differs: shouldBe: ") +
-             (recordedSF ? recordedSF->getFilename().str()
-                         : std::string("<no name>")) +
-             std::string(" is: ") +
-             (other.recordedSF ? other.recordedSF->getFilename().str()
-                               : std::string("<no name>")));
-    assert(false && "other lookup recordedSF differs");
-  }
-  if (recordedSF && recordedIsCascadingUse != other.recordedIsCascadingUse) {
-    writeErr(std::string("recordedIsCascadingUse differs: shouldBe: ") +
-             std::to_string(recordedIsCascadingUse) + std::string(" is: ") +
-             std::to_string(other.recordedIsCascadingUse));
-    assert(false && "other lookup recordedIsCascadingUse differs");
-  }
-  return true;
-}
-
-bool UnqualifiedLookupFactory::shouldDiffer() const {
-  auto *SF = dyn_cast<SourceFile>(DC->getModuleScopeContext());
-  if (!SF)
-    return false;
-  
-  static std::vector<const char*> testsThatShouldDiffer {
-    "swift/test/Constraints/diagnostics.swift",
-    "swift/test/Constraints/enum_cases.swift",
-    "swift/test/Constraints/rdar39401774.swift",
-    "swift/test/Constraints/rdar39401774-astscope.swift",
-    "swift/test/Interpreter/repl.swift",
-    "swift/test/Sema/diag_defer_captures.swift",
-    "swift/test/Sema/diag_use_before_declaration.swift",
-    "swift/test/SourceKit/CodeFormat/indent-closure.swift",
-    "swift/test/TypeCoercion/overload_noncall.swift",
-    "swift/test/expr/capture/nested_class.swift",
-    "swift/test/expr/capture/order.swift",
-    "swift/test/NameLookup/name_lookup2.swift"
-  };
-  StringRef fileName = SF->getFilename();
-  return llvm::any_of(testsThatShouldDiffer, [&](const char *testFile) {
-    return fileName.endswith(testFile);
-  });
-}
-
 #pragma mark breakpointing
 #ifndef NDEBUG
 

lib/Driver/ToolChains.cpp

@@ -259,8 +259,6 @@ void ToolChain::addCommonFrontendArgs(const OutputInfo &OI,
   inputArgs.AddLastArg(arguments, options::OPT_debug_diagnostic_names);
   inputArgs.AddLastArg(arguments, options::OPT_print_educational_notes);
   inputArgs.AddLastArg(arguments, options::OPT_diagnostic_style);
-  inputArgs.AddLastArg(arguments, options::OPT_enable_astscope_lookup);
-  inputArgs.AddLastArg(arguments, options::OPT_disable_astscope_lookup);
   inputArgs.AddLastArg(arguments, options::OPT_disable_parser_lookup);
   inputArgs.AddLastArg(arguments,
                        options::OPT_enable_experimental_concise_pound_file);

lib/Frontend/CompilerInvocation.cpp

@@ -443,14 +443,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   }
 
   Opts.DisableParserLookup |= Args.hasArg(OPT_disable_parser_lookup);
-  Opts.EnableASTScopeLookup =
-      Args.hasFlag(options::OPT_enable_astscope_lookup,
-                   options::OPT_disable_astscope_lookup, Opts.EnableASTScopeLookup) ||
-      Opts.DisableParserLookup;
-  Opts.CrosscheckUnqualifiedLookup |=
-      Args.hasArg(OPT_crosscheck_unqualified_lookup);
   Opts.StressASTScopeLookup |= Args.hasArg(OPT_stress_astscope_lookup);
-  Opts.WarnIfASTScopeLookup |= Args.hasArg(OPT_warn_if_astscope_lookup);
   Opts.LazyASTScopes |= Args.hasArg(OPT_lazy_astscopes);
   Opts.EnableNewOperatorLookup = Args.hasFlag(OPT_enable_new_operator_lookup,
                                               OPT_disable_new_operator_lookup,

lib/Parse/ParseDecl.cpp

@@ -104,30 +104,24 @@ namespace {
     template <typename T>
     ParserResult<T>
     fixupParserResult(T *D) {
-      if (movedToTopLevel()) {
-        D->setHoisted();
-        SF->addHoistedDecl(D);
-        getDebuggerClient()->didGlobalize(D);
-      }
+      if (movedToTopLevel())
+        hoistDecl(D);
       return ParserResult<T>(D);
     }
 
     template <typename T>
     ParserResult<T>
     fixupParserResult(ParserStatus Status, T *D) {
-      if (movedToTopLevel()) {
-        D->setHoisted();
-        SF->addHoistedDecl(D);
-        getDebuggerClient()->didGlobalize(D);
-      }
+      if (movedToTopLevel())
+        hoistDecl(D);
       return makeParserResult(Status, D);
     }
 
     // The destructor doesn't need to do anything, the CC's destructor will
     // pop the context if we set it.
     ~DebuggerContextChange () {}
-  protected:
-  
+
+  private:
     DebuggerClient *getDebuggerClient() {
       ModuleDecl *M = P.CurDeclContext->getParentModule();
       return M->getDebugClient();
@@ -152,6 +146,13 @@ namespace {
       SF = P.CurDeclContext->getParentSourceFile();
       CC.emplace(P, SF);
     }
+
+    template<typename T>
+    void hoistDecl(T *D) {
+      D->setHoisted();
+      SF->addHoistedDecl(D);
+      getDebuggerClient()->didGlobalize(D);
+    }
   };
 } // end anonymous namespace