Merge pull request swiftlang#39190 from xedin/diagnostics-for-weak-patterns

[ResultBuilders] Properly diagnose `weak` declarations with non-optional type

Author: xedin

include/swift/Sema/CSFix.h

@@ -331,7 +331,10 @@ enum class FixKind : uint8_t {
 
   /// Specify a type for an explicitly written placeholder that could not be
   /// resolved.
-  SpecifyTypeForPlaceholder
+  SpecifyTypeForPlaceholder,
+
+  /// Allow `weak` declarations to be bound to a non-optional type.
+  AllowNonOptionalWeak,
 };
 
 class ConstraintFix {
@@ -2424,6 +2427,21 @@ class AllowInvalidStaticMemberRefOnProtocolMetatype final
   create(ConstraintSystem &cs, ConstraintLocator *locator);
 };
 
+class AllowNonOptionalWeak final : public ConstraintFix {
+  AllowNonOptionalWeak(ConstraintSystem &cs, ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::AllowNonOptionalWeak, locator) {}
+
+public:
+  std::string getName() const override {
+    return "allow `weak` with non-optional type";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  static AllowNonOptionalWeak *create(ConstraintSystem &cs,
+                                      ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSDiagnostics.cpp

@@ -7776,3 +7776,27 @@ bool UnsupportedRuntimeCheckedCastFailure::diagnoseAsError() {
       .fixItReplace(getCastRange(), "as");
   return true;
 }
+
+bool InvalidWeakAttributeUse::diagnoseAsError() {
+  auto *pattern =
+      dyn_cast_or_null<NamedPattern>(getAnchor().dyn_cast<Pattern *>());
+  if (!pattern)
+    return false;
+
+  auto *var = pattern->getDecl();
+  auto varType = OptionalType::get(getType(var));
+
+  auto diagnostic =
+      emitDiagnosticAt(var, diag::invalid_ownership_not_optional,
+                       ReferenceOwnership::Weak, varType);
+
+  auto typeRange = var->getTypeSourceRangeForDiagnostics();
+  if (varType->hasSimpleTypeRepr()) {
+    diagnostic.fixItInsertAfter(typeRange.End, "?");
+  } else {
+    diagnostic.fixItInsert(typeRange.Start, "(")
+        .fixItInsertAfter(typeRange.End, ")?");
+  }
+
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -2589,6 +2589,25 @@ class InvalidMemberRefOnProtocolMetatype final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose situations where `weak` attribute is used with a non-optional type
+/// in declaration e.g. `weak var x: <Type>`:
+///
+/// \code
+/// class X {
+/// }
+///
+/// weak var x: X = ...
+/// \endcode
+///
+/// `weak` declaration is required to use an optional type e.g. `X?`.
+class InvalidWeakAttributeUse final : public FailureDiagnostic {
+public:
+  InvalidWeakAttributeUse(const Solution &solution, ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -1989,3 +1989,14 @@ AllowInvalidStaticMemberRefOnProtocolMetatype::create(
   return new (cs.getAllocator())
       AllowInvalidStaticMemberRefOnProtocolMetatype(cs, locator);
 }
+
+bool AllowNonOptionalWeak::diagnose(const Solution &solution,
+                                    bool asNote) const {
+  InvalidWeakAttributeUse failure(solution, getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowNonOptionalWeak *AllowNonOptionalWeak::create(ConstraintSystem &cs,
+                                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator()) AllowNonOptionalWeak(cs, locator);
+}

lib/Sema/CSGen.cpp

@@ -2210,31 +2210,19 @@ namespace {
           oneWayVarType = CS.createTypeVariable(
               CS.getConstraintLocator(locator), TVO_CanBindToNoEscape);
 
-          // If there is an externally-imposed pattern type and the
-          // binding/capture is marked as `weak`, let's make sure
-          // that the imposed type is optional.
+          // If there is externally-imposed type, and the variable
+          // is marked as `weak`, let's fallthrough and allow the
+          // `one-way` constraint to be fixed in diagnostic mode.
           //
-          // Note that there is no need to check `varType` since
-          // it's only "externally" bound if this pattern isn't marked
-          // as `weak`.
-          if (externalPatternType &&
-              optionality == ReferenceOwnershipOptionality::Required) {
-            // If the type is not yet known, let's add a constraint
-            // to make sure that it can only be bound to an optional type.
-            if (externalPatternType->isTypeVariableOrMember()) {
-              auto objectTy = CS.createTypeVariable(
-                  CS.getConstraintLocator(locator.withPathElement(
-                      ConstraintLocator::OptionalPayload)),
-                  TVO_CanBindToLValue | TVO_CanBindToNoEscape);
-
-              CS.addConstraint(ConstraintKind::OptionalObject,
-                               externalPatternType, objectTy, locator);
-            } else if (!externalPatternType->getOptionalObjectType()) {
-              // TODO(diagnostics): A tailored fix to indiciate that `weak`
-              // should have an optional type.
-              return Type();
-            }
-          }
+          // That would make sure that type of this variable is
+          // recorded in the constraint  system, which would then
+          // be used instead of `getVarType` upon discovering a
+          // reference to this variable in subsequent expression(s).
+          //
+          // If we let constraint generation fail here, it would trigger
+          // interface type request via `var->getType()` that would
+          // attempt to validate `weak` attribute, and produce a
+          // diagnostic in the middle of the solver path.
 
           CS.addConstraint(ConstraintKind::OneWayEqual, oneWayVarType,
                            externalPatternType ? externalPatternType : varType,

lib/Sema/CSSimplify.cpp

@@ -4956,6 +4956,25 @@ bool ConstraintSystem::repairFailures(
           *this, lhs, rhs, getConstraintLocator(locator)));
     }
 
+    // `weak` declaration with an explicit non-optional type e.g.
+    // `weak x: X = ...` where `X` is a class.
+    if (auto *TP = dyn_cast<TypedPattern>(pattern)) {
+      if (auto *NP = dyn_cast<NamedPattern>(TP->getSubPattern())) {
+        auto *var = NP->getDecl();
+
+        auto ROK = ReferenceOwnership::Strong;
+        if (auto *OA = var->getAttrs().getAttribute<ReferenceOwnershipAttr>())
+          ROK = OA->get();
+
+        if (!rhs->getOptionalObjectType() &&
+            optionalityOf(ROK) == ReferenceOwnershipOptionality::Required) {
+          conversionsOrFixes.push_back(
+              AllowNonOptionalWeak::create(*this, getConstraintLocator(NP)));
+          break;
+        }
+      }
+    }
+
     break;
   }
 
@@ -11709,6 +11728,17 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
     return SolutionKind::Solved;
   }
 
+  case FixKind::AllowNonOptionalWeak: {
+    if (recordFix(fix))
+      return SolutionKind::Error;
+
+    (void)matchTypes(type1, OptionalType::get(type2),
+                     ConstraintKind::Conversion,
+                     TypeMatchFlags::TMF_ApplyingFix, locator);
+
+    return SolutionKind::Solved;
+  }
+
   case FixKind::UseSubscriptOperator:
   case FixKind::ExplicitlyEscaping:
   case FixKind::MarkGlobalActorFunction:

test/Constraints/result_builder_diags.swift

@@ -781,3 +781,19 @@ func test_rdar65667992() {
     }
   }
 }
+
+func test_weak_with_nonoptional_type() {
+  class X {
+    func test() -> Int { 0 }
+  }
+
+  tuplify(true) { c in
+    weak var x: X = X() // expected-error {{'weak' variable should have optional type 'X?'}}
+
+    if let x = x {
+      x.test()
+    }
+
+    42
+  }
+}