[Sema/SILGen] InitAccessors: Memberwise initializers with init accessors should follow field order

Skip stored properties that are initialized via init accessors and
emit parameters/initializations in field order which allows us to
cover more use-cases.

Author: xedin

lib/SILGen/SILGenConstructor.cpp

@@ -400,29 +400,39 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF,
 
   // If we have an indirect return slot, initialize it in-place.
   if (resultSlot) {
-    // Tracks all the init accessors we have emitted
-    // because they can initialize more than one property.
-    llvm::SmallPtrSet<AccessorDecl *, 2> emittedInitAccessors;
     auto elti = elements.begin(), eltEnd = elements.end();
-    for (VarDecl *field : decl->getStoredProperties()) {
+
+    llvm::SmallPtrSet<VarDecl *, 4> storedProperties;
+    {
+      auto properties = decl->getStoredProperties();
+      storedProperties.insert(properties.begin(), properties.end());
+    }
+
+    for (auto *member : decl->getMembers()) {
+      auto *field = dyn_cast<VarDecl>(member);
+      if (!field)
+        continue;
+
+      if (initializedViaAccessor.count(field))
+        continue;
 
       // Handle situations where this stored propery is initialized
       // via a call to an init accessor on some other property.
-      if (initializedViaAccessor.count(field) == 1) {
-        auto *initProperty = initializedViaAccessor.find(field)->second;
-        auto *initAccessor = initProperty->getAccessor(AccessorKind::Init);
-
-        if (!emittedInitAccessors.insert(initAccessor).second)
+      if (auto *initAccessor = field->getAccessor(AccessorKind::Init)) {
+        if (field->isMemberwiseInitialized(/*preferDeclaredProperties=*/true)) {
+          assert(elti != eltEnd &&
+                 "number of args does not match number of fields");
+
+          emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy,
+                                  std::move(*elti));
+          ++elti;
           continue;
+        }
+      }
 
-        assert(elti != eltEnd &&
-               "number of args does not match number of fields");
-
-        emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy,
-                                std::move(*elti));
-        ++elti;
+      // If this is not one of the stored properties, let's move on.
+      if (!storedProperties.count(field))
         continue;
-      }
 
       auto fieldTy =
           selfTy.getFieldType(field, SGF.SGM.M, SGF.getTypeExpansionContext());

lib/Sema/CodeSynthesis.cpp

@@ -303,14 +303,6 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl,
     std::multimap<VarDecl *, VarDecl *> initializedViaAccessor;
     decl->collectPropertiesInitializableByInitAccessors(initializedViaAccessor);
 
-    auto createParameter = [&](VarDecl *property) {
-      accessLevel = std::min(accessLevel, property->getFormalAccess());
-      params.push_back(createMemberwiseInitParameter(decl, Loc, property));
-    };
-
-    // A single property could be used to initialize N other stored
-    // properties via a call to its init accessor.
-    llvm::SmallPtrSet<VarDecl *, 4> usedInitProperties;
     for (auto member : decl->getMembers()) {
       auto var = dyn_cast<VarDecl>(member);
       if (!var)
@@ -319,14 +311,11 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl,
       if (!var->isMemberwiseInitialized(/*preferDeclaredProperties=*/true))
         continue;
 
-      // Check whether this property could be initialized via init accessor.
-      if (initializedViaAccessor.count(var) == 1) {
-        auto *initializerProperty = initializedViaAccessor.find(var)->second;
-        if (usedInitProperties.insert(initializerProperty).second)
-          createParameter(initializerProperty);
-      } else {
-        createParameter(var);
-      }
+      if (initializedViaAccessor.count(var))
+        continue;
+
+      accessLevel = std::min(accessLevel, var->getFormalAccess());
+      params.push_back(createMemberwiseInitParameter(decl, Loc, var));
     }
   } else if (ICK == ImplicitConstructorKind::DefaultDistributedActor) {
     auto classDecl = dyn_cast<ClassDecl>(decl);
@@ -1333,48 +1322,47 @@ HasMemberwiseInitRequest::evaluate(Evaluator &evaluator,
       if (initializedViaAccessor.empty())
         return true;
 
-      switch (initializedViaAccessor.count(var)) {
-      // Not covered by an init accessor.
-      case 0:
-        initializedProperties.insert(var);
-        continue;
-
-      // Covered by a single init accessor.
-      case 1:
-        break;
-
-      // Covered by one than one init accessor which means that we
-      // cannot synthesize memberwise initializer.
-      default:
-        return false;
-      }
-
       // Check whether use of init accessors results in access to uninitialized
       // properties.
 
-      for (auto iter = initializedViaAccessor.find(var);
-           iter != initializedViaAccessor.end(); ++iter) {
-        auto *initializerProperty = iter->second;
-        auto *initAccessor =
-            initializerProperty->getAccessor(AccessorKind::Init);
-
+      if (auto *initAccessor = var->getAccessor(AccessorKind::Init)) {
         // Make sure that all properties accessed by init accessor
         // are previously initialized.
         if (auto accessAttr =
-                initAccessor->getAttrs().getAttribute<AccessesAttr>()) {
+            initAccessor->getAttrs().getAttribute<AccessesAttr>()) {
           for (auto *property : accessAttr->getPropertyDecls(initAccessor)) {
             if (!initializedProperties.count(property))
               invalidOrderings.push_back(
-                  {initializerProperty, property->getName()});
+                {var, property->getName()});
           }
         }
 
         // Record all of the properties initialized by calling init accessor.
         if (auto initAttr =
-                initAccessor->getAttrs().getAttribute<InitializesAttr>()) {
+            initAccessor->getAttrs().getAttribute<InitializesAttr>()) {
           auto properties = initAttr->getPropertyDecls(initAccessor);
           initializedProperties.insert(properties.begin(), properties.end());
         }
+
+        continue;
+      }
+
+      switch (initializedViaAccessor.count(var)) {
+      // Not covered by an init accessor.
+      case 0:
+        initializedProperties.insert(var);
+        continue;
+
+      // Covered by a single init accessor, we'll handle that
+      // once we get to the property with init accessor.
+      case 1:
+        continue;
+
+      // Covered by more than one init accessor which means that we
+      // cannot synthesize memberwise initializer due to intersecting
+      // initializations.
+      default:
+        return false;
       }
     }
   }

test/Interpreter/init_accessors.swift

@@ -332,3 +332,64 @@ test_assignments()
 // CHECK-NEXT: test-assignments-1: (3, 42)
 // CHECK-NEXT: a-init-accessor: 0
 // CHECK-NEXT: test-assignments-2: (0, 2)
+
+func test_memberwise_ordering() {
+  struct Test1 {
+    var _a: Int
+    var _b: Int
+
+    var a: Int {
+      init(initialValue) initializes(_a) accesses(_b) {
+        _a = initialValue
+      }
+
+      get { _a }
+      set { }
+    }
+  }
+
+  let test1 = Test1(_b: 42, a: 0)
+  print("test-memberwise-ordering-1: \(test1)")
+
+  struct Test2 {
+    var _a: Int
+
+    var pair: (Int, Int) {
+      init(initialValue) initializes(_a, _b) {
+        _a = initialValue.0
+        _b = initialValue.1
+      }
+
+      get { (_a, _b) }
+      set { }
+    }
+
+    var _b: Int
+  }
+
+  let test2 = Test2(pair: (-1, -2))
+  print("test-memberwise-ordering-2: \(test2)")
+
+  struct Test3 {
+    var _a: Int
+    var _b: Int
+
+    var pair: (Int, Int) {
+      init(initialValue) accesses(_a, _b) {
+      }
+
+      get { (_a, _b) }
+      set { }
+    }
+
+    var _c: Int
+  }
+
+  let test3 = Test3(_a: 1, _b: 2, _c: 3)
+  print("test-memberwise-ordering-3: \(test3)")
+}
+
+test_memberwise_ordering()
+// CHECK: test-memberwise-ordering-1: Test1(_a: 0, _b: 42)
+// CHECK-NEXT: test-memberwise-ordering-2: Test2(_a: -1, _b: -2)
+// CHECK-NEXT: test-memberwise-ordering-3: Test3(_a: 1, _b: 2, _c: 3)

test/decl/var/init_accessors.swift

@@ -360,11 +360,26 @@ func test_memberwise_with_overlaps_dont_synthesize_inits() {
   // expected-error@-1 {{'Test3<String, [Double]>' cannot be constructed because it has no accessible initializers}}
 }
 
-func test_invalid_memberwise() {
-  struct Test1 { // expected-error {{cannot synthesize memberwise initializer}}
+func test_memberwise_ordering() {
+  struct Test1 {
     var _a: Int
     var _b: Int
 
+    var a: Int {
+      init(initialValue) initializes(_a) accesses(_b) {
+        _a = initialValue
+      }
+
+      get { _a }
+      set { }
+    }
+  }
+
+  _ = Test1(_b: 42, a: 0) // Ok
+
+  struct Test2 { // expected-error {{cannot synthesize memberwise initializer}}
+    var _a: Int
+
     var a: Int {
       init(initialValue) initializes(_a) accesses(_b) {
         // expected-note@-1 {{init accessor for 'a' cannot access stored property '_b' because it is called before '_b' is initialized}}
@@ -373,5 +388,59 @@ func test_invalid_memberwise() {
 
       get { _a }
     }
+
+    var _b: Int
   }
+
+  struct Test3 {
+    var _a: Int
+
+    var pair: (Int, Int) {
+      init(initialValue) initializes(_a, _b) {
+        _a = initialValue.0
+        _b = initialValue.1
+      }
+
+      get { (_a, _b) }
+      set { }
+    }
+
+    var _b: Int
+  }
+
+  _ = Test3(pair: (0, 1)) // Ok
+
+  struct Test4 {
+    var _a: Int
+    var _b: Int
+
+    var pair: (Int, Int) {
+      init(initalValue) accesses(_a, _b) {
+      }
+
+      get { (_a, _b) }
+      set { }
+    }
+
+    var _c: Int
+  }
+
+  _ = Test4(_a: 0, _b: 1, _c: 2) // Ok
+
+  struct Test5 {
+    var _a: Int
+    var _b: Int
+
+    var c: Int {
+      init(initalValue) initializes(_c) accesses(_a, _b) {
+      }
+
+      get { _c }
+      set { }
+    }
+
+    var _c: Int
+  }
+
+  _ = Test5(_a: 0, _b: 1, c: 2) // Ok
 }