Merge pull request #28988 from slavapestov/enclosing-self-type-check-expression-crash

Sema: Fix crash-on-invalid with 'enclosing self' property wrappers

Author: slavapestov

lib/Sema/TypeCheckStorage.cpp

@@ -596,7 +596,9 @@ getEnclosingSelfPropertyWrapperAccess(VarDecl *property, bool forProjected) {
   return result;
 }
 
-/// Build an l-value for the storage of a declaration.
+/// Build an l-value for the storage of a declaration. Returns nullptr if there
+/// was an error. This should only occur if an invalid declaration was type
+/// checked; another diagnostic should have been emitted already.
 static Expr *buildStorageReference(AccessorDecl *accessor,
                                    AbstractStorageDecl *storage,
                                    TargetImpl target,
@@ -683,12 +685,8 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
     auto *backing = var->getPropertyWrapperBackingProperty();
 
     // Error recovery.
-    if (!backing) {
-      auto type = storage->getValueInterfaceType();
-      if (isLValue)
-        type = LValueType::get(type);
-      return new (ctx) ErrorExpr(SourceRange(), type);
-    }
+    if (!backing)
+      return nullptr;
 
     storage = backing;
 
@@ -730,12 +728,8 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
     auto *backing = var->getPropertyWrapperBackingProperty();
 
     // Error recovery.
-    if (!backing) {
-      auto type = storage->getValueInterfaceType();
-      if (isLValue)
-        type = LValueType::get(type);
-      return new (ctx) ErrorExpr(SourceRange(), type);
-    }
+    if (!backing)
+      return nullptr;
 
     storage = backing;
 
@@ -845,7 +839,12 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
         ctx, wrapperMetatype, SourceLoc(), args,
         subscriptDecl->getFullName().getArgumentNames(), { }, SourceLoc(),
         nullptr, subscriptDecl, /*Implicit=*/true);
-    TypeChecker::typeCheckExpression(lookupExpr, accessor);
+
+    // FIXME: Since we're not resolving overloads or anything, we should be
+    // building fully type-checked AST above; we already have all the
+    // information that we need.
+    if (!TypeChecker::typeCheckExpression(lookupExpr, accessor))
+      return nullptr;
 
     // Make sure we produce an lvalue only when desired.
     if (isMemberLValue != lookupExpr->getType()->is<LValueType>()) {
@@ -1043,6 +1042,10 @@ void createPropertyStoreOrCallSuperclassSetter(AccessorDecl *accessor,
   Expr *dest = buildStorageReference(accessor, storage, target,
                                      /*isLValue=*/true, ctx);
 
+  // Error recovery.
+  if (dest == nullptr)
+    return;
+
   // A lazy property setter will store a value of type T into underlying storage
   // of type T?.
   auto destType = dest->getType()->getWithoutSpecifierType();
@@ -1052,6 +1055,7 @@ void createPropertyStoreOrCallSuperclassSetter(AccessorDecl *accessor,
     return;
 
   if (!destType->isEqual(value->getType())) {
+    assert(destType->getOptionalObjectType());
     assert(destType->getOptionalObjectType()->isEqual(value->getType()));
     value = new (ctx) InjectIntoOptionalExpr(value, destType);
   }
@@ -1087,10 +1091,15 @@ synthesizeTrivialGetterBody(AccessorDecl *getter, TargetImpl target,
 
   Expr *result =
     createPropertyLoadOrCallSuperclassGetter(getter, storage, target, ctx);
-  ASTNode returnStmt = new (ctx) ReturnStmt(SourceLoc(), result,
-                                            /*IsImplicit=*/true);
 
-  return { BraceStmt::create(ctx, loc, returnStmt, loc, true),
+  SmallVector<ASTNode, 2> body;
+  if (result != nullptr) {
+    ASTNode returnStmt = new (ctx) ReturnStmt(SourceLoc(), result,
+                                              /*IsImplicit=*/true);
+    body.push_back(returnStmt);
+  }
+
+  return { BraceStmt::create(ctx, loc, body, loc, true),
            /*isTypeChecked=*/true };
 }
 
@@ -1482,7 +1491,13 @@ synthesizeObservedSetterBody(AccessorDecl *Set, TargetImpl target,
   if (VD->getParsedAccessor(AccessorKind::DidSet)) {
     Expr *OldValueExpr
       = buildStorageReference(Set, VD, target, /*isLValue=*/true, Ctx);
-    OldValueExpr = new (Ctx) LoadExpr(OldValueExpr, VD->getType());
+
+    // Error recovery.
+    if (OldValueExpr == nullptr) {
+      OldValueExpr = new (Ctx) ErrorExpr(SourceRange(), VD->getType());
+    } else {
+      OldValueExpr = new (Ctx) LoadExpr(OldValueExpr, VD->getType());
+    }
 
     OldValue = new (Ctx) VarDecl(/*IsStatic*/false, VarDecl::Introducer::Let,
                                  /*IsCaptureList*/false, SourceLoc(),
@@ -1610,13 +1625,14 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
 
   // Build a reference to the storage.
   Expr *ref = buildStorageReference(accessor, storage, target, isLValue, ctx);
+  if (ref != nullptr) {
+    // Wrap it with an `&` marker if this is a modify.
+    ref = maybeWrapInOutExpr(ref, ctx);
 
-  // Wrap it with an `&` marker if this is a modify.
-  ref = maybeWrapInOutExpr(ref, ctx);
-
-  // Yield it.
-  YieldStmt *yield = YieldStmt::create(ctx, loc, loc, ref, loc, true);
-  body.push_back(yield);
+    // Yield it.
+    YieldStmt *yield = YieldStmt::create(ctx, loc, loc, ref, loc, true);
+    body.push_back(yield);
+  }
 
   return { BraceStmt::create(ctx, loc, body, loc, true),
            /*isTypeChecked=*/true };

test/decl/var/property_wrappers_invalid.swift

@@ -0,0 +1,36 @@
+// RUN: %target-swift-frontend -typecheck %s -verify -verify-ignore-unknown
+
+// FIXME: This should produce a diagnostic with a proper
+// source location. Right now, we just get three useless errors:
+
+// <unknown>:0: error: type of expression is ambiguous without more context
+// <unknown>:0: error: type of expression is ambiguous without more context
+// <unknown>:0: error: type of expression is ambiguous without more context
+
+// The actual problem is the type of the subscript declaration is wrong.
+
+public class Store {
+    @Published public var state: Any
+    init() {
+      self.state = 0
+    }
+}
+
+@propertyWrapper public struct Published<Value> {
+  public init(wrappedValue: Value) {}
+  public var wrappedValue: Value {
+    get { fatalError() }
+    set {}
+  }
+  public static subscript(_enclosingInstance object: Any,
+                          wrapped wrappedKeyPath: Any,
+                          storage storageKeyPath: Any)
+      -> Value {
+    get { fatalError() }
+    set {}
+  }
+  public struct Publisher {}
+  public var projectedValue: Publisher {
+    mutating get { fatalError() }
+  }
+}