Merge pull request #80070 from gottesmm/swift-settings-only-once

[swift-settings] Only allow for a setting to be passed exactly once to #SwiftSettings.

Author: gottesmm

include/swift/AST/DiagnosticsSema.def

@@ -8399,5 +8399,10 @@ ERROR(swift_settings_invalid_setting, none,
 ERROR(swift_settings_must_be_top_level, none,
       "#SwiftSettings can only be invoked as a top level declaration", ())
 
+ERROR(swift_settings_duplicate_setting, none,
+      "duplicate setting passed to #SwiftSettings", ())
+NOTE(swift_settings_duplicate_setting_original_loc, none,
+     "setting originally passed here", ())
+
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

lib/AST/Module.cpp

@@ -61,6 +61,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/TinyPtrVector.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -4195,20 +4196,41 @@ version::Version ModuleDecl::getLanguageVersionBuiltWith() const {
 //                            MARK: SwiftSettings
 //===----------------------------------------------------------------------===//
 
+static llvm::cl::opt<bool> AllowForDuplicateSwiftSettings(
+    "swift-settings-allow-duplicates",
+    llvm::cl::desc("Option that allows for duplicate SwiftSettings. Just for "
+                   "compiler testing"),
+    llvm::cl::Hidden);
+
 namespace {
 
 enum class SwiftSettingKind {
   Unknown = 0,
   DefaultIsolation,
+
+  LastKind = DefaultIsolation,
 };
 
 struct SwiftSettingsWalker : ASTWalker {
   SourceFile &sf;
   ASTContext &ctx;
   SourceFileLangOptions result;
 
+  SmallVector<Expr *, 1> swiftSettingIndexToOriginalExprMap;
+
   SwiftSettingsWalker(SourceFile &sf, ASTContext &ctx)
-      : sf(sf), ctx(ctx), result() {}
+      : sf(sf), ctx(ctx), result() {
+    // NOTE: We do not store a value for Unknown.
+    for (unsigned i : range(unsigned(SwiftSettingKind::LastKind))) {
+      (void)i;
+      swiftSettingIndexToOriginalExprMap.push_back(nullptr);
+    }
+  }
+
+  Expr *&getOriginalSwiftSetting(SwiftSettingKind kind) {
+    assert(kind != SwiftSettingKind::Unknown);
+    return swiftSettingIndexToOriginalExprMap[unsigned(kind) - 1];
+  }
 
   /// Given a specific CallExpr, pattern matches the CallExpr's first argument
   /// to validate it is MainActor.self. Returns CanType() if the CallExpr has
@@ -4267,13 +4289,30 @@ struct SwiftSettingsWalker : ASTWalker {
         continue;
 
       case SwiftSettingKind::DefaultIsolation:
+        auto *&expr =
+            getOriginalSwiftSetting(SwiftSettingKind::DefaultIsolation);
+
+        // If we already have an expr, emit an error and continue.
+        if (!AllowForDuplicateSwiftSettings && expr) {
+          ctx.Diags.diagnose(arg.getStartLoc(),
+                             diag::swift_settings_duplicate_setting);
+          ctx.Diags.diagnose(
+              expr->getLoc(),
+              diag::swift_settings_duplicate_setting_original_loc);
+          foundValidArg = true;
+          continue;
+        }
+
+        // Otherwise, set things up appropriately.
         if (auto actor = patternMatchDefaultIsolationMainActor(callExpr)) {
+          expr = callExpr;
           result.defaultIsolation = actor;
           foundValidArg = true;
           continue;
         }
 
         if (isa<NilLiteralExpr>(callExpr->getArgs()->getExpr(0))) {
+          expr = callExpr;
           result.defaultIsolation = {Type()};
           foundValidArg = true;
           continue;
@@ -4344,7 +4383,7 @@ SwiftSettingsWalker::getSwiftSettingArgDecl(Argument arg) {
     return {};
 
   // Now lookup our swiftSettingDecl.
-  NominalTypeDecl *swiftSettingsDecl;
+  NominalTypeDecl *swiftSettingsDecl = nullptr;
   {
     SmallVector<ValueDecl *, 1> decls;
     ctx.lookupInSwiftModule("SwiftSetting", decls);

test/Sema/swiftsettings.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -enable-experimental-feature SwiftSettings -enable-experimental-feature Macros -c -swift-version 6 -disable-availability-checking -verify %s
+// RUN: %target-swift-frontend -enable-experimental-feature SwiftSettings -enable-experimental-feature Macros -c -swift-version 6 -disable-availability-checking -verify -Xllvm -swift-settings-allow-duplicates %s
 
 // REQUIRES: asserts
 // REQUIRES: concurrency
@@ -15,7 +15,6 @@ actor MyActor {}
 #SwiftSettings(2) // expected-error {{Unrecognized setting passed to #SwiftSettings}}
 // expected-error @-1 {{cannot convert value of type 'Int' to expected argument type 'SwiftSetting'}}
 
-// We should for now just take the last one that is specified.
 #SwiftSettings(.defaultIsolation(MainActor.self),
                .defaultIsolation(nil))
 

test/Sema/swiftsettings_duplicates.swift

@@ -0,0 +1,24 @@
+// RUN: %target-swift-frontend -enable-experimental-feature SwiftSettings -enable-experimental-feature Macros -c -swift-version 6 -disable-availability-checking -verify %s
+
+// REQUIRES: asserts
+// REQUIRES: concurrency
+// REQUIRES: swift_feature_Macros
+// REQUIRES: swift_feature_SwiftSettings
+
+// This test specifically tests the behavior of #SwiftSettings when we find
+// multiple instances of the same setting.
+
+actor MyActor {}
+
+#SwiftSettings(.defaultIsolation(MainActor.self), // expected-note 6 {{setting originally passed here}}
+               .defaultIsolation(nil)) // expected-error {{duplicate setting passed to #SwiftSettings}}
+#SwiftSettings(.defaultIsolation(nil)) // expected-error {{duplicate setting passed to #SwiftSettings}}
+#SwiftSettings(.defaultIsolation(MyActor.self)) // expected-error {{duplicate setting passed to #SwiftSettings}}
+#SwiftSettings(.defaultIsolation(1)) // expected-error {{duplicate setting passed to #SwiftSettings}}
+// expected-error @-1 {{cannot convert value of type 'Int' to expected argument type 'any Actor.Type'}}
+#SwiftSettings(2) // expected-error {{Unrecognized setting passed to #SwiftSettings}}
+// expected-error @-1 {{cannot convert value of type 'Int' to expected argument type 'SwiftSetting'}}
+
+#SwiftSettings(.defaultIsolation(MainActor.self), // expected-error {{duplicate setting passed to #SwiftSettings}}
+               .defaultIsolation(nil)) // expected-error {{duplicate setting passed to #SwiftSettings}}
+