Merge pull request #24052 from xedin/static-member-in-keypath-diags

[ConstraintSystem] Detect and fix use of static members in a key path in the solver

Author: xedin

lib/Sema/CSApply.cpp

@@ -4572,10 +4572,8 @@ namespace {
           }
 
           // Key paths don't currently support static members.
-          if (varDecl->isStatic()) {
-            cs.TC.diagnose(componentLoc, diag::expr_keypath_static_member,
-                           property->getFullName());
-          }
+          // There is a fix which diagnoses such situation already.
+          assert(!varDecl->isStatic());
         }
 
         cs.TC.requestMemberLayout(property);

lib/Sema/CSDiagnostics.cpp

@@ -1748,12 +1748,16 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
 
   Expr *expr = getParentExpr();
   SourceRange baseRange = expr ? expr->getSourceRange() : SourceRange();
-  auto resolvedOverloadChoice = getResolvedOverload(locator)->Choice;
+
+  auto overload = getOverloadChoiceIfAvailable(locator);
+  if (!overload)
+    return false;
 
   ValueDecl *decl = nullptr;
 
-  if (!resolvedOverloadChoice.isDecl()) {
-    if (auto MT = resolvedOverloadChoice.getBaseType()->getAs<MetatypeType>()) {
+  if (!overload->choice.isDecl()) {
+    auto baseTy = overload->choice.getBaseType();
+    if (auto MT = baseTy->getAs<MetatypeType>()) {
       if (auto VD = dyn_cast<ValueDecl>(
               MT->getMetatypeInstanceType()->getAnyNominal()->getAsDecl())) {
         decl = VD;
@@ -1763,7 +1767,7 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
     }
   }
 
-  auto member = decl ? decl : resolvedOverloadChoice.getDecl();
+  auto member = decl ? decl : overload->choice.getDecl();
 
   // If the base is an implicit self type reference, and we're in a
   // an initializer, then the user wrote something like:
@@ -1933,16 +1937,24 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
 
       return true;
     }
-    
-    if (isa<EnumElementDecl>(member)) {
-      Diag.emplace(emitDiagnostic(loc, diag::could_not_use_enum_element_on_instance,
-                                  Name));
+
+    // If this is a reference to a static member by one of the key path
+    // components, let's provide a tailored diagnostic and return because
+    // that is unsupported so there is no fix-it.
+    if (locator->isForKeyPathComponent()) {
+      InvalidStaticMemberRefInKeyPath failure(expr, getConstraintSystem(),
+                                              member, locator);
+      return failure.diagnoseAsError();
     }
-    else {
-      Diag.emplace(emitDiagnostic(loc, diag::could_not_use_type_member_on_instance,
-                                  baseObjTy, Name));
+
+    if (isa<EnumElementDecl>(member)) {
+      Diag.emplace(emitDiagnostic(
+          loc, diag::could_not_use_enum_element_on_instance, Name));
+    } else {
+      Diag.emplace(emitDiagnostic(
+          loc, diag::could_not_use_type_member_on_instance, baseObjTy, Name));
     }
-    
+
     Diag->highlight(getAnchor()->getSourceRange());
 
     if (Name.isSimpleName(DeclBaseName::createConstructor()) &&
@@ -2002,7 +2014,7 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
         }
       }
     }
-    
+
     // Fall back to a fix-it with a full type qualifier
     auto nominal = member->getDeclContext()->getSelfNominalTypeDecl();
     SmallString<32> typeName;
@@ -2436,3 +2448,22 @@ bool KeyPathSubscriptIndexHashableFailure::diagnoseAsError() {
                  resolveType(NonConformingType));
   return true;
 }
+
+bool InvalidStaticMemberRefInKeyPath::diagnoseAsError() {
+  auto *anchor = getRawAnchor();
+  auto loc = anchor->getLoc();
+
+  if (auto *KPE = dyn_cast<KeyPathExpr>(anchor)) {
+    auto *locator = getLocator();
+    auto component =
+        llvm::find_if(locator->getPath(), [](const LocatorPathElt &elt) {
+          return elt.isKeyPathComponent();
+        });
+
+    assert(component != locator->getPath().end());
+    loc = KPE->getComponents()[component->getValue()].getLoc();
+  }
+
+  emitDiagnostic(loc, diag::expr_keypath_static_member, Member->getBaseName());
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -1031,6 +1031,30 @@ class KeyPathSubscriptIndexHashableFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose an attempt to reference a static member as a key path component
+/// e.g.
+///
+/// ```swift
+/// struct S {
+///   static var foo: Int = 42
+/// }
+///
+/// _ = \S.Type.foo
+/// ```
+class InvalidStaticMemberRefInKeyPath final : public FailureDiagnostic {
+  ValueDecl *Member;
+
+public:
+  InvalidStaticMemberRefInKeyPath(Expr *root, ConstraintSystem &cs,
+                                  ValueDecl *member, ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator), Member(member) {
+    assert(member->hasName());
+    assert(locator->isForKeyPathComponent());
+  }
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -422,3 +422,16 @@ TreatKeyPathSubscriptIndexAsHashable::create(ConstraintSystem &cs, Type type,
   return new (cs.getAllocator())
       TreatKeyPathSubscriptIndexAsHashable(cs, type, locator);
 }
+
+bool AllowStaticMemberRefInKeyPath::diagnose(Expr *root, bool asNote) const {
+  InvalidStaticMemberRefInKeyPath failure(root, getConstraintSystem(), Member,
+                                          getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowStaticMemberRefInKeyPath *
+AllowStaticMemberRefInKeyPath::create(ConstraintSystem &cs, ValueDecl *member,
+                                      ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowStaticMemberRefInKeyPath(cs, member, locator);
+}

lib/Sema/CSFix.h

@@ -139,10 +139,13 @@ enum class FixKind : uint8_t {
 
   /// Allow KeyPaths to use AnyObject as root type
   AllowAnyObjectKeyPathRoot,
-  
+
   /// Using subscript references in the keypath requires that each
   /// of the index arguments to be Hashable.
   TreatKeyPathSubscriptIndexAsHashable,
+
+  /// Allow a reference to a static member as a key path component.
+  AllowStaticMemberRefInKeyPath,
 };
 
 class ConstraintFix {
@@ -780,6 +783,25 @@ class TreatKeyPathSubscriptIndexAsHashable final : public ConstraintFix {
   create(ConstraintSystem &cs, Type type, ConstraintLocator *locator);
 };
 
+class AllowStaticMemberRefInKeyPath final : public ConstraintFix {
+  ValueDecl *Member;
+
+  AllowStaticMemberRefInKeyPath(ConstraintSystem &cs, ValueDecl *member,
+                                ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::AllowStaticMemberRefInKeyPath, locator),
+        Member(member) {}
+
+public:
+  std::string getName() const override {
+    return "allow reference to a static member as a key path component";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static AllowStaticMemberRefInKeyPath *
+  create(ConstraintSystem &cs, ValueDecl *member, ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -5015,11 +5015,26 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
       if (!choices[i].isDecl()) {
         return SolutionKind::Error;
       }
+
       auto storage = dyn_cast<AbstractStorageDecl>(choices[i].getDecl());
       if (!storage) {
         return SolutionKind::Error;
       }
 
+      // Referencing static members in key path is not currently allowed.
+      if (storage->isStatic()) {
+        if (!shouldAttemptFixes())
+          return SolutionKind::Error;
+
+        auto componentLoc =
+            locator.withPathElement(LocatorPathElt::getKeyPathComponent(i));
+        auto *fix = AllowStaticMemberRefInKeyPath::create(
+            *this, choices[i].getDecl(), getConstraintLocator(componentLoc));
+
+        if (recordFix(fix))
+          return SolutionKind::Error;
+      }
+
       if (isReadOnlyKeyPathComponent(storage)) {
         capability = ReadOnly;
         continue;
@@ -6314,6 +6329,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AllowInaccessibleMember:
   case FixKind::AllowAnyObjectKeyPathRoot:
   case FixKind::TreatKeyPathSubscriptIndexAsHashable:
+  case FixKind::AllowStaticMemberRefInKeyPath:
     llvm_unreachable("handled elsewhere");
   }
 

lib/Sema/ConstraintLocator.cpp

@@ -147,6 +147,12 @@ bool ConstraintLocator::isKeyPathSubscriptComponent() const {
   });
 }
 
+bool ConstraintLocator::isForKeyPathComponent() const {
+  return llvm::any_of(getPath(), [&](const LocatorPathElt &elt) {
+    return elt.isKeyPathComponent();
+  });
+}
+
 void ConstraintLocator::dump(SourceManager *sm) {
   dump(sm, llvm::errs());
   llvm::errs() << "\n";

lib/Sema/ConstraintLocator.h

@@ -540,10 +540,14 @@ class ConstraintLocator : public llvm::FoldingSetNode {
   /// as result of the key path dynamic member lookup operation.
   bool isResultOfKeyPathDynamicMemberLookup() const;
 
-  /// Determine whether given locator points to a subscript component
+  /// Determine whether this locator points to a subscript component
   /// of the key path at some index.
   bool isKeyPathSubscriptComponent() const;
 
+  /// Determine whether this locator points to one of the key path
+  /// components.
+  bool isForKeyPathComponent() const;
+
   /// Produce a profile of this locator, for use in a folding set.
   static void Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
                       ArrayRef<PathElement> path);

test/SILOptimizer/access_wmo_diagnose.swift

@@ -10,5 +10,5 @@ public class C {
 }
 
 public func testGlobalProp() {
-  let a: AnyKeyPath = \C.globalProp // expected-error{{static member 'globalProp' cannot be used on instance of type 'C'}}
+  let a: AnyKeyPath = \C.globalProp // expected-error{{key path cannot refer to static member 'globalProp'}}
 }

test/expr/unary/keypath/keypath.swift

@@ -514,9 +514,9 @@ class X {
 }
 
 func testStaticKeyPathComponent() {
-  _ = \X.a // expected-error{{}}
+  _ = \X.a // expected-error{{cannot refer to static member}}
   _ = \X.Type.a // expected-error{{cannot refer to static member}}
-  _ = \X.b // expected-error{{}}
+  _ = \X.b // expected-error{{cannot refer to static member}}
   _ = \X.Type.b // expected-error{{cannot refer to static member}}
 }
 
@@ -708,6 +708,46 @@ var identity11: AnyKeyPath = \Container.self
 
 var interleavedIdentityComponents = \Container.self.base.self?.self.i.self
 
+protocol P_With_Static_Members {
+  static var x: Int { get }
+  static var arr: [Int] { get }
+}
+
+func test_keypath_with_static_members(_ p: P_With_Static_Members) {
+  let _ = p[keyPath: \.x]
+  // expected-error@-1 {{key path cannot refer to static member 'x'}}
+  let _: KeyPath<P_With_Static_Members, Int> = \.x
+  // expected-error@-1 {{key path cannot refer to static member 'x'}}
+  let _ = \P_With_Static_Members.arr.count
+  // expected-error@-1 {{key path cannot refer to static member 'arr'}}
+  let _ = p[keyPath: \.arr.count]
+  // expected-error@-1 {{key path cannot refer to static member 'arr'}}
+
+  struct S {
+    static var foo: String = "Hello"
+    var bar: Bar
+  }
+
+  struct Bar {
+    static var baz: Int = 42
+  }
+
+func foo(_ s: S) {
+    let _ = \S.Type.foo
+    // expected-error@-1 {{key path cannot refer to static member 'foo'}}
+    let _ = s[keyPath: \.foo]
+    // expected-error@-1 {{key path cannot refer to static member 'foo'}}
+    let _: KeyPath<S, String> = \.foo
+    // expected-error@-1 {{key path cannot refer to static member 'foo'}}
+    let _ = \S.foo
+    // expected-error@-1 {{key path cannot refer to static member 'foo'}}
+    let _ = \S.bar.baz
+    // expected-error@-1 {{key path cannot refer to static member 'baz'}}
+    let _ = s[keyPath: \.bar.baz]
+    // expected-error@-1 {{key path cannot refer to static member 'baz'}}
+  }
+}
+
 func testSyntaxErrors() { // expected-note{{}}
   _ = \.  ; // expected-error{{expected member name following '.'}}
   _ = \.a ;