[IDE] SourceEntityWalker should walk all explicit declarations

`SourceEntityWalker` had an unbalanced `walkToDeclPre` and
`walkToDeclPost`, ie. `walkToDeclPost` could be called even though
`walkToDeclPre` was not. Specifically, this would occur for both
`OperatorDecl` and `PrecedenceGroupDecl` declarations.

These could both be added to the `if` in `walkToDeclPost`, but this
seems fairly errorprone in general - especially as new decls are added.
Indeed, there's already declarations that are being skipped because they
aren't explicitly tested for in `walkToDeclPre`, ie.
`PatternBindingDecl`.

Instead of skipping if not explcitly handled, only skip running the
`SEWalker` walk methods if the declaration is implicit (and not a
constructor decl, see TODO). This should probably also always visit
children, with various decls changed to become implicit (eg.
TopLevelCodeDecl), but we can do that later - breaks too many tests for
now.

This change exposed a few parameter declarations that were missing their
implicit flag, as well as unbalanced walk methods in `RangeResolver`.

Author: bnbarham

lib/AST/Decl.cpp

@@ -6088,6 +6088,9 @@ ParamDecl *ParamDecl::cloneWithoutType(const ASTContext &Ctx, ParamDecl *PD) {
 
   Clone->setSpecifier(PD->getSpecifier());
   Clone->setImplicitlyUnwrappedOptional(PD->isImplicitlyUnwrappedOptional());
+  if (PD->isImplicit()) {
+    Clone->setImplicit();
+  }
   return Clone;
 }
 

lib/IDE/IDERequests.cpp

@@ -995,16 +995,24 @@ bool RangeResolver::walkToDeclPre(Decl *D, CharSourceRange Range) {
 }
 
 bool RangeResolver::walkToExprPost(Expr *E) {
+  if (!Impl->shouldEnter(E))
+    return true;
   Impl->leave(E);
   return !Impl->hasResult();
 }
 
 bool RangeResolver::walkToStmtPost(Stmt *S) {
+  if (!Impl->shouldEnter(S))
+    return true;
   Impl->leave(S);
   return !Impl->hasResult();
 };
 
 bool RangeResolver::walkToDeclPost(Decl *D) {
+  if (D->isImplicit())
+    return true;
+  if (!Impl->shouldEnter(D))
+    return true;
   Impl->leave(D);
   return !Impl->hasResult();
 }

lib/IDE/Refactoring.cpp

@@ -4761,7 +4761,9 @@ class AsyncConversionStringBuilder : private SourceEntityWalker {
   }
 
 private:
-  bool walkToDeclPre(Decl *D, CharSourceRange Range) override { return false; }
+  bool walkToDeclPre(Decl *D, CharSourceRange Range) override {
+    return isa<PatternBindingDecl>(D);
+  }
 
 #define PLACEHOLDER_START "<#"
 #define PLACEHOLDER_END "#>"

lib/IDE/SourceEntityWalker.cpp

@@ -84,7 +84,7 @@ class SemaAnnotator : public ASTWalker {
 
   bool passCallArgNames(Expr *Fn, TupleExpr *TupleE);
 
-  bool shouldIgnore(Decl *D, bool &ShouldVisitChildren);
+  bool shouldIgnore(Decl *D);
 
   ValueDecl *extractDecl(Expr *Fn) const {
     Fn = Fn->getSemanticsProvidingExpr();
@@ -106,9 +106,8 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
   if (isDone())
     return false;
 
-  bool ShouldVisitChildren;
-  if (shouldIgnore(D, ShouldVisitChildren))
-    return ShouldVisitChildren;
+  if (shouldIgnore(D))
+    return isa<PatternBindingDecl>(D);
 
   if (!handleCustomAttributes(D)) {
     Cancelled = true;
@@ -177,14 +176,12 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
       }
       return false;
     }
-  } else {
-    return true;
   }
 
   CharSourceRange Range = (Loc.isValid()) ? CharSourceRange(Loc, NameLen)
                                           : CharSourceRange();
-  ShouldVisitChildren = SEWalker.walkToDeclPre(D, Range);
-  if (ShouldVisitChildren && IsExtension) {
+  bool ShouldVisitChildren = SEWalker.walkToDeclPre(D, Range);
+  if (IsExtension) {
     ExtDecls.push_back(static_cast<ExtensionDecl*>(D));
   }
   return ShouldVisitChildren;
@@ -194,19 +191,21 @@ bool SemaAnnotator::walkToDeclPost(Decl *D) {
   if (isDone())
     return false;
 
-  bool ShouldVisitChildren;
-  if (shouldIgnore(D, ShouldVisitChildren))
+  if (shouldIgnore(D))
     return true;
 
+  // Note: This should match any early (non-cancelling) returns in
+  // `walkToDeclPre` above
+  if (auto *ICD = dyn_cast<IfConfigDecl>(D)) {
+    if (SEWalker.shouldWalkInactiveConfigRegion())
+      return true;
+  }
+
   if (isa<ExtensionDecl>(D)) {
     assert(ExtDecls.back() == D);
     ExtDecls.pop_back();
   }
 
-  if (!isa<ValueDecl>(D) && !isa<ExtensionDecl>(D) && !isa<ImportDecl>(D) &&
-      !isa<IfConfigDecl>(D))
-    return true;
-
   bool Continue = SEWalker.walkToDeclPost(D);
   if (!Continue)
     Cancelled = true;
@@ -772,14 +771,10 @@ bool SemaAnnotator::passCallArgNames(Expr *Fn, TupleExpr *TupleE) {
   return true;
 }
 
-bool SemaAnnotator::shouldIgnore(Decl *D, bool &ShouldVisitChildren) {
-  if (D->isImplicit() &&
-      !isa<PatternBindingDecl>(D) &&
-      !isa<ConstructorDecl>(D)) {
-    ShouldVisitChildren = false;
-    return true;
-  }
-  return false;
+bool SemaAnnotator::shouldIgnore(Decl *D) {
+  // TODO: There should really be a separate field controlling whether
+  //       constructors are visited or not
+  return D->isImplicit() && !isa<ConstructorDecl>(D);
 }
 
 bool SourceEntityWalker::walk(SourceFile &SrcFile) {

lib/Sema/CSApply.cpp

@@ -1481,6 +1481,7 @@ namespace {
         selfParamDecl->setSpecifier(
           ParamDecl::getParameterSpecifierForValueOwnership(
             selfParam.getValueOwnership()));
+        selfParamDecl->setImplicit();
 
         auto *outerParams =
             ParameterList::create(context, SourceLoc(), selfParamDecl,
@@ -4803,6 +4804,7 @@ namespace {
           /*parameter name*/ SourceLoc(), ctx.getIdentifier("$0"), closure);
       param->setInterfaceType(baseTy->mapTypeOutOfContext());
       param->setSpecifier(ParamSpecifier::Default);
+      param->setImplicit();
 
       // The outer closure.
       //
@@ -4819,6 +4821,7 @@ namespace {
                               ctx.getIdentifier("$kp$"), outerClosure);
       outerParam->setInterfaceType(keyPathTy->mapTypeOutOfContext());
       outerParam->setSpecifier(ParamSpecifier::Default);
+      outerParam->setImplicit();
 
       // let paramRef = "$0"
       auto *paramRef = new (ctx)

lib/Sema/DerivedConformanceActor.cpp

@@ -143,6 +143,7 @@ static ValueDecl *deriveActor_enqueuePartialTask(DerivedConformance &derived) {
       SourceLoc(), ctx.Id_partialTask, parentDC);
   partialTaskParamDecl->setInterfaceType(partialTaskType);
   partialTaskParamDecl->setSpecifier(ParamSpecifier::Default);
+  partialTaskParamDecl->setImplicit();
 
   // enqueue(partialTask:) method.
   ParameterList *params = ParameterList::createWithoutLoc(partialTaskParamDecl);

lib/Sema/DerivedConformanceAdditiveArithmetic.cpp

@@ -181,6 +181,7 @@ static ValueDecl *deriveMathOperator(DerivedConformance &derived,
                           C.getIdentifier(name), parentDC);
     param->setSpecifier(ParamDecl::Specifier::Default);
     param->setInterfaceType(type);
+    param->setImplicit();
     return param;
   };
 

lib/Sema/DerivedConformanceCodable.cpp

@@ -627,6 +627,7 @@ static FuncDecl *deriveEncodable_encode(DerivedConformance &derived) {
                 SourceLoc(), C.Id_encoder, conformanceDC);
   encoderParam->setSpecifier(ParamSpecifier::Default);
   encoderParam->setInterfaceType(encoderType);
+  encoderParam->setImplicit();
 
   ParameterList *params = ParameterList::createWithoutLoc(encoderParam);
 

lib/Sema/DerivedConformanceComparable.cpp

@@ -235,6 +235,7 @@ deriveComparable_lt(
                                     C.getIdentifier(s), parentDC);
     param->setSpecifier(ParamSpecifier::Default);
     param->setInterfaceType(selfIfaceTy);
+    param->setImplicit();
     return param;
   };
 

lib/Sema/DerivedConformanceDifferentiable.cpp

@@ -305,6 +305,7 @@ static ValueDecl *deriveDifferentiable_method(
                                   SourceLoc(), parameterName, parentDC);
   param->setSpecifier(ParamDecl::Specifier::Default);
   param->setInterfaceType(parameterType);
+  param->setImplicit();
   ParameterList *params = ParameterList::create(C, {param});
 
   DeclName declName(C, methodName, params);