[Diagnostics] Add property wrapper diagnostic for unnecessary $/'_' in member access

sl · xedin · commit 68026e9688e9 · 2019-06-28T17:54:11.000-07:00
Detect and diagnose incorrect use of `$` or `_` while referencing members. Also: - Refactor some of the logic for the original missing `$` diagnostic, so that a lot of logic can be shared between the two. - Rename the original fix and diagnostic to better reflect their purpose. Resolves: [SR-10928](https://bugs.swift.org/browse/SR-10928) Resolves: rdar://problem/51580712
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -5148,6 +5148,10 @@ class VarDecl : public AbstractStorageDecl {
   /// bound generic version.
   VarDecl *getPropertyWrapperBackingProperty() const;
 
+  /// Retreive the storage wrapper for a property that has an attached
+  /// property wrapper.
+  VarDecl *getPropertyWrapperStorageWrapper() const;
+
   /// Whether this is a property with a property wrapper that was initialized
   /// via a value of the original type, e.g.,
   ///
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -981,10 +981,6 @@ ERROR(missing_address_of,none,
 ERROR(missing_address_of_yield,none,
       "yielding mutable value of type %0 requires explicit '&'",
       (Type))
-ERROR(extraneous_property_wrapper_unwrap,none,
-      "property %0 will be unwrapped to value of type %1, use '_' to refer to "
-      "wrapper type %2",
-      (DeclName, Type, Type))
 ERROR(extraneous_address_of,none,
       "use of extraneous '&'",
       ())
@@ -999,6 +995,14 @@ ERROR(cannot_pass_inout_arg_to_subscript,none,
       "'withUnsafeMutablePointer' to explicitly convert argument "
       "to a pointer", ())
 
+ERROR(incorrect_property_wrapper_reference,none,
+      "cannot convert value %0 of type %1 to expected type %2, "
+      "use %select{wrapper| wrapped value}3 instead",
+      (Identifier, Type, Type, bool))
+ERROR(incorrect_property_wrapper_reference_member,none,
+      "referencing %0 %1 requires %select{wrapper|wrapped value of type}2 %3",
+      (DescriptiveDeclKind, DeclName, bool, Type))
+
 ERROR(missing_init_on_metatype_initialization,none,
       "initializing from a metatype value must reference 'init' explicitly",
       ())
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -5454,6 +5454,10 @@ VarDecl *VarDecl::getPropertyWrapperBackingProperty() const {
   return getPropertyWrapperBackingPropertyInfo().backingVar;
 }
 
+VarDecl *VarDecl::getPropertyWrapperStorageWrapper() const {
+  return getPropertyWrapperBackingPropertyInfo().storageWrapperVar;
+}
+
 static bool propertyWrapperInitializedViaInitialValue(
    const VarDecl *var, bool checkDefaultInit) {
   auto customAttrs = var->getAttachedPropertyWrappers();
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -1721,11 +1721,39 @@ bool MissingCallFailure::diagnoseAsError() {
   return true;
 }
 
+bool ExtraneousPropertyWrapperUnwrapFailure::diagnoseAsError() {
+  auto loc = getAnchor()->getLoc();
+  auto newPrefix = usingStorageWrapper() ? "$" : "_";
+
+  if (auto *member = getReferencedMember()) {
+    emitDiagnostic(loc, diag::incorrect_property_wrapper_reference_member,
+                   member->getDescriptiveKind(), member->getFullName(), false,
+                   getToType())
+        .fixItInsert(loc, newPrefix);
+    return true;
+  }
+
+  emitDiagnostic(loc, diag::incorrect_property_wrapper_reference,
+                 getPropertyName(), getFromType(), getToType(), false)
+      .fixItInsert(loc, newPrefix);
+  return true;
+}
+
 bool MissingPropertyWrapperUnwrapFailure::diagnoseAsError() {
-  emitDiagnostic(getAnchor()->getLoc(),
-                 diag::extraneous_property_wrapper_unwrap, getPropertyName(),
-                 getFromType(), getToType())
-      .fixItInsert(getAnchor()->getLoc(), "_");
+  auto loc = getAnchor()->getLoc();
+  auto endLoc = getAnchor()->getLoc().getAdvancedLoc(1);
+
+  if (auto *member = getReferencedMember()) {
+    emitDiagnostic(loc, diag::incorrect_property_wrapper_reference_member,
+                   member->getDescriptiveKind(), member->getFullName(), true,
+                   getToType())
+        .fixItRemoveChars(loc, endLoc);
+    return true;
+  }
+
+  emitDiagnostic(loc, diag::incorrect_property_wrapper_reference,
+                 getPropertyName(), getFromType(), getToType(), true)
+      .fixItRemoveChars(loc, endLoc);
   return true;
 }
 
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -119,7 +119,7 @@ class FailureDiagnostic {
     return None;
   }
 
-  ValueDecl *getResolvedMemberRef(UnresolvedDotExpr *member) {
+  ValueDecl *getResolvedMemberRef(UnresolvedDotExpr *member) const {
     auto locator = CS.getConstraintLocator(member, ConstraintLocator::Member);
     return CS.findResolvedMemberRef(locator);
   }
@@ -759,20 +759,57 @@ class MissingCallFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
-class MissingPropertyWrapperUnwrapFailure final : public ContextualFailure {
-  DeclName PropertyName;
+class PropertyWrapperReferenceFailure : public ContextualFailure {
+  VarDecl *Property;
+  bool UsingStorageWrapper;
 
+public:
+  PropertyWrapperReferenceFailure(Expr *root, ConstraintSystem &cs,
+                                  VarDecl *property, bool usingStorageWrapper,
+                                  Type base, Type wrapper,
+                                  ConstraintLocator *locator)
+      : ContextualFailure(root, cs, base, wrapper, locator), Property(property),
+        UsingStorageWrapper(usingStorageWrapper) {}
+
+  VarDecl *getProperty() const { return Property; }
+
+  Identifier getPropertyName() const { return Property->getName(); }
+
+  bool usingStorageWrapper() const { return UsingStorageWrapper; }
+
+  ValueDecl *getReferencedMember() const {
+    auto *locator = getLocator();
+    if (auto overload = getOverloadChoiceIfAvailable(locator))
+      return overload->choice.getDeclOrNull();
+    return nullptr;
+  }
+};
+
+class ExtraneousPropertyWrapperUnwrapFailure final
+    : public PropertyWrapperReferenceFailure {
+public:
+  ExtraneousPropertyWrapperUnwrapFailure(Expr *root, ConstraintSystem &cs,
+                                         VarDecl *property,
+                                         bool usingStorageWrapper, Type base,
+                                         Type wrapper,
+                                         ConstraintLocator *locator)
+      : PropertyWrapperReferenceFailure(root, cs, property, usingStorageWrapper,
+                                        base, wrapper, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
+class MissingPropertyWrapperUnwrapFailure final
+    : public PropertyWrapperReferenceFailure {
 public:
   MissingPropertyWrapperUnwrapFailure(Expr *root, ConstraintSystem &cs,
-                                      DeclName propertyName, Type base,
+                                      VarDecl *property,
+                                      bool usingStorageWrapper, Type base,
                                       Type wrapper, ConstraintLocator *locator)
-      : ContextualFailure(root, cs, base, wrapper, locator),
-        PropertyName(propertyName) {}
+      : PropertyWrapperReferenceFailure(root, cs, property, usingStorageWrapper,
+                                        base, wrapper, locator) {}
 
   bool diagnoseAsError() override;
-
-private:
-  DeclName getPropertyName() const { return PropertyName; }
 };
 
 class SubscriptMisuseFailure final : public FailureDiagnostic {
diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp
@@ -260,19 +260,36 @@ InsertExplicitCall *InsertExplicitCall::create(ConstraintSystem &cs,
   return new (cs.getAllocator()) InsertExplicitCall(cs, locator);
 }
 
-bool InsertPropertyWrapperUnwrap::diagnose(Expr *root, bool asNote) const {
+bool UsePropertyWrapper::diagnose(Expr *root, bool asNote) const {
+  auto &cs = getConstraintSystem();
+  auto failure = ExtraneousPropertyWrapperUnwrapFailure(
+      root, cs, Wrapped, UsingStorageWrapper, Base, Wrapper, getLocator());
+  return failure.diagnose(asNote);
+}
+
+UsePropertyWrapper *UsePropertyWrapper::create(ConstraintSystem &cs,
+                                               VarDecl *wrapped,
+                                               bool usingStorageWrapper,
+                                               Type base, Type wrapper,
+                                               ConstraintLocator *locator) {
+  return new (cs.getAllocator()) UsePropertyWrapper(
+      cs, wrapped, usingStorageWrapper, base, wrapper, locator);
+}
+
+bool UseWrappedValue::diagnose(Expr *root, bool asNote) const {
+  auto &cs = getConstraintSystem();
   auto failure = MissingPropertyWrapperUnwrapFailure(
-      root, getConstraintSystem(), getPropertyName(), getBase(), getWrapper(),
+      root, cs, PropertyWrapper, usingStorageWrapper(), Base, Wrapper,
       getLocator());
   return failure.diagnose(asNote);
 }
 
-InsertPropertyWrapperUnwrap *
-InsertPropertyWrapperUnwrap::create(ConstraintSystem &cs, DeclName propertyName,
-                                    Type base, Type wrapper,
-                                    ConstraintLocator *locator) {
+UseWrappedValue *UseWrappedValue::create(ConstraintSystem &cs,
+                                         VarDecl *propertyWrapper, Type base,
+                                         Type wrapper,
+                                         ConstraintLocator *locator) {
   return new (cs.getAllocator())
-      InsertPropertyWrapperUnwrap(cs, propertyName, base, wrapper, locator);
+      UseWrappedValue(cs, propertyWrapper, base, wrapper, locator);
 }
 
 bool UseSubscriptOperator::diagnose(Expr *root, bool asNote) const {
diff --git a/lib/Sema/CSFix.h b/lib/Sema/CSFix.h
@@ -100,8 +100,13 @@ enum class FixKind : uint8_t {
   /// Add explicit `()` at the end of function or member to call it.
   InsertCall,
 
-  /// Add one or more property unwrap operators ('$')
-  InsertPropertyWrapperUnwrap,
+  /// Add '$' or '_' to refer to the property wrapper or storage instead
+  /// of the wrapped property type.
+  UsePropertyWrapper,
+
+  /// Remove '$' or '_' to refer to the wrapped property type instead of
+  /// the storage or property wrapper.
+  UseWrappedValue,
 
   /// Instead of spelling out `subscript` directly, use subscript operator.
   UseSubscriptOperator,
@@ -566,32 +571,56 @@ class InsertExplicitCall final : public ConstraintFix {
                                     ConstraintLocator *locator);
 };
 
-class InsertPropertyWrapperUnwrap final : public ConstraintFix {
-  DeclName PropertyName;
+class UsePropertyWrapper final : public ConstraintFix {
+  VarDecl *Wrapped;
+  bool UsingStorageWrapper;
   Type Base;
   Type Wrapper;
 
-  InsertPropertyWrapperUnwrap(ConstraintSystem &cs, DeclName propertyName,
-                              Type base, Type wrapper,
-                              ConstraintLocator *locator)
-      : ConstraintFix(cs, FixKind::InsertPropertyWrapperUnwrap, locator),
-        PropertyName(propertyName), Base(base), Wrapper(wrapper) {}
+  UsePropertyWrapper(ConstraintSystem &cs, VarDecl *wrapped,
+                     bool usingStorageWrapper, Type base, Type wrapper,
+                     ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::UsePropertyWrapper, locator),
+        Wrapped(wrapped), UsingStorageWrapper(usingStorageWrapper), Base(base),
+        Wrapper(wrapper) {}
 
 public:
   std::string getName() const override {
-    return "insert a $ to unwrap the property wrapper";
+    return "insert '$' or '_' to use property wrapper type instead of wrapped type";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static UsePropertyWrapper *create(ConstraintSystem &cs, VarDecl *wrapped,
+                                    bool usingStorageWrapper, Type base,
+                                    Type wrapper, ConstraintLocator *locator);
+};
+
+class UseWrappedValue final : public ConstraintFix {
+  VarDecl *PropertyWrapper;
+  Type Base;
+  Type Wrapper;
+
+  UseWrappedValue(ConstraintSystem &cs, VarDecl *propertyWrapper, Type base,
+                  Type wrapper, ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::UseWrappedValue, locator),
+        PropertyWrapper(propertyWrapper), Base(base), Wrapper(wrapper) {}
+
+  bool usingStorageWrapper() const {
+    auto nameStr = PropertyWrapper->getName().str();
+    return !nameStr.startswith("_");
   }
 
-  DeclName getPropertyName() const { return PropertyName; }
-  Type getBase() const { return Base; }
-  Type getWrapper() const { return Wrapper; }
+public:
+  std::string getName() const override {
+    return "remove '$' or _ to use wrapped type instead of wrapper type";
+  }
 
   bool diagnose(Expr *root, bool asNote = false) const override;
 
-  static InsertPropertyWrapperUnwrap *create(ConstraintSystem &cs,
-                                             DeclName propertyName, Type base,
-                                             Type wrapper,
-                                             ConstraintLocator *locator);
+  static UseWrappedValue *create(ConstraintSystem &cs, VarDecl *propertyWrapper,
+                                 Type base, Type wrapper,
+                                 ConstraintLocator *locator);
 };
 
 class UseSubscriptOperator final : public ConstraintFix {
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -4741,31 +4741,61 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
       return SolutionKind::Solved;
     }
 
-    auto solveWithNewBaseOrName = [&](Type baseType,
-                                      DeclName memberName) -> SolutionKind {
+    auto solveWithNewBaseOrName = [&](Type baseType, DeclName memberName,
+                                      bool allowFixes = true) -> SolutionKind {
       // Let's re-enable fixes for this member, because
       // the base or member name has been changed.
-      MissingMembers.remove(locator);
+      if (allowFixes)
+        MissingMembers.remove(locator);
       return simplifyMemberConstraint(kind, baseType, memberName, memberTy,
                                       useDC, functionRefKind, outerAlternatives,
                                       flags, locatorB);
     };
 
     // Check if any property wrappers on the base of the member lookup have
-    // mactching members that we can fall back to.
+    // matching members that we can fall back to, or if the type wraps any
+    // properties that have matching members.
     if (auto dotExpr =
             dyn_cast_or_null<UnresolvedDotExpr>(locator->getAnchor())) {
       auto baseExpr = dotExpr->getBase();
-      auto resolvedOverload = findSelectedOverloadFor(baseExpr);
-      if (auto wrappedProperty =
-              getPropertyWrapperInformation(resolvedOverload)) {
-        auto wrapperTy = wrappedProperty->second;
-        auto result = solveWithNewBaseOrName(wrapperTy, member);
-        if (result == SolutionKind::Solved) {
-          auto *fix = InsertPropertyWrapperUnwrap::create(
-              *this, wrappedProperty->first->getFullName(), baseTy, wrapperTy,
-              locator);
-          return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
+
+      if (auto resolvedOverload = findSelectedOverloadFor(baseExpr)) {
+        if (auto storageWrapper =
+                getStorageWrapperInformation(resolvedOverload)) {
+          auto wrapperTy = storageWrapper->second;
+          auto result =
+              solveWithNewBaseOrName(wrapperTy, member, /*allowFixes=*/false);
+          if (result == SolutionKind::Solved) {
+            auto *fix = UsePropertyWrapper::create(*this, storageWrapper->first,
+                                                   /*usingStorageWrapper=*/true,
+                                                   baseTy, wrapperTy, locator);
+            return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
+          }
+        }
+
+        if (auto wrapper = getPropertyWrapperInformation(resolvedOverload)) {
+          auto wrapperTy = wrapper->second;
+          auto result =
+              solveWithNewBaseOrName(wrapperTy, member, /*allowFixes=*/false);
+          if (result == SolutionKind::Solved) {
+            auto *fix = UsePropertyWrapper::create(
+                *this, wrapper->first,
+                /*usingStorageWrappeer=*/false, baseTy, wrapperTy, locator);
+            return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
+          }
+        }
+
+        if (auto wrappedProperty =
+                getWrappedPropertyInformation(resolvedOverload)) {
+          auto wrappedTy = wrappedProperty->second;
+          auto result =
+              solveWithNewBaseOrName(wrappedTy, member, /*allowFixes=*/false);
+
+          if (result == SolutionKind::Solved) {
+            auto *fix = UseWrappedValue::create(*this, wrappedProperty->first,
+                                                baseTy, wrappedTy, locator);
+            return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
+          }
         }
       }
     }
@@ -6716,7 +6746,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::TreatKeyPathSubscriptIndexAsHashable:
   case FixKind::AllowInvalidRefInKeyPath:
   case FixKind::ExplicitlySpecifyGenericArguments:
-  case FixKind::InsertPropertyWrapperUnwrap:
+  case FixKind::UsePropertyWrapper:
+  case FixKind::UseWrappedValue:
     llvm_unreachable("handled elsewhere");
   }
 
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
diff --git a/test/decl/var/property_wrappers.swift b/test/decl/var/property_wrappers.swift
