Merge pull request #26823 from theblixguy/fix/SR_10557

[Typechecker] @dynamicMemberLookup can't find subscript without arg label

Author: xedin

include/swift/AST/Decl.h

@@ -5158,6 +5158,7 @@ class VarDecl : public AbstractStorageDecl {
 /// A function parameter declaration.
 class ParamDecl : public VarDecl {
   Identifier ArgumentName;
+  SourceLoc ParameterNameLoc;
   SourceLoc ArgumentNameLoc;
   SourceLoc SpecifierLoc;
 
@@ -5208,7 +5209,9 @@ class ParamDecl : public VarDecl {
   /// The resulting source location will be valid if the argument name
   /// was specified separately from the parameter name.
   SourceLoc getArgumentNameLoc() const { return ArgumentNameLoc; }
-  
+
+  SourceLoc getParameterNameLoc() const { return ParameterNameLoc; }
+
   SourceLoc getSpecifierLoc() const { return SpecifierLoc; }
 
   bool isTypeLocImplicit() const { return Bits.ParamDecl.IsTypeLocImplicit; }

include/swift/AST/DiagnosticsSema.def

@@ -1121,7 +1121,10 @@ ERROR(missing_dynamic_callable_kwargs_method,none,
 ERROR(invalid_dynamic_member_lookup_type,none,
       "@dynamicMemberLookup attribute requires %0 to have a "
       "'subscript(dynamicMember:)' method that accepts either "
-      "'ExpressibleByStringLiteral' or a keypath", (Type))
+      "'ExpressibleByStringLiteral' or a key path", (Type))
+NOTE(invalid_dynamic_member_subscript, none,
+     "add an explicit argument label to this subscript to satisfy "
+     "the @dynamicMemberLookup requirement", ())
 
 ERROR(string_index_not_integer,none,
       "String must not be indexed with %0, it has variable size elements",

lib/AST/Decl.cpp

@@ -5696,19 +5696,19 @@ void VarDecl::emitLetToVarNoteIfSimple(DeclContext *UseDC) const {
   }
 }
 
-ParamDecl::ParamDecl(Specifier specifier,
-                     SourceLoc specifierLoc, SourceLoc argumentNameLoc,
-                     Identifier argumentName, SourceLoc parameterNameLoc,
-                     Identifier parameterName, DeclContext *dc)
-  : VarDecl(DeclKind::Param,
-            /*IsStatic*/false,
-            specifier == ParamDecl::Specifier::Default
-            ? VarDecl::Introducer::Let
-            : VarDecl::Introducer::Var,
-            /*IsCaptureList*/false, parameterNameLoc, parameterName, dc,
-            StorageIsMutable_t(!isImmutableSpecifier(specifier))),
-  ArgumentName(argumentName), ArgumentNameLoc(argumentNameLoc),
-  SpecifierLoc(specifierLoc) {
+ParamDecl::ParamDecl(Specifier specifier, SourceLoc specifierLoc,
+                     SourceLoc argumentNameLoc, Identifier argumentName,
+                     SourceLoc parameterNameLoc, Identifier parameterName,
+                     DeclContext *dc)
+    : VarDecl(DeclKind::Param,
+              /*IsStatic*/ false,
+              specifier == ParamDecl::Specifier::Default
+                  ? VarDecl::Introducer::Let
+                  : VarDecl::Introducer::Var,
+              /*IsCaptureList*/ false, parameterNameLoc, parameterName, dc,
+              StorageIsMutable_t(!isImmutableSpecifier(specifier))),
+      ArgumentName(argumentName), ParameterNameLoc(parameterNameLoc),
+      ArgumentNameLoc(argumentNameLoc), SpecifierLoc(specifierLoc) {
 
   Bits.ParamDecl.Specifier = static_cast<unsigned>(specifier);
   Bits.ParamDecl.IsTypeLocImplicit = false;

lib/Sema/TypeCheckAttr.cpp

@@ -1144,7 +1144,8 @@ visitDynamicCallableAttr(DynamicCallableAttr *attr) {
 }
 
 static bool hasSingleNonVariadicParam(SubscriptDecl *decl,
-                                      Identifier expectedLabel) {
+                                      Identifier expectedLabel,
+                                      bool ignoreLabel = false) {
   auto *indices = decl->getIndices();
   if (decl->isInvalid() || indices->size() != 1)
     return false;
@@ -1153,6 +1154,10 @@ static bool hasSingleNonVariadicParam(SubscriptDecl *decl,
   if (index->isVariadic() || !index->hasValidSignature())
     return false;
 
+  if (ignoreLabel) {
+    return true;
+  }
+
   return index->getArgumentName() == expectedLabel;
 }
 
@@ -1161,22 +1166,23 @@ static bool hasSingleNonVariadicParam(SubscriptDecl *decl,
 /// The method is given to be defined as `subscript(dynamicMember:)`.
 bool swift::isValidDynamicMemberLookupSubscript(SubscriptDecl *decl,
                                                 DeclContext *DC,
-                                                TypeChecker &TC) {
+                                                TypeChecker &TC,
+                                                bool ignoreLabel) {
   // It could be
   // - `subscript(dynamicMember: {Writable}KeyPath<...>)`; or
   // - `subscript(dynamicMember: String*)`
-  return isValidKeyPathDynamicMemberLookup(decl, TC) ||
-         isValidStringDynamicMemberLookup(decl, DC, TC);
-
+  return isValidKeyPathDynamicMemberLookup(decl, TC, ignoreLabel) ||
+         isValidStringDynamicMemberLookup(decl, DC, TC, ignoreLabel);
 }
 
 bool swift::isValidStringDynamicMemberLookup(SubscriptDecl *decl,
-                                             DeclContext *DC,
-                                             TypeChecker &TC) {
+                                             DeclContext *DC, TypeChecker &TC,
+                                             bool ignoreLabel) {
   // There are two requirements:
   // - The subscript method has exactly one, non-variadic parameter.
   // - The parameter type conforms to `ExpressibleByStringLiteral`.
-  if (!hasSingleNonVariadicParam(decl, TC.Context.Id_dynamicMember))
+  if (!hasSingleNonVariadicParam(decl, TC.Context.Id_dynamicMember,
+                                 ignoreLabel))
     return false;
 
   const auto *param = decl->getIndices()->get(0);
@@ -1191,8 +1197,10 @@ bool swift::isValidStringDynamicMemberLookup(SubscriptDecl *decl,
 }
 
 bool swift::isValidKeyPathDynamicMemberLookup(SubscriptDecl *decl,
-                                              TypeChecker &TC) {
-  if (!hasSingleNonVariadicParam(decl, TC.Context.Id_dynamicMember))
+                                              TypeChecker &TC,
+                                              bool ignoreLabel) {
+  if (!hasSingleNonVariadicParam(decl, TC.Context.Id_dynamicMember,
+                                 ignoreLabel))
     return false;
 
   const auto *param = decl->getIndices()->get(0);
@@ -1217,33 +1225,76 @@ visitDynamicMemberLookupAttr(DynamicMemberLookupAttr *attr) {
   // This attribute is only allowed on nominal types.
   auto decl = cast<NominalTypeDecl>(D);
   auto type = decl->getDeclaredType();
-  
+  auto &ctx = decl->getASTContext();
+
+  auto emitInvalidTypeDiagnostic = [&](const SourceLoc loc) {
+    TC.diagnose(loc, diag::invalid_dynamic_member_lookup_type, type);
+    attr->setInvalid();
+  };
+
   // Look up `subscript(dynamicMember:)` candidates.
-  auto subscriptName = DeclName(TC.Context, DeclBaseName::createSubscript(),
-                                TC.Context.Id_dynamicMember);
+  auto subscriptName =
+      DeclName(ctx, DeclBaseName::createSubscript(), ctx.Id_dynamicMember);
   auto candidates = TC.lookupMember(decl, type, subscriptName);
-  
-  // If there are no candidates, then the attribute is invalid.
-  if (candidates.empty()) {
-    TC.diagnose(attr->getLocation(), diag::invalid_dynamic_member_lookup_type,
-                type);
-    attr->setInvalid();
+
+  if (!candidates.empty()) {
+    // If no candidates are valid, then reject one.
+    auto oneCandidate = candidates.front().getValueDecl();
+    candidates.filter([&](LookupResultEntry entry, bool isOuter) -> bool {
+      auto cand = cast<SubscriptDecl>(entry.getValueDecl());
+      TC.validateDeclForNameLookup(cand);
+      return isValidDynamicMemberLookupSubscript(cand, decl, TC);
+    });
+
+    if (candidates.empty()) {
+      emitInvalidTypeDiagnostic(oneCandidate->getLoc());
+    }
+
     return;
   }
 
-  // If no candidates are valid, then reject one.
-  auto oneCandidate = candidates.front();
-  candidates.filter([&](LookupResultEntry entry, bool isOuter) -> bool {
+  // If we couldn't find any candidates, it's likely because:
+  //
+  // 1. We don't have a subscript with `dynamicMember` label.
+  // 2. We have a subscript with `dynamicMember` label, but no argument label.
+  //
+  // Let's do another lookup using just the base name.
+  auto newCandidates =
+      TC.lookupMember(decl, type, DeclBaseName::createSubscript());
+
+  // Validate the candidates while ignoring the label.
+  newCandidates.filter([&](const LookupResultEntry entry, bool isOuter) {
     auto cand = cast<SubscriptDecl>(entry.getValueDecl());
     TC.validateDeclForNameLookup(cand);
-    return isValidDynamicMemberLookupSubscript(cand, decl, TC);
+    return isValidDynamicMemberLookupSubscript(cand, decl, TC,
+                                               /*ignoreLabel*/ true);
   });
 
-  if (candidates.empty()) {
-    TC.diagnose(oneCandidate.getValueDecl()->getLoc(),
-                diag::invalid_dynamic_member_lookup_type, type);
-    attr->setInvalid();
+  // If there were no potentially valid candidates, then throw an error.
+  if (newCandidates.empty()) {
+    emitInvalidTypeDiagnostic(attr->getLocation());
+    return;
+  }
+
+  // For each candidate, emit a diagnostic. If we don't have an explicit
+  // argument label, then emit a fix-it to suggest the user to add one.
+  for (auto cand : newCandidates) {
+    auto SD = cast<SubscriptDecl>(cand.getValueDecl());
+    auto index = SD->getIndices()->get(0);
+    TC.diagnose(SD, diag::invalid_dynamic_member_lookup_type, type);
+
+    // If we have something like `subscript(foo:)` then we want to insert
+    // `dynamicMember` before `foo`.
+    if (index->getParameterNameLoc().isValid() &&
+        index->getArgumentNameLoc().isInvalid()) {
+      TC.diagnose(SD, diag::invalid_dynamic_member_subscript)
+          .highlight(index->getSourceRange())
+          .fixItInsert(index->getParameterNameLoc(), "dynamicMember ");
+    }
   }
+
+  attr->setInvalid();
+  return;
 }
 
 /// Get the innermost enclosing declaration for a declaration.

lib/Sema/TypeChecker.h

@@ -2022,22 +2022,25 @@ bool isValidDynamicCallableMethod(FuncDecl *decl, DeclContext *DC,
 /// the `subscript(dynamicMember:)` requirement for @dynamicMemberLookup.
 /// The method is given to be defined as `subscript(dynamicMember:)`.
 bool isValidDynamicMemberLookupSubscript(SubscriptDecl *decl, DeclContext *DC,
-                                         TypeChecker &TC);
+                                         TypeChecker &TC,
+                                         bool ignoreLabel = false);
 
 /// Returns true if the given subscript method is an valid implementation of
 /// the `subscript(dynamicMember:)` requirement for @dynamicMemberLookup.
 /// The method is given to be defined as `subscript(dynamicMember:)` which
 /// takes a single non-variadic parameter that conforms to
 /// `ExpressibleByStringLiteral` protocol.
 bool isValidStringDynamicMemberLookup(SubscriptDecl *decl, DeclContext *DC,
-                                      TypeChecker &TC);
+                                      TypeChecker &TC,
+                                      bool ignoreLabel = false);
 
 /// Returns true if the given subscript method is an valid implementation of
 /// the `subscript(dynamicMember: {Writable}KeyPath<...>)` requirement for
 /// @dynamicMemberLookup.
 /// The method is given to be defined as `subscript(dynamicMember:)` which
 /// takes a single non-variadic parameter of `{Writable}KeyPath<T, U>` type.
-bool isValidKeyPathDynamicMemberLookup(SubscriptDecl *decl, TypeChecker &TC);
+bool isValidKeyPathDynamicMemberLookup(SubscriptDecl *decl, TypeChecker &TC,
+                                       bool ignoreLabel = false);
 
 /// Compute the wrapped value type for the given property that has attached
 /// property wrappers, when the backing storage is known to have the given type.

test/attr/attr_dynamic_member_lookup.swift

@@ -140,7 +140,7 @@ func testIUOResult(x: IUOResult) {
 // Subscript index must be ExpressibleByStringLiteral.
 @dynamicMemberLookup
 struct Invalid1 {
-  // expected-error @+1 {{@dynamicMemberLookup attribute requires 'Invalid1' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a keypath}}
+  // expected-error @+1 {{@dynamicMemberLookup attribute requires 'Invalid1' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
   subscript(dynamicMember member: Int) -> Int {
     return 42
   }
@@ -149,7 +149,7 @@ struct Invalid1 {
 // Subscript may not be variadic.
 @dynamicMemberLookup
 struct Invalid2 {
-  // expected-error @+1 {{@dynamicMemberLookup attribute requires 'Invalid2' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a keypath}}
+  // expected-error @+1 {{@dynamicMemberLookup attribute requires 'Invalid2' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
   subscript(dynamicMember member: String...) -> Int {
     return 42
   }
@@ -188,7 +188,7 @@ func NotAllowedOnFunc() {}
 // @dynamicMemberLookup cannot be declared on a base class and fulfilled with a
 // derived class.
 
-// expected-error @+1 {{@dynamicMemberLookup attribute requires 'InvalidBase' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a keypath}}
+// expected-error @+1 {{@dynamicMemberLookup attribute requires 'InvalidBase' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
 @dynamicMemberLookup
 class InvalidBase {}
 
@@ -722,3 +722,25 @@ struct SR10597_1_W<T> {
 
 _ = SR10597_1_W<SR10597_1>(SR10597_1()).wooo // expected-error {{value of type 'SR10597_1_W<SR10597_1>' has no dynamic member 'wooo' using key path from root type 'SR10597_1'; did you mean 'woo'?}}
 _ = SR10597_1_W<SR10597_1>(SR10597_1()).bla // expected-error {{value of type 'SR10597_1_W<SR10597_1>' has no dynamic member 'bla' using key path from root type 'SR10597_1'}}
+
+// SR-10557
+
+@dynamicMemberLookup
+struct SR_10557_S {
+  subscript(dynamicMember: String) -> String { // expected-error {{@dynamicMemberLookup attribute requires 'SR_10557_S' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
+  // expected-note@-1 {{add an explicit argument label to this subscript to satisfy the @dynamicMemberLookup requirement}}{{13-13=dynamicMember }}
+    fatalError()
+  }
+}
+
+@dynamicMemberLookup
+struct SR_10557_S1 {
+  subscript(foo bar: String) -> String { // expected-error {{@dynamicMemberLookup attribute requires 'SR_10557_S1' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
+    fatalError()
+  }
+
+  subscript(foo: String) -> String { // expected-error {{@dynamicMemberLookup attribute requires 'SR_10557_S1' to have a 'subscript(dynamicMember:)' method that accepts either 'ExpressibleByStringLiteral' or a key path}}
+  // expected-note@-1 {{add an explicit argument label to this subscript to satisfy the @dynamicMemberLookup requirement}} {{13-13=dynamicMember }}
+    fatalError()
+  }
+}