[CS] Avoid doing performStmtDiagnostics in CSApply

Instead, ensure we walk into expressions in
SyntacticDiagnosticWalker, allowing
`performStmtDiagnostics` to be called there for
all statements present in the target. This avoids
a case where we'd double diagnose.

While here, inherit the walker from
BaseDiagnosticWalker.

Author: hamishknight

lib/Sema/CSSyntacticElement.cpp

@@ -1733,20 +1733,6 @@ class SyntacticElementSolutionApplication
     return Type();
   }
 
-  ASTNode visit(Stmt *S, bool performSyntacticDiagnostics = true) {
-    auto rewritten = ASTVisitor::visit(S);
-    if (!rewritten)
-      return {};
-
-    if (performSyntacticDiagnostics) {
-      if (auto *stmt = getAsStmt(rewritten)) {
-        performStmtDiagnostics(stmt, context.getAsDeclContext());
-      }
-    }
-
-    return rewritten;
-  }
-
   bool visitPatternBindingDecl(PatternBindingDecl *PBD) {
     // If this is a placeholder variable with an initializer, we just need to
     // set the inferred type.
@@ -2274,7 +2260,7 @@ class ResultBuilderRewriter : public SyntacticElementSolutionApplication {
 private:
   ASTNode visitDoStmt(DoStmt *doStmt) override {
     if (auto transformed = transformDo(doStmt)) {
-      return visit(transformed.get(), /*performSyntacticDiagnostics=*/false);
+      return visit(transformed.get());
     }
 
     auto newBody = visit(doStmt->getBody());

lib/Sema/TypeCheckConstraints.cpp

@@ -327,9 +327,10 @@ void ParentConditionalConformance::diagnoseConformanceStack(
 
 namespace {
 /// Produce any additional syntactic diagnostics for a SyntacticElementTarget.
-class SyntacticDiagnosticWalker final : public ASTWalker {
+class SyntacticDiagnosticWalker final : public BaseDiagnosticWalker {
   const SyntacticElementTarget &Target;
   bool IsTopLevelExprStmt;
+  unsigned ExprDepth = 0;
 
   SyntacticDiagnosticWalker(const SyntacticElementTarget &target,
                             bool isExprStmt)
@@ -341,27 +342,30 @@ class SyntacticDiagnosticWalker final : public ASTWalker {
     target.walk(walker);
   }
 
-  MacroWalking getMacroWalkingBehavior() const override {
-    // We only want to walk macro arguments. Expansions will be walked when
-    // they're type-checked, not as part of the surrounding code.
-    return MacroWalking::Arguments;
+  PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+    // We only want to call the diagnostic logic for the top-level expression,
+    // since the underlying logic will visit each sub-expression. We want to
+    // continue walking however to diagnose any child statements in e.g
+    // closures.
+    if (ExprDepth == 0) {
+      auto isExprStmt = (E == Target.getAsExpr()) ? IsTopLevelExprStmt : false;
+      performSyntacticExprDiagnostics(E, Target.getDeclContext(), isExprStmt);
+    }
+    ExprDepth += 1;
+    return Action::Continue(E);
   }
 
-  PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
-    auto isExprStmt = (expr == Target.getAsExpr()) ? IsTopLevelExprStmt : false;
-    performSyntacticExprDiagnostics(expr, Target.getDeclContext(), isExprStmt);
-    return Action::SkipNode(expr);
+  PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
+    assert(ExprDepth > 0);
+    ExprDepth -= 1;
+    return Action::Continue(E);
   }
 
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *stmt) override {
     performStmtDiagnostics(stmt, Target.getDeclContext());
     return Action::Continue(stmt);
   }
 
-  PreWalkAction walkToDeclPre(Decl *D) override {
-    return Action::VisitNodeIf(isa<PatternBindingDecl>(D));
-  }
-
   PreWalkAction walkToTypeReprPre(TypeRepr *typeRepr) override {
     return Action::SkipNode();
   }

test/Constraints/result_builder_diags.swift

@@ -1029,3 +1029,31 @@ func testMissingElementInEmptyBuilder() {
   func test2() -> Int {}
   // expected-error@-1 {{expected expression of type 'Int' in result builder 'SingleElementBuilder'}} {{24-24=<#T##Int#>}}
 }
+
+// https://github.com/swiftlang/swift/issues/77453
+func testNoDuplicateStmtDiags() {
+  @resultBuilder
+  struct Builder {
+    static func buildBlock<T>(_ components: T...) -> T {
+      components.first!
+    }
+    static func buildEither<T>(first component: T) -> T {
+      component
+    }
+    static func buildEither<T>(second component: T) -> T {
+      component
+    }
+  }
+
+  func takesClosure(_ fn: () -> Void) -> Int? { nil }
+
+  @Builder
+  func foo() -> Int {
+    if let x = takesClosure {} {
+      // expected-warning@-1 {{trailing closure in this context is confusable with the body of the statement}}
+      x
+    } else {
+      1
+    }
+  }
+}

test/Macros/macro_misc_diags.swift

@@ -80,12 +80,12 @@ _ = #identity(deprecatedFunc())
 #makeBinding((deprecatedFunc(), Int, {
   if let _ = takesClosure {} {}
 }()))
-// CHECK-DIAG: Client.swift:[[@LINE-2]]:27: warning: trailing closure in this context is confusable with the body of the statement
-// CHECK-DIAG: Client.swift:[[@LINE-4]]:33: error: expected member name or initializer call after type name
-// CHECK-DIAG: Client.swift:[[@LINE-5]]:15: warning: 'deprecatedFunc()' is deprecated
-// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-7]]{{.*}}makeBindingfMf_.swift:2:27: warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
-// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-8]]{{.*}}makeBindingfMf_.swift:1:28: error: expected member name or initializer call after type name
-// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-9]]{{.*}}makeBindingfMf_.swift:1:10: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: Client.swift:[[@LINE-3]]:33: error: expected member name or initializer call after type name
+// CHECK-DIAG: Client.swift:[[@LINE-4]]:15: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: Client.swift:[[@LINE-4]]:27: warning: trailing closure in this context is confusable with the body of the statement
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-7]]{{.*}}makeBindingfMf_.swift:1:28: error: expected member name or initializer call after type name
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-8]]{{.*}}makeBindingfMf_.swift:1:10: warning: 'deprecatedFunc()' is deprecated
+// CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-9]]{{.*}}makeBindingfMf_.swift:2:27: warning: trailing closure in this context is confusable with the body of the statement
 // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-10]]{{.*}}makeBindingfMf_.swift:1:5: warning: initialization of immutable value
 
 struct S1 {
@@ -104,8 +104,8 @@ func takesClosure(_ fn: () -> Void) -> Int? { nil }
 
 _ = #trailingClosure {
   if let _ = takesClosure {} {}
-  // CHECK-DIAG: Client.swift:[[@LINE-1]]:27: warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
-  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-4]]{{.*}}trailingClosurefMf_.swift:2:27: warning: trailing closure in this context is confusable with the body of the statement
+  // CHECK-DIAG: @__swiftmacro_6Client0017Clientswift_yEEFcfMX[[@LINE-3]]{{.*}}trailingClosurefMf_.swift:2:27: warning: trailing closure in this context is confusable with the body of the statement
+  // CHECK-DIAG: Client.swift:[[@LINE-2]]:27: warning: trailing closure in this context is confusable with the body of the statement
 }
 
 func testOptionalToAny(_ y: Int?) {