[Macros] Do not look into macro expansions while resolving macro arguments.

Macros introduced a significant wrinkle into Swift's name lookup mechanism.
Specifically, when resolving names (and, really, anything else) within the
arguments to a macro expansion, name lookup must not try to expand any
macros, because doing so trivially creates a cyclic dependency amongst the
macro expansions that will be detected by the request-evaluator.

Our lookup requests don't always have enough information to answer the
question "is this part of an argument to a macro?", so we do a much simpler,
more efficient, and not-entirely-sound hack based on the request-evaluator.
Specifically, if we are in the process of resolving a macro (which is
determined by checking for the presence of a `ResolveMacroRequest` in the
request-evaluator stack), then we adjust the options used for the name
lookup request we are forming to exclude macro expansions. The evaluation
of that request will then avoid expanding any macros, and not produce any
results that involve entries in already-expanded macros. By adjusting the
request itself, we still distinguish between requests that can and cannot
look into macro expansions, so it doesn't break caching for those immediate
requests.

Over time, we should seek to replace this heuristic with a location-based
check, where we use ASTScope to determine whether we are inside a macro
argument. This existing check might still be useful because it's going to
be faster than a location-based query, but the location-based query can be
fully correct.

This addresses a class of cyclic dependencies that we've been seeing
with macros, and aligns the lookup behavior for module-level lookups
with that specified in the macros proposals. It is not fully complete
because lookup until nominal types does not yet support excluding
results from macro expansions.

Author: DougGregor

include/swift/AST/Evaluator.h

@@ -208,6 +208,18 @@ class Evaluator {
   /// is treated as a stack and is used to detect cycles.
   llvm::SetVector<ActiveRequest> activeRequests;
 
+  /// How many `ResolveMacroRequest` requests are active.
+  ///
+  /// This allows us to quickly determine whether there is any
+  /// `ResolveMacroRequest` active in the active request stack.
+  /// It saves us from a linear scan through `activeRequests` when
+  /// we need to determine this information.
+  ///
+  /// Why on earth would we need to determine this information?
+  /// Please see the extended comment that goes with the constructor
+  /// of `UnqualifiedLookupRequest`.
+  unsigned numActiveResolveMacroRequests = 0;
+
   /// A cache that stores the results of requests.
   evaluator::RequestCache cache;
 
@@ -324,6 +336,16 @@ class Evaluator {
     return activeRequests.count(ActiveRequest(request));
   }
 
+  /// Determine whether there is any active "resolve macro" request
+  /// on the request stack.
+  ///
+  /// Why on earth would we need to determine this information?
+  /// Please see the extended comment that goes with the constructor
+  /// of `UnqualifiedLookupRequest`.
+  bool hasActiveResolveMacroRequest() const {
+    return numActiveResolveMacroRequests > 0;
+  }
+
 private:
   /// Diagnose a cycle detected in the evaluation of the given
   /// request.
@@ -337,6 +359,10 @@ class Evaluator {
   /// request to the \c activeRequests stack.
   bool checkDependency(const ActiveRequest &request);
 
+  /// Note that we have finished this request, popping it from the
+  /// \c activeRequests stack.
+  void finishedRequest(const ActiveRequest &request);
+
   /// Produce the result of the request without caching.
   template<typename Request>
   llvm::Expected<typename Request::OutputType>
@@ -366,8 +392,7 @@ class Evaluator {
 
     // Make sure we remove this from the set of active requests once we're
     // done.
-    assert(activeRequests.back() == activeReq);
-    activeRequests.pop_back();
+    finishedRequest(activeReq);
 
     return std::move(result);
   }

include/swift/AST/NameLookupRequests.h

@@ -430,7 +430,7 @@ class UnqualifiedLookupRequest
                            RequestFlags::Uncached |
                                RequestFlags::DependencySink> {
 public:
-  using SimpleRequest::SimpleRequest;
+  UnqualifiedLookupRequest(UnqualifiedLookupDescriptor);
 
 private:
   friend SimpleRequest;
@@ -456,7 +456,10 @@ class LookupInModuleRequest
                                 NLOptions),
           RequestFlags::Uncached | RequestFlags::DependencySink> {
 public:
-  using SimpleRequest::SimpleRequest;
+  LookupInModuleRequest(
+      const DeclContext *, DeclName, NLKind,
+      namelookup::ResolutionKind, const DeclContext *,
+      NLOptions);
 
 private:
   friend SimpleRequest;
@@ -504,7 +507,9 @@ class ModuleQualifiedLookupRequest
                            RequestFlags::Uncached |
                               RequestFlags::DependencySink> {
 public:
-  using SimpleRequest::SimpleRequest;
+  ModuleQualifiedLookupRequest(const DeclContext *,
+                               ModuleDecl *, DeclNameRef,
+                               NLOptions);
 
 private:
   friend SimpleRequest;
@@ -528,7 +533,9 @@ class QualifiedLookupRequest
                                                  DeclNameRef, NLOptions),
                            RequestFlags::Uncached> {
 public:
-  using SimpleRequest::SimpleRequest;
+  QualifiedLookupRequest(const DeclContext *,
+                         SmallVector<NominalTypeDecl *, 4>,
+                         DeclNameRef, NLOptions);
 
 private:
   friend SimpleRequest;
@@ -579,7 +586,7 @@ class DirectLookupRequest
                            TinyPtrVector<ValueDecl *>(DirectLookupDescriptor),
                            RequestFlags::Uncached|RequestFlags::DependencySink> {
 public:
-  using SimpleRequest::SimpleRequest;
+  DirectLookupRequest(DirectLookupDescriptor);
 
 private:
   friend SimpleRequest;

lib/AST/Evaluator.cpp

@@ -17,6 +17,7 @@
 #include "swift/AST/Evaluator.h"
 #include "swift/AST/DeclContext.h"
 #include "swift/AST/DiagnosticEngine.h"
+#include "swift/AST/TypeCheckRequests.h" // for ResolveMacroRequest
 #include "swift/Basic/LangOptions.h"
 #include "swift/Basic/Range.h"
 #include "swift/Basic/SourceManager.h"
@@ -61,14 +62,25 @@ Evaluator::Evaluator(DiagnosticEngine &diags, const LangOptions &opts)
 
 bool Evaluator::checkDependency(const ActiveRequest &request) {
   // Record this as an active request.
-  if (activeRequests.insert(request))
+  if (activeRequests.insert(request)) {
+    if (request.getAs<ResolveMacroRequest>())
+      ++numActiveResolveMacroRequests;
     return false;
+  }
 
   // Diagnose cycle.
   diagnoseCycle(request);
   return true;
 }
 
+void Evaluator::finishedRequest(const ActiveRequest &request) {
+  if (request.getAs<ResolveMacroRequest>())
+    --numActiveResolveMacroRequests;
+
+  assert(activeRequests.back() == request);
+  activeRequests.pop_back();
+}
+
 void Evaluator::diagnoseCycle(const ActiveRequest &request) {
   if (debugDumpCycles) {
     const auto printIndent = [](llvm::raw_ostream &OS, unsigned indent) {

lib/AST/NameLookupRequests.cpp

@@ -508,6 +508,99 @@ swift::extractNearestSourceLoc(CustomRefCountingOperationDescriptor desc) {
   return SourceLoc();
 }
 
+//----------------------------------------------------------------------------//
+// Macro-related adjustments to name lookup requests.
+//----------------------------------------------------------------------------//
+//
+// Macros introduced a significant wrinkle into Swift's name lookup mechanism.
+// Specifically, when resolving names (and, really, anything else) within the
+// arguments to a macro expansion, name lookup must not try to expand any
+// macros, because doing so trivially creates a cyclic dependency amongst the
+// macro expansions that will be detected by the request-evaluator.
+//
+// Our lookup requests don't always have enough information to answer the
+// question "is this part of an argument to a macro?", so we do a much simpler,
+// more efficient, and not-entirely-sound hack based on the request-evaluator.
+// Specifically, if we are in the process of resolving a macro (which is
+// determined by checking for the presence of a `ResolveMacroRequest` in the
+// request-evaluator stack), then we adjust the options used for the name
+// lookup request we are forming to exclude macro expansions. The evaluation
+// of that request will then avoid expanding any macros, and not produce any
+// results that involve entries in already-expanded macros. By adjusting the
+// request itself, we still distinguish between requests that can and cannot
+// look into macro expansions, so it doesn't break caching for those immediate
+// requests.
+//
+// Over time, we should seek to replace this heuristic with a location-based
+// check, where we use ASTScope to determine whether we are inside a macro
+// argument. This existing check might still be useful because it's going to
+// be faster than a location-based query, but the location-based query can be
+// fully correct.
+
+/// Exclude macros in the unqualified lookup descriptor if we need to.
+static UnqualifiedLookupDescriptor excludeMacrosIfNeeded(
+    UnqualifiedLookupDescriptor descriptor) {
+  if (descriptor.Options.contains(
+          UnqualifiedLookupFlags::ExcludeMacroExpansions))
+    return descriptor;
+
+  auto &evaluator = descriptor.DC->getASTContext().evaluator;
+  if (!evaluator.hasActiveResolveMacroRequest())
+    return descriptor;
+
+  descriptor.Options |= UnqualifiedLookupFlags::ExcludeMacroExpansions;
+  return descriptor;
+}
+
+/// Exclude macros in the direct lookup descriptor if we need to.
+static DirectLookupDescriptor excludeMacrosIfNeeded(
+    DirectLookupDescriptor descriptor) {
+  // FIXME: Direct lookups don't currently have an "exclude macro expansions"
+  // flag.
+  return descriptor;
+}
+
+/// Exclude macros in the name lookup options if we need to.
+static NLOptions
+excludeMacrosIfNeeded(const DeclContext *dc, NLOptions options) {
+  if (options & NL_ExcludeMacroExpansions)
+    return options;
+
+  auto &evaluator = dc->getASTContext().evaluator;
+  if (!evaluator.hasActiveResolveMacroRequest())
+    return options;
+
+  return options | NL_ExcludeMacroExpansions;
+}
+
+UnqualifiedLookupRequest::UnqualifiedLookupRequest(
+    UnqualifiedLookupDescriptor descriptor
+) : SimpleRequest(excludeMacrosIfNeeded(descriptor)) { }
+
+LookupInModuleRequest::LookupInModuleRequest(
+      const DeclContext *moduleOrFile, DeclName name, NLKind lookupKind,
+      namelookup::ResolutionKind resolutionKind,
+      const DeclContext *moduleScopeContext,
+      NLOptions options
+ ) : SimpleRequest(moduleOrFile, name, lookupKind, resolutionKind,
+                   moduleScopeContext,
+                   excludeMacrosIfNeeded(moduleOrFile, options)) { }
+
+ModuleQualifiedLookupRequest::ModuleQualifiedLookupRequest(
+    const DeclContext *dc, ModuleDecl *module, DeclNameRef name,
+    NLOptions options
+ ) : SimpleRequest(dc, module, name, excludeMacrosIfNeeded(dc, options)) { }
+
+QualifiedLookupRequest::QualifiedLookupRequest(
+                       const DeclContext *dc,
+                       SmallVector<NominalTypeDecl *, 4> decls,
+                       DeclNameRef name, NLOptions options
+) : SimpleRequest(dc, std::move(decls), name,
+                  excludeMacrosIfNeeded(dc, options)) { }
+
+DirectLookupRequest::DirectLookupRequest(DirectLookupDescriptor descriptor)
+    : SimpleRequest(excludeMacrosIfNeeded(descriptor)) { }
+
 // Implement the clang importer type zone.
 #define SWIFT_TYPEID_ZONE ClangImporter
 #define SWIFT_TYPEID_HEADER "swift/ClangImporter/ClangImporterTypeIDZone.def"