From 56130069444534a6eb9441d3ae46d0fc5087dbbf Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 7 Jun 2023 14:31:32 -0700 Subject: [PATCH 01/10] [Sema] PreCheck: Diagnose standalone self within init accessors 'self' within init accessor could only be used to refer to properties listed in `initializes` and `accesses` attributes. --- include/swift/AST/DeclContext.h | 13 ++++++++++ include/swift/AST/DiagnosticsSema.def | 4 +++ lib/AST/DeclContext.cpp | 18 +++++++++++++ lib/Sema/PreCheckExpr.cpp | 20 +++++++++++++-- test/decl/var/init_accessors.swift | 37 +++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 2 deletions(-) diff --git a/include/swift/AST/DeclContext.h b/include/swift/AST/DeclContext.h index d6e21e3167959..41b20f8e5b5b3 100644 --- a/include/swift/AST/DeclContext.h +++ b/include/swift/AST/DeclContext.h @@ -82,6 +82,7 @@ namespace swift { class SerializedDefaultArgumentInitializer; class SerializedTopLevelCodeDecl; class StructDecl; + class AccessorDecl; namespace serialization { using DeclID = llvm::PointerEmbeddedInt; @@ -457,6 +458,18 @@ class alignas(1 << DeclContextAlignInBits) DeclContext return const_cast(this)->getInnermostMethodContext(); } + /// Returns the innermost accessor context that belongs to a property. + /// + /// This routine looks through closure, initializer, and local function + /// contexts to find the innermost accessor declaration. + /// + /// \returns the innermost accessor, or null if there is no such context. + LLVM_READONLY + AccessorDecl *getInnermostPropertyAccessorContext(); + const AccessorDecl *getInnermostPropertyAccessorContext() const { + return const_cast(this)->getInnermostPropertyAccessorContext(); + } + /// Returns the innermost type context. /// /// This routine looks through closure, initializer, and local function diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index d77f402f63d7b..2b5a55cdd88b2 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -7342,6 +7342,10 @@ ERROR(init_accessor_accesses_attribute_on_other_declaration,none, ERROR(init_accessor_property_both_init_and_accessed,none, "property %0 cannot be both initialized and accessed", (DeclName)) +ERROR(invalid_use_of_self_in_init_accessor,none, + "'self' within init accessors can only be used to reference " + "properties listed in 'initializes' and 'accesses'; " + "init accessors are run before 'self' is fully available", ()) #define UNDEFINE_DIAGNOSTIC_MACROS diff --git a/lib/AST/DeclContext.cpp b/lib/AST/DeclContext.cpp index 9a02c2f6cc9e0..88e9cf8da265b 100644 --- a/lib/AST/DeclContext.cpp +++ b/lib/AST/DeclContext.cpp @@ -211,6 +211,24 @@ AbstractFunctionDecl *DeclContext::getInnermostMethodContext() { return nullptr; } +AccessorDecl *DeclContext::getInnermostPropertyAccessorContext() { + auto dc = this; + do { + if (auto decl = dc->getAsDecl()) { + auto accessor = dyn_cast(decl); + // If we found a non-accessor decl, we're done. + if (accessor == nullptr) + return nullptr; + + auto *storage = accessor->getStorage(); + if (isa(storage) && storage->getDeclContext()->isTypeContext()) + return accessor; + } + } while ((dc = dc->getParent())); + + return nullptr; +} + bool DeclContext::isTypeContext() const { if (auto decl = getAsDecl()) return isa(decl) || isa(decl); diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index f634cdf72cdaa..c80255133c9bf 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -1066,8 +1066,24 @@ namespace { if (auto unresolved = dyn_cast(expr)) { TypeChecker::checkForForbiddenPrefix( getASTContext(), unresolved->getName().getBaseName()); - return finish(true, TypeChecker::resolveDeclRefExpr(unresolved, DC, - UseErrorExprs)); + auto *refExpr = + TypeChecker::resolveDeclRefExpr(unresolved, DC, UseErrorExprs); + + // Check whether this is standalone `self` in init accessor, which + // is invalid. + if (auto *accessor = DC->getInnermostPropertyAccessorContext()) { + if (accessor->isInitAccessor() && isa(refExpr)) { + auto *DRE = cast(refExpr); + if (accessor->getImplicitSelfDecl() == DRE->getDecl() && + !isa_and_nonnull(Parent.getAsExpr())) { + Ctx.Diags.diagnose(unresolved->getLoc(), + diag::invalid_use_of_self_in_init_accessor); + refExpr = new (Ctx) ErrorExpr(unresolved->getSourceRange()); + } + } + } + + return finish(true, refExpr); } // Let's try to figure out if `InOutExpr` is out of place early diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index 93eeb37bdc5af..582e1c057ce8d 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -136,3 +136,40 @@ func test_duplicate_and_computed_lazy_properties() { lazy var c: Int = 42 } } + +func test_invalid_self_uses() { + func capture(_: T) -> Int? { nil } + + struct Test { + var a: Int { + init(initialValue) { + let x = self + // expected-error@-1 {{'self' within init accessors can only be used to reference properties listed in 'initializes' and 'accesses'; init accessors are run before 'self' is fully available}} + + _ = { + print(self) + // expected-error@-1 {{'self' within init accessors can only be used to reference properties listed in 'initializes' and 'accesses'; init accessors are run before 'self' is fully available}} + } + + _ = { [weak self] in + // expected-error@-1 {{'self' within init accessors can only be used to reference properties listed in 'initializes' and 'accesses'; init accessors are run before 'self' is fully available}} + } + + _ = { + guard let _ = capture(self) else { + // expected-error@-1 {{'self' within init accessors can only be used to reference properties listed in 'initializes' and 'accesses'; init accessors are run before 'self' is fully available}} + return + } + } + + Test.test(self) + // expected-error@-1 {{'self' within init accessors can only be used to reference properties listed in 'initializes' and 'accesses'; init accessors are run before 'self' is fully available}} + } + + get { 42 } + set { } + } + + static func test(_: T) {} + } +} From 935142ff496189da3ca697e32341141cdc531e36 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 7 Jun 2023 16:18:21 -0700 Subject: [PATCH 02/10] [ConstraintSystem] InitAccessors: Detect invalid references to members within init accessors Only properties that are listed in 'initializes' and 'accesses' attributes could be referenced within init accessor. Detect any and all invalid member references in the solver. --- include/swift/Sema/ConstraintSystem.h | 5 ++++ lib/Sema/CSSimplify.cpp | 41 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h index 6d6bb23a2712c..a18165f4ac5ab 100644 --- a/include/swift/Sema/ConstraintSystem.h +++ b/include/swift/Sema/ConstraintSystem.h @@ -1879,6 +1879,11 @@ struct MemberLookupResult { /// This is a static member being access through a protocol metatype /// but its result type doesn't conform to this protocol. UR_InvalidStaticMemberOnProtocolMetatype, + + /// This is a member that doesn't appear in 'initializes' and/or + /// 'accesses' attributes of the init accessor and therefore canno + /// t be referenced in its body. + UR_UnavailableWithinInitAccessor, }; /// This is a list of considered (but rejected) candidates, along with a diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index da48e9cd8286a..fa8b9c6bdbd83 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -9632,6 +9632,44 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName, } } + if (auto *UDE = + getAsExpr(memberLocator->getAnchor())) { + auto *base = UDE->getBase(); + if (auto *accessor = DC->getInnermostPropertyAccessorContext()) { + if (accessor->isInitAccessor() && isa(base) && + accessor->getImplicitSelfDecl() == + cast(base)->getDecl()) { + bool isValidReference = false; + + // If name doesn't appear in either `initializes` or `accesses` + // then it's invalid instance member. + + if (auto *initializesAttr = + accessor->getAttrs().getAttribute()) { + isValidReference |= llvm::any_of( + initializesAttr->getProperties(), [&](Identifier name) { + return DeclNameRef(name) == memberName; + }); + } + + if (auto *accessesAttr = + accessor->getAttrs().getAttribute()) { + isValidReference |= llvm::any_of( + accessesAttr->getProperties(), [&](Identifier name) { + return DeclNameRef(name) == memberName; + }); + } + + if (!isValidReference) { + result.addUnviable( + candidate, + MemberLookupResult::UR_UnavailableWithinInitAccessor); + return; + } + } + } + } + // If the underlying type of a typealias is fully concrete, it is legal // to access the type with a protocol metatype base. } else if (instanceTy->isExistentialType() && @@ -10208,6 +10246,9 @@ fixMemberRef(ConstraintSystem &cs, Type baseTy, case MemberLookupResult::UR_InvalidStaticMemberOnProtocolMetatype: return AllowInvalidStaticMemberRefOnProtocolMetatype::create(cs, locator); + + case MemberLookupResult::UR_UnavailableWithinInitAccessor: + return nullptr; } } From 3c924be2a058be26bd7c1d224623103445fb2c80 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 7 Jun 2023 16:21:54 -0700 Subject: [PATCH 03/10] [CSFix] InitAccessors: Add a fix that tracks invalid member references within init accessors --- include/swift/Sema/CSFix.h | 37 +++++++++++++++++++++++++++++++++++++ lib/Sema/CSFix.cpp | 13 +++++++++++++ lib/Sema/CSSimplify.cpp | 7 ++++++- 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/include/swift/Sema/CSFix.h b/include/swift/Sema/CSFix.h index 55d9349726ec3..4b05b20679351 100644 --- a/include/swift/Sema/CSFix.h +++ b/include/swift/Sema/CSFix.h @@ -447,6 +447,10 @@ enum class FixKind : uint8_t { /// Ignore missing 'each' keyword before value pack reference. IgnoreMissingEachKeyword, + + /// Ignore the fact that member couldn't be referenced within init accessor + /// because its name doesn't appear in 'initializes' or 'accesses' attributes. + AllowInvalidMemberReferenceInInitAccessor, }; class ConstraintFix { @@ -3536,6 +3540,39 @@ class IgnoreMissingEachKeyword final : public ConstraintFix { } }; +class AllowInvalidMemberReferenceInInitAccessor final : public ConstraintFix { + DeclNameRef MemberName; + + AllowInvalidMemberReferenceInInitAccessor(ConstraintSystem &cs, + DeclNameRef memberName, + ConstraintLocator *locator) + : ConstraintFix(cs, FixKind::AllowInvalidMemberReferenceInInitAccessor, + locator), + MemberName(memberName) {} + +public: + std::string getName() const override { + llvm::SmallVector scratch; + auto memberName = MemberName.getString(scratch); + return "allow reference to member '" + memberName.str() + + "' in init accessor"; + } + + bool diagnose(const Solution &solution, bool asNote = false) const override; + + bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override { + return diagnose(*commonFixes.front().first); + } + + static AllowInvalidMemberReferenceInInitAccessor * + create(ConstraintSystem &cs, DeclNameRef memberName, + ConstraintLocator *locator); + + static bool classof(const ConstraintFix *fix) { + return fix->getKind() == FixKind::AllowInvalidMemberReferenceInInitAccessor; + } +}; + } // end namespace constraints } // end namespace swift diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp index 9b9373bd993c2..2e027dc036a50 100644 --- a/lib/Sema/CSFix.cpp +++ b/lib/Sema/CSFix.cpp @@ -2796,3 +2796,16 @@ IgnoreMissingEachKeyword::create(ConstraintSystem &cs, Type valuePackTy, return new (cs.getAllocator()) IgnoreMissingEachKeyword(cs, valuePackTy, locator); } + +bool AllowInvalidMemberReferenceInInitAccessor::diagnose( + const Solution &solution, bool asNote) const { + return false; +} + +AllowInvalidMemberReferenceInInitAccessor * +AllowInvalidMemberReferenceInInitAccessor::create(ConstraintSystem &cs, + DeclNameRef memberName, + ConstraintLocator *locator) { + return new (cs.getAllocator()) + AllowInvalidMemberReferenceInInitAccessor(cs, memberName, locator); +} diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index fa8b9c6bdbd83..303e8161ea8dd 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -10248,7 +10248,8 @@ fixMemberRef(ConstraintSystem &cs, Type baseTy, return AllowInvalidStaticMemberRefOnProtocolMetatype::create(cs, locator); case MemberLookupResult::UR_UnavailableWithinInitAccessor: - return nullptr; + return AllowInvalidMemberReferenceInInitAccessor::create(cs, memberName, + locator); } } @@ -14674,6 +14675,10 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint( return recordFix(fix, 100) ? SolutionKind::Error : SolutionKind::Solved; } + case FixKind::AllowInvalidMemberReferenceInInitAccessor: { + return recordFix(fix, 5) ? SolutionKind::Error : SolutionKind::Solved; + } + case FixKind::ExplicitlyConstructRawRepresentable: { // Let's increase impact of this fix for binary operators because // it's possible to get both `.rawValue` and construction fixes for From db024d973e5c840fc2e62c33b349a9bebc7cba58 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 7 Jun 2023 16:52:48 -0700 Subject: [PATCH 04/10] [CSDiagnostics] InitAccessors: Implement invalid member reference diagnostics within init accessors --- include/swift/AST/DiagnosticsSema.def | 6 +++- lib/Sema/CSDiagnostics.cpp | 5 +++ lib/Sema/CSDiagnostics.h | 13 ++++++++ lib/Sema/CSFix.cpp | 4 ++- test/decl/var/init_accessors.swift | 48 +++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 2 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 2b5a55cdd88b2..d1360f1c88d03 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -7346,7 +7346,11 @@ ERROR(invalid_use_of_self_in_init_accessor,none, "'self' within init accessors can only be used to reference " "properties listed in 'initializes' and 'accesses'; " "init accessors are run before 'self' is fully available", ()) - +ERROR(init_accessor_invalid_member_ref,none, + "cannot reference instance member %0; init accessors can only " + "refer to instance properties listed in 'initializes' and " + "'accesses' attributes", + (DeclNameRef)) #define UNDEFINE_DIAGNOSTIC_MACROS #include "DefineDiagnosticMacros.h" diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp index c2ed909db3352..19f63f946a384 100644 --- a/lib/Sema/CSDiagnostics.cpp +++ b/lib/Sema/CSDiagnostics.cpp @@ -9058,3 +9058,8 @@ bool MissingEachForValuePackReference::diagnoseAsError() { return true; } + +bool InvalidMemberReferenceWithinInitAccessor::diagnoseAsError() { + emitDiagnostic(diag::init_accessor_invalid_member_ref, MemberName); + return true; +} diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h index 446d5e2dfe6b8..858279d586786 100644 --- a/lib/Sema/CSDiagnostics.h +++ b/lib/Sema/CSDiagnostics.h @@ -3015,6 +3015,19 @@ class MissingEachForValuePackReference final : public FailureDiagnostic { bool diagnoseAsError() override; }; +class InvalidMemberReferenceWithinInitAccessor final + : public FailureDiagnostic { + DeclNameRef MemberName; + +public: + InvalidMemberReferenceWithinInitAccessor(const Solution &solution, + DeclNameRef memberName, + ConstraintLocator *locator) + : FailureDiagnostic(solution, locator), MemberName(memberName) {} + + bool diagnoseAsError() override; +}; + } // end namespace constraints } // end namespace swift diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp index 2e027dc036a50..f4611c0b92758 100644 --- a/lib/Sema/CSFix.cpp +++ b/lib/Sema/CSFix.cpp @@ -2799,7 +2799,9 @@ IgnoreMissingEachKeyword::create(ConstraintSystem &cs, Type valuePackTy, bool AllowInvalidMemberReferenceInInitAccessor::diagnose( const Solution &solution, bool asNote) const { - return false; + InvalidMemberReferenceWithinInitAccessor failure(solution, MemberName, + getLocator()); + return failure.diagnose(asNote); } AllowInvalidMemberReferenceInInitAccessor * diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index 582e1c057ce8d..d7477f40898fb 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -173,3 +173,51 @@ func test_invalid_self_uses() { static func test(_: T) {} } } + +func test_invalid_references() { + struct Test { + var _a: Int + var _b: Int + + var c: String + static var c: String = "" + + var _d: String + + var data: Int { + init(initialValue) initializes(_a) accesses(_d) { + _a = initialValue // Ok + + print(_d) // Ok + self._d = "" // Ok + + if self._b > 0 { // expected-error {{cannot reference instance member '_b'; init accessors can only refer to instance properties listed in 'initializes' and 'accesses' attributes}} + } + + let x = c.lowercased() + // expected-error@-1 {{static member 'c' cannot be used on instance of type 'Test'}} + + print(Test.c.lowercased()) // Ok + + guard let v = test() else { + // expected-error@-1 {{cannot reference instance member 'test'; init accessors can only refer to instance properties listed in 'initializes' and 'accesses' attributes}} + return + } + + _ = { + if true { + print(_b) + // expected-error@-1 {{cannot reference instance member '_b'; init accessors can only refer to instance properties listed in 'initializes' and 'accesses' attributes}} + print(self._b) + // expected-error@-1 {{cannot reference instance member '_b'; init accessors can only refer to instance properties listed in 'initializes' and 'accesses' attributes}} + } + } + } + + get { _a } + set { } + } + + func test() -> Int? { 42 } + } +} From f6fd0bc1a722b7794cffa4876ae0cbc53a589819 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 8 Jun 2023 15:40:35 -0700 Subject: [PATCH 05/10] [DI] InitAccessors: Enforce that @out parameters are fully initialized before every terminator This closes a hole where an early return could leave some properties from `initializes(...)` list uninitialized. For example: ```swift init(initialValue) initializes(_a, _b) { _a = initialValue.0 if _a > 0 { return } _b = initialValue.1 } ``` Here `_b` is not initialized when `_a > 0`. --- .../Mandatory/DefiniteInitialization.cpp | 40 +++++++++++++++---- ...t_accessor_definite_init_diagnostics.swift | 40 +++++++++++++++++++ 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp b/lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp index 12cb54a377583..4ac28a2b993b6 100644 --- a/lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp +++ b/lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp @@ -1154,14 +1154,38 @@ void LifetimeChecker::doIt() { // All of the indirect results marked as "out" have to be fully initialized // before their lifetime ends. - if (TheMemory.isOut() && Uses.empty()) { - auto loc = TheMemory.getLoc(); - - std::string propertyName; - auto *property = TheMemory.getPathStringToElement(0, propertyName); - diagnose(Module, F.getLocation(), - diag::ivar_not_initialized_by_init_accessor, property->getName()); - EmittedErrorLocs.push_back(loc); + if (TheMemory.isOut()) { + auto diagnoseMissingInit = [&]() { + std::string propertyName; + auto *property = TheMemory.getPathStringToElement(0, propertyName); + diagnose(Module, F.getLocation(), + diag::ivar_not_initialized_by_init_accessor, + property->getName()); + EmittedErrorLocs.push_back(TheMemory.getLoc()); + }; + + // No uses means that there was no initialization. + if (Uses.empty()) { + diagnoseMissingInit(); + return; + } + + // Go over every return block and check whether member is fully initialized + // because it's possible that there is branch that doesn't have any use of + // the memory and nothing else is going to diagnose that. This is different + // from `self`, for example, because it would always have either `copy_addr` + // or `load` before return. + + auto returnBB = F.findReturnBB(); + + while (returnBB != F.end()) { + auto *terminator = returnBB->getTerminator(); + + if (!isInitializedAtUse(DIMemoryUse(terminator, DIUseKind::Load, 0, 1))) + diagnoseMissingInit(); + + ++returnBB; + } } // If the memory object has nontrivial type, then any destroy/release of the diff --git a/test/SILOptimizer/init_accessor_definite_init_diagnostics.swift b/test/SILOptimizer/init_accessor_definite_init_diagnostics.swift index 5198db703c0c8..2f5137d25b814 100644 --- a/test/SILOptimizer/init_accessor_definite_init_diagnostics.swift +++ b/test/SILOptimizer/init_accessor_definite_init_diagnostics.swift @@ -139,3 +139,43 @@ struct TestAccessBeforeInit { self.y = y } } + +class TestInitWithGuard { + var _a: Int + var _b: Int + + var pair1: (Int, Int) { + init(initialValue) initializes(_a, _b) { // expected-error {{property '_b' not initialized by init accessor}} + _a = initialValue.0 + + if _a > 0 { + return + } + + _b = initialValue.1 + } + + get { (_a, _b) } + set { } + } + + var pair2: (Int, Int) { + init(initialValue) initializes(_a, _b) { // Ok + _a = initialValue.0 + + if _a > 0 { + _b = 0 + return + } + + _b = initialValue.1 + } + + get { (_a, _b) } + set { } + } + + init(a: Int, b: Int) { + self.pair2 = (a, b) + } +} From ddcfe01ba51207ea25618e8aab8fe0c95ed2ed78 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 9 Jun 2023 10:20:40 -0700 Subject: [PATCH 06/10] [Sema/SILGen] InitAccessors: Emit intersecting init accessor calls in memberwise init If init accessor initialize the same properties, let's emit them in sequence and emit `destroy_addr` in-between to make sure that there is no double initialization. --- lib/SILGen/SILGenConstructor.cpp | 50 ++++--- lib/Sema/CodeSynthesis.cpp | 30 ++--- test/Interpreter/init_accessors.swift | 122 ++++++++++++++++++ .../init_accessor_raw_sil_lowering.swift | 62 +++++++++ test/decl/var/init_accessors.swift | 114 ++++++++++++++++ 5 files changed, 348 insertions(+), 30 deletions(-) diff --git a/lib/SILGen/SILGenConstructor.cpp b/lib/SILGen/SILGenConstructor.cpp index 93a158f8eeb60..1c510e0561103 100644 --- a/lib/SILGen/SILGenConstructor.cpp +++ b/lib/SILGen/SILGenConstructor.cpp @@ -272,22 +272,31 @@ static RValue maybeEmitPropertyWrapperInitFromValue( subs, std::move(arg)); } -static void emitApplyOfInitAccessor(SILGenFunction &SGF, SILLocation loc, - AccessorDecl *accessor, SILValue selfValue, - SILType selfTy, RValue &&initialValue) { +static void +emitApplyOfInitAccessor(SILGenFunction &SGF, SILLocation loc, + AccessorDecl *accessor, SILValue selfValue, + SILType selfTy, RValue &&initialValue, + llvm::SmallPtrSetImpl &initializedFields) { SmallVector arguments; - auto emitFieldReference = [&](VarDecl *field) { + auto emitFieldReference = [&](VarDecl *field, bool forInit = false) { auto fieldTy = selfTy.getFieldType(field, SGF.SGM.M, SGF.getTypeExpansionContext()); - return SGF.B.createStructElementAddr(loc, selfValue, field, - fieldTy.getAddressType()); + auto fieldAddr = SGF.B.createStructElementAddr(loc, selfValue, field, + fieldTy.getAddressType()); + + // If another init accessor already initialized this field then we need + // to emit destroy before calling this accessor. + if (forInit && !initializedFields.insert(field).second) + SGF.B.emitDestroyAddr(loc, fieldAddr); + + return fieldAddr; }; // First, let's emit all of the indirect results. if (auto *initAttr = accessor->getAttrs().getAttribute()) { for (auto *property : initAttr->getPropertyDecls(accessor)) { - arguments.push_back(emitFieldReference(property)); + arguments.push_back(emitFieldReference(property, /*forInit=*/true)); } } @@ -402,6 +411,9 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, // Tracks all the init accessors we have emitted // because they can initialize more than one property. llvm::SmallPtrSet emittedInitAccessors; + // Tracks all the fields that have been already initialized + // via an init accessor. + llvm::SmallPtrSet fieldsInitializedViaAccessor; auto elti = elements.begin(), eltEnd = elements.end(); for (VarDecl *field : decl->getStoredProperties()) { @@ -409,17 +421,25 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, // Handle situations where this stored propery is initialized // via a call to an init accessor on some other property. if (initializedViaAccessor.count(field)) { - auto *initProperty = initializedViaAccessor.find(field)->second; - auto *initAccessor = initProperty->getAccessor(AccessorKind::Init); + for (auto iter = initializedViaAccessor.find(field); + iter != initializedViaAccessor.end(); ++iter) { + auto *initProperty = iter->second; + auto *initAccessor = initProperty->getAccessor(AccessorKind::Init); - if (emittedInitAccessors.count(initAccessor)) - continue; + if (!emittedInitAccessors.insert(initAccessor).second) + continue; - emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy, - std::move(*elti)); + assert(elti != eltEnd && + "number of args does not match number of fields"); - emittedInitAccessors.insert(initAccessor); - ++elti; + emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy, + std::move(*elti), + fieldsInitializedViaAccessor); + ++elti; + } + + // After all init accessors are emitted let's move on to the next + // property. continue; } diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp index 26e6a088d0cea..6279b44b2267c 100644 --- a/lib/Sema/CodeSynthesis.cpp +++ b/lib/Sema/CodeSynthesis.cpp @@ -303,6 +303,11 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl, std::multimap 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 usedInitProperties; @@ -315,22 +320,17 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl, continue; // Check whether this property could be initialized via init accessor. - // - // Note that we check for a single match here because intersecting - // properties are going to be diagnosed. - if (initializedViaAccessor.count(var) == 1) { - auto *initializerProperty = initializedViaAccessor.find(var)->second; - // Parameter for this property is already emitted. - if (usedInitProperties.count(initializerProperty)) - continue; - - var = initializerProperty; - usedInitProperties.insert(initializerProperty); - } - - accessLevel = std::min(accessLevel, var->getFormalAccess()); + if (initializedViaAccessor.count(var)) { + for (auto iter = initializedViaAccessor.find(var); + iter != initializedViaAccessor.end(); ++iter) { + auto *initializerProperty = iter->second; - params.push_back(createMemberwiseInitParameter(decl, Loc, var)); + if (usedInitProperties.insert(initializerProperty).second) + createParameter(initializerProperty); + } + } else { + createParameter(var); + } } } else if (ICK == ImplicitConstructorKind::DefaultDistributedActor) { auto classDecl = dyn_cast(decl); diff --git a/test/Interpreter/init_accessors.swift b/test/Interpreter/init_accessors.swift index 69e5f52ea99fa..765c6a9323e0e 100644 --- a/test/Interpreter/init_accessors.swift +++ b/test/Interpreter/init_accessors.swift @@ -332,3 +332,125 @@ 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_with_overlaps() { + struct Test1 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var pair: (T, Int) { + init(initialValue) initializes(_a, _b) { + _a = initialValue.0 + _b = initialValue.1 + } + + get { (_a, _b) } + set { } + } + + var c: U + } + + let test1 = Test1(a: "a", pair: ("b", 1), c: [3.0]) + print("memberwise-overlaps-1: \(test1)") + + struct Test2 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var b: Int { + init(initialValue) initializes(_b) { + _b = initialValue + } + + get { _b } + set { } + } + + var _c: U + + var pair: (T, U) { + init(initialValue) initializes(_a, _c) { + _a = initialValue.0 + _c = initialValue.1 + } + + get { (_a, _c) } + set { } + } + } + + let test2 = Test2(a: "a", pair: ("c", 2), b: 0) + print("memberwise-overlaps-2: \(test2)") + + struct Test3 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var b: Int { + init(initialValue) initializes(_b) { + _b = initialValue + } + + get { _b } + set { } + } + + var _c: U + + var c: U { + init(initialValue) initializes(_c) { + _c = initialValue + } + + get { _c } + set { } + } + + var triple: (T, Int, U) { + init(initialValue) initializes(_a, _b, _c) { + _a = initialValue.0 + _b = initialValue.1 + _c = initialValue.2 + } + + get { (_a, _b, _c) } + set { } + } + } + + let test3 = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) + print("memberwise-overlaps-3: \(test3)") +} + +test_memberwise_with_overlaps() +// CHECK: memberwise-overlaps-1: Test1>(_a: "b", _b: 1, c: [3.0]) +// CHECK-NEXT: memberwise-overlaps-2: Test2(_a: "c", _b: 0, _c: 2) +// CHECK-NEXT: memberwise-overlaps-3: Test3>(_a: "b", _b: 0, _c: [1.0]) diff --git a/test/SILOptimizer/init_accessor_raw_sil_lowering.swift b/test/SILOptimizer/init_accessor_raw_sil_lowering.swift index f12398a4d37d9..92816c958c5bb 100644 --- a/test/SILOptimizer/init_accessor_raw_sil_lowering.swift +++ b/test/SILOptimizer/init_accessor_raw_sil_lowering.swift @@ -2,6 +2,68 @@ // REQUIRES: asserts +// Makes sure that SILGen for memberwise initializer has destroy_addr for overlapping properties. +struct TestMemberwiseWithOverlap { + var _a: Int + var _b: Int + var _c: Int + + var a: Int { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var pair: (Int, Int) { + init(initialValue) initializes(_a, _b) { + _a = initialValue.0 + _b = initialValue.1 + } + + get { (_a, _b) } + set { } + } + + var c: Int { + init(initialValue) initializes(_b, _c) accesses(_a) { + _b = -1 + _c = initialValue + } + + get { _c } + set { } + } + + // CHECK-LABEL: sil hidden [ossa] @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1a4pair1cACSi_Si_SitSitcfC : $@convention(method) (Int, Int, Int, Int, @thin TestMemberwiseWithOverlap.Type) -> TestMemberwiseWithOverlap + // + // CHECK: [[SELF:%.*]] = alloc_stack $TestMemberwiseWithOverlap + // + // CHECK: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a + // CHECK: [[A_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1aSivi : $@convention(thin) (Int) -> @out Int + // CHECK-NEXT: {{.*}} = apply [[A_INIT]]([[A_REF]], %0) : $@convention(thin) (Int) -> @out Int + // + // CHECK: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a + // CHECK-NEXT: destroy_addr [[A_REF]] : $*Int + // CHECK-NEXT: [[B_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._b + // CHECK: [[PAIR_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV4pairSi_Sitvi : $@convention(thin) (Int, Int) -> (@out Int, @out Int) + // CHECK-NEXT: {{.*}} = apply [[PAIR_INIT]]([[A_REF]], [[B_REF]], %1, %2) : $@convention(thin) (Int, Int) -> (@out Int, @out Int) + // + // CHECK: [[B_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._b + // CHECK-NEXT: destroy_addr [[B_REF]] : $*Int + // CHECK-NEXT: [[C_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._c + // CHECK-NEXT: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a + // CHECK: [[C_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1cSivi : $@convention(thin) (Int, @inout Int) -> (@out Int, @out Int) + // CHECK-NEXT: {{.*}} = apply [[C_INIT]]([[B_REF]], [[C_REF]], %3, [[A_REF]]) : $@convention(thin) (Int, @inout Int) -> (@out Int, @out Int) + // CHECK-NEXT: [[RESULT:%.*]] = load [trivial] [[SELF]] : $*TestMemberwiseWithOverlap + // CHECK-NEXT: dealloc_stack [[SELF]] : $*TestMemberwiseWithOverlap + // CHECK-NEXT: return [[RESULT]] : $TestMemberwiseWithOverlap +} + +_ = TestMemberwiseWithOverlap(a: 0, pair: (1, 2), c: 3) + struct Test1 { var _a: Int var _b: String diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index d7477f40898fb..988ad3dc539c0 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -221,3 +221,117 @@ func test_invalid_references() { func test() -> Int? { 42 } } } + +func test_memberwise_with_overlaps() { + struct Test1 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var pair: (T, Int) { + init(initialValue) initializes(_a, _b) { + _a = initialValue.0 + _b = initialValue.1 + } + + get { (_a, _b) } + set { } + } + + var c: U + } + + _ = Test1(a: "a", pair: ("b", 1), c: [3.0]) // Ok + + struct Test2 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var b: Int { + init(initialValue) initializes(_b) { + _b = initialValue + } + + get { _b } + set { } + } + + var _c: U + + var pair: (T, U) { + init(initialValue) initializes(_a, _c) { + _a = initialValue.0 + _c = initialValue.1 + } + + get { (_a, _c) } + set { } + } + } + + _ = Test2(a: "a", pair: ("c", 2), b: 0) // Ok + + struct Test3 { + var _a: T + var _b: Int + + var a: T { + init(initialValue) initializes(_a) { + _a = initialValue + } + + get { _a } + set { } + } + + var b: Int { + init(initialValue) initializes(_b) { + _b = initialValue + } + + get { _b } + set { } + } + + var _c: U + + var c: U { + init(initialValue) initializes(_c) { + _c = initialValue + } + + get { _c } + set { } + } + + var triple: (T, Int, U) { + init(initialValue) initializes(_a, _b, _c) { + _a = initialValue.0 + _b = initialValue.1 + _c = initialValue.2 + } + + get { (_a, _b, _c) } + set { } + } + } + + _ = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) // Ok +} From 4f59538eba6628d842afbcc7fc9c2d3ba8878b54 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 9 Jun 2023 13:30:21 -0700 Subject: [PATCH 07/10] [Sema] InitAccessors: Diagnose situations when memberwise init cannot be synthesized If some of the properties with init accessors have out of order accesses diagnose that while checking whether memberwise init could be synthesized. --- include/swift/AST/DiagnosticsSema.def | 7 +++ lib/Sema/CodeSynthesis.cpp | 63 ++++++++++++++++++++++++++- test/decl/var/init_accessors.swift | 37 ++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index d1360f1c88d03..e849dd17348d6 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -7351,6 +7351,13 @@ ERROR(init_accessor_invalid_member_ref,none, "refer to instance properties listed in 'initializes' and " "'accesses' attributes", (DeclNameRef)) +ERROR(cannot_synthesize_memberwise_due_to_property_init_order,none, + "cannot synthesize memberwise initializer", + ()) +NOTE(out_of_order_access_in_init_accessor,none, + "init accessor for %0 cannot access stored property %1 because it " + "is called before %1 is initialized", + (Identifier, Identifier)) #define UNDEFINE_DIAGNOSTIC_MACROS #include "DefineDiagnosticMacros.h" diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp index 6279b44b2267c..db8bdc7c3c973 100644 --- a/lib/Sema/CodeSynthesis.cpp +++ b/lib/Sema/CodeSynthesis.cpp @@ -1317,6 +1317,12 @@ HasMemberwiseInitRequest::evaluate(Evaluator &evaluator, if (hasUserDefinedDesignatedInit(evaluator, decl)) return false; + std::multimap initializedViaAccessor; + decl->collectPropertiesInitializableByInitAccessors(initializedViaAccessor); + + llvm::SmallPtrSet initializedProperties; + llvm::SmallVector> invalidOrderings; + for (auto *member : decl->getMembers()) { if (auto *var = dyn_cast(member)) { // If this is a backing storage property for a property wrapper, @@ -1324,10 +1330,65 @@ HasMemberwiseInitRequest::evaluate(Evaluator &evaluator, if (var->getOriginalWrappedProperty()) continue; - if (var->isMemberwiseInitialized(/*preferDeclaredProperties=*/true)) + if (!var->isMemberwiseInitialized(/*preferDeclaredProperties=*/true)) + continue; + + // If init accessors are not involved, we are done. + if (initializedViaAccessor.empty()) return true; + + if (!initializedViaAccessor.count(var)) { + initializedProperties.insert(var); + continue; + } + + // 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); + + // Make sure that all properties accessed by init accessor + // are previously initialized. + if (auto accessAttr = + initAccessor->getAttrs().getAttribute()) { + for (auto *property : accessAttr->getPropertyDecls(initAccessor)) { + if (!initializedProperties.count(property)) + invalidOrderings.push_back( + {initializerProperty, property->getName()}); + } + } + + // Record all of the properties initialized by calling init accessor. + if (auto initAttr = + initAccessor->getAttrs().getAttribute()) { + auto properties = initAttr->getPropertyDecls(initAccessor); + initializedProperties.insert(properties.begin(), properties.end()); + } + } } } + + if (invalidOrderings.empty()) + return !initializedProperties.empty(); + + { + auto &diags = decl->getASTContext().Diags; + + diags.diagnose( + decl, diag::cannot_synthesize_memberwise_due_to_property_init_order); + + for (const auto &invalid : invalidOrderings) { + auto *accessor = invalid.first->getAccessor(AccessorKind::Init); + diags.diagnose(accessor->getLoc(), + diag::out_of_order_access_in_init_accessor, + invalid.first->getName(), invalid.second); + } + } + return false; } diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index 988ad3dc539c0..0b1765d74c0cd 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -50,6 +50,7 @@ func test_use_of_initializes_accesses_on_non_inits() { } get { x } + set { } } var _y: String { @@ -63,6 +64,11 @@ func test_use_of_initializes_accesses_on_non_inits() { set(initialValue) accesses(x) {} // expected-error@-1 {{accesses(...) attribute could only be used with init accessors}} } + + init(x: Int, y: String) { + self.y = y + self._x = x + } } } @@ -103,6 +109,13 @@ func test_assignment_to_let_properties() { self.x = initialValue.0 // Ok self.y = initialValue.1 // Ok } + + get { (x, y) } + set { } + } + + init(x: Int, y: Int) { + self.point = (x, y) } } } @@ -116,6 +129,12 @@ func test_duplicate_and_computed_lazy_properties() { init(initialValue) initializes(_b, _a) accesses(_a) { // expected-error@-1 {{property '_a' cannot be both initialized and accessed}} } + + get { _a } + set { } + } + + init() { } } @@ -219,6 +238,8 @@ func test_invalid_references() { } func test() -> Int? { 42 } + + init() {} } } @@ -335,3 +356,19 @@ func test_memberwise_with_overlaps() { _ = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) // Ok } + +func test_invalid_memberwise() { + struct Test1 { // expected-error {{cannot synthesize memberwise initializer}} + var _a: Int + var _b: 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}} + _a = initialValue + } + + get { _a } + } + } +} From cffc3fd73d099132cdbf496394a474cfec8015cd Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Tue, 13 Jun 2023 12:01:23 -0700 Subject: [PATCH 08/10] [Sema/SILGen] InitAccessors: Don't synthesize memberwise init if `initializes` intersect If some property is initializable by one than one init accessor let's not sythesize a memberwise initializer in that case because it's ambiguous what is the best init accessor to use. --- lib/SILGen/SILGenConstructor.cpp | 45 ++----- lib/Sema/CodeSynthesis.cpp | 25 ++-- test/Interpreter/init_accessors.swift | 122 ------------------ .../init_accessor_raw_sil_lowering.swift | 62 --------- test/decl/var/init_accessors.swift | 11 +- 5 files changed, 36 insertions(+), 229 deletions(-) diff --git a/lib/SILGen/SILGenConstructor.cpp b/lib/SILGen/SILGenConstructor.cpp index 1c510e0561103..945441484c9b4 100644 --- a/lib/SILGen/SILGenConstructor.cpp +++ b/lib/SILGen/SILGenConstructor.cpp @@ -275,22 +275,14 @@ static RValue maybeEmitPropertyWrapperInitFromValue( static void emitApplyOfInitAccessor(SILGenFunction &SGF, SILLocation loc, AccessorDecl *accessor, SILValue selfValue, - SILType selfTy, RValue &&initialValue, - llvm::SmallPtrSetImpl &initializedFields) { + SILType selfTy, RValue &&initialValue) { SmallVector arguments; auto emitFieldReference = [&](VarDecl *field, bool forInit = false) { auto fieldTy = selfTy.getFieldType(field, SGF.SGM.M, SGF.getTypeExpansionContext()); - auto fieldAddr = SGF.B.createStructElementAddr(loc, selfValue, field, - fieldTy.getAddressType()); - - // If another init accessor already initialized this field then we need - // to emit destroy before calling this accessor. - if (forInit && !initializedFields.insert(field).second) - SGF.B.emitDestroyAddr(loc, fieldAddr); - - return fieldAddr; + return SGF.B.createStructElementAddr(loc, selfValue, field, + fieldTy.getAddressType()); }; // First, let's emit all of the indirect results. @@ -411,35 +403,24 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, // Tracks all the init accessors we have emitted // because they can initialize more than one property. llvm::SmallPtrSet emittedInitAccessors; - // Tracks all the fields that have been already initialized - // via an init accessor. - llvm::SmallPtrSet fieldsInitializedViaAccessor; - auto elti = elements.begin(), eltEnd = elements.end(); for (VarDecl *field : decl->getStoredProperties()) { // Handle situations where this stored propery is initialized // via a call to an init accessor on some other property. - if (initializedViaAccessor.count(field)) { - for (auto iter = initializedViaAccessor.find(field); - iter != initializedViaAccessor.end(); ++iter) { - auto *initProperty = iter->second; - auto *initAccessor = initProperty->getAccessor(AccessorKind::Init); - - if (!emittedInitAccessors.insert(initAccessor).second) - continue; + if (initializedViaAccessor.count(field) == 1) { + auto *initProperty = initializedViaAccessor.find(field)->second; + auto *initAccessor = initProperty->getAccessor(AccessorKind::Init); - assert(elti != eltEnd && - "number of args does not match number of fields"); + if (!emittedInitAccessors.insert(initAccessor).second) + continue; - emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy, - std::move(*elti), - fieldsInitializedViaAccessor); - ++elti; - } + assert(elti != eltEnd && + "number of args does not match number of fields"); - // After all init accessors are emitted let's move on to the next - // property. + emitApplyOfInitAccessor(SGF, Loc, initAccessor, resultSlot, selfTy, + std::move(*elti)); + ++elti; continue; } diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp index db8bdc7c3c973..f918286f5f8ab 100644 --- a/lib/Sema/CodeSynthesis.cpp +++ b/lib/Sema/CodeSynthesis.cpp @@ -320,14 +320,10 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl, continue; // Check whether this property could be initialized via init accessor. - if (initializedViaAccessor.count(var)) { - for (auto iter = initializedViaAccessor.find(var); - iter != initializedViaAccessor.end(); ++iter) { - auto *initializerProperty = iter->second; - - if (usedInitProperties.insert(initializerProperty).second) - createParameter(initializerProperty); - } + if (initializedViaAccessor.count(var) == 1) { + auto *initializerProperty = initializedViaAccessor.find(var)->second; + if (usedInitProperties.insert(initializerProperty).second) + createParameter(initializerProperty); } else { createParameter(var); } @@ -1337,9 +1333,20 @@ HasMemberwiseInitRequest::evaluate(Evaluator &evaluator, if (initializedViaAccessor.empty()) return true; - if (!initializedViaAccessor.count(var)) { + 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 diff --git a/test/Interpreter/init_accessors.swift b/test/Interpreter/init_accessors.swift index 765c6a9323e0e..69e5f52ea99fa 100644 --- a/test/Interpreter/init_accessors.swift +++ b/test/Interpreter/init_accessors.swift @@ -332,125 +332,3 @@ 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_with_overlaps() { - struct Test1 { - var _a: T - var _b: Int - - var a: T { - init(initialValue) initializes(_a) { - _a = initialValue - } - - get { _a } - set { } - } - - var pair: (T, Int) { - init(initialValue) initializes(_a, _b) { - _a = initialValue.0 - _b = initialValue.1 - } - - get { (_a, _b) } - set { } - } - - var c: U - } - - let test1 = Test1(a: "a", pair: ("b", 1), c: [3.0]) - print("memberwise-overlaps-1: \(test1)") - - struct Test2 { - var _a: T - var _b: Int - - var a: T { - init(initialValue) initializes(_a) { - _a = initialValue - } - - get { _a } - set { } - } - - var b: Int { - init(initialValue) initializes(_b) { - _b = initialValue - } - - get { _b } - set { } - } - - var _c: U - - var pair: (T, U) { - init(initialValue) initializes(_a, _c) { - _a = initialValue.0 - _c = initialValue.1 - } - - get { (_a, _c) } - set { } - } - } - - let test2 = Test2(a: "a", pair: ("c", 2), b: 0) - print("memberwise-overlaps-2: \(test2)") - - struct Test3 { - var _a: T - var _b: Int - - var a: T { - init(initialValue) initializes(_a) { - _a = initialValue - } - - get { _a } - set { } - } - - var b: Int { - init(initialValue) initializes(_b) { - _b = initialValue - } - - get { _b } - set { } - } - - var _c: U - - var c: U { - init(initialValue) initializes(_c) { - _c = initialValue - } - - get { _c } - set { } - } - - var triple: (T, Int, U) { - init(initialValue) initializes(_a, _b, _c) { - _a = initialValue.0 - _b = initialValue.1 - _c = initialValue.2 - } - - get { (_a, _b, _c) } - set { } - } - } - - let test3 = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) - print("memberwise-overlaps-3: \(test3)") -} - -test_memberwise_with_overlaps() -// CHECK: memberwise-overlaps-1: Test1>(_a: "b", _b: 1, c: [3.0]) -// CHECK-NEXT: memberwise-overlaps-2: Test2(_a: "c", _b: 0, _c: 2) -// CHECK-NEXT: memberwise-overlaps-3: Test3>(_a: "b", _b: 0, _c: [1.0]) diff --git a/test/SILOptimizer/init_accessor_raw_sil_lowering.swift b/test/SILOptimizer/init_accessor_raw_sil_lowering.swift index 92816c958c5bb..f12398a4d37d9 100644 --- a/test/SILOptimizer/init_accessor_raw_sil_lowering.swift +++ b/test/SILOptimizer/init_accessor_raw_sil_lowering.swift @@ -2,68 +2,6 @@ // REQUIRES: asserts -// Makes sure that SILGen for memberwise initializer has destroy_addr for overlapping properties. -struct TestMemberwiseWithOverlap { - var _a: Int - var _b: Int - var _c: Int - - var a: Int { - init(initialValue) initializes(_a) { - _a = initialValue - } - - get { _a } - set { } - } - - var pair: (Int, Int) { - init(initialValue) initializes(_a, _b) { - _a = initialValue.0 - _b = initialValue.1 - } - - get { (_a, _b) } - set { } - } - - var c: Int { - init(initialValue) initializes(_b, _c) accesses(_a) { - _b = -1 - _c = initialValue - } - - get { _c } - set { } - } - - // CHECK-LABEL: sil hidden [ossa] @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1a4pair1cACSi_Si_SitSitcfC : $@convention(method) (Int, Int, Int, Int, @thin TestMemberwiseWithOverlap.Type) -> TestMemberwiseWithOverlap - // - // CHECK: [[SELF:%.*]] = alloc_stack $TestMemberwiseWithOverlap - // - // CHECK: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a - // CHECK: [[A_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1aSivi : $@convention(thin) (Int) -> @out Int - // CHECK-NEXT: {{.*}} = apply [[A_INIT]]([[A_REF]], %0) : $@convention(thin) (Int) -> @out Int - // - // CHECK: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a - // CHECK-NEXT: destroy_addr [[A_REF]] : $*Int - // CHECK-NEXT: [[B_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._b - // CHECK: [[PAIR_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV4pairSi_Sitvi : $@convention(thin) (Int, Int) -> (@out Int, @out Int) - // CHECK-NEXT: {{.*}} = apply [[PAIR_INIT]]([[A_REF]], [[B_REF]], %1, %2) : $@convention(thin) (Int, Int) -> (@out Int, @out Int) - // - // CHECK: [[B_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._b - // CHECK-NEXT: destroy_addr [[B_REF]] : $*Int - // CHECK-NEXT: [[C_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._c - // CHECK-NEXT: [[A_REF:%.*]] = struct_element_addr [[SELF]] : $*TestMemberwiseWithOverlap, #TestMemberwiseWithOverlap._a - // CHECK: [[C_INIT:%.*]] = function_ref @$s23assign_or_init_lowering25TestMemberwiseWithOverlapV1cSivi : $@convention(thin) (Int, @inout Int) -> (@out Int, @out Int) - // CHECK-NEXT: {{.*}} = apply [[C_INIT]]([[B_REF]], [[C_REF]], %3, [[A_REF]]) : $@convention(thin) (Int, @inout Int) -> (@out Int, @out Int) - // CHECK-NEXT: [[RESULT:%.*]] = load [trivial] [[SELF]] : $*TestMemberwiseWithOverlap - // CHECK-NEXT: dealloc_stack [[SELF]] : $*TestMemberwiseWithOverlap - // CHECK-NEXT: return [[RESULT]] : $TestMemberwiseWithOverlap -} - -_ = TestMemberwiseWithOverlap(a: 0, pair: (1, 2), c: 3) - struct Test1 { var _a: Int var _b: String diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index 0b1765d74c0cd..ae44b7ff82ff1 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -243,7 +243,7 @@ func test_invalid_references() { } } -func test_memberwise_with_overlaps() { +func test_memberwise_with_overlaps_dont_synthesize_inits() { struct Test1 { var _a: T var _b: Int @@ -270,7 +270,8 @@ func test_memberwise_with_overlaps() { var c: U } - _ = Test1(a: "a", pair: ("b", 1), c: [3.0]) // Ok + _ = Test1(a: "a", pair: ("b", 1), c: [3.0]) + // expected-error@-1 {{'Test1' cannot be constructed because it has no accessible initializers}} struct Test2 { var _a: T @@ -307,7 +308,8 @@ func test_memberwise_with_overlaps() { } } - _ = Test2(a: "a", pair: ("c", 2), b: 0) // Ok + _ = Test2(a: "a", pair: ("c", 2), b: 0) + // expected-error@-1 {{'Test2' cannot be constructed because it has no accessible initializers}} struct Test3 { var _a: T @@ -354,7 +356,8 @@ func test_memberwise_with_overlaps() { } } - _ = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) // Ok + _ = Test3(a: "a", triple: ("b", 2, [1.0, 2.0]), b: 0, c: [1.0]) + // expected-error@-1 {{'Test3' cannot be constructed because it has no accessible initializers}} } func test_invalid_memberwise() { From 6ca9728079585aaa961c32f15c9073f7e92bd2e1 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Tue, 13 Jun 2023 13:50:08 -0700 Subject: [PATCH 09/10] [AST] InitAccessors: Mark properties that init other properties as memberwise initializable --- lib/AST/Decl.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 47dd732090d67..6373df6bb389b 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -7132,6 +7132,12 @@ bool VarDecl::isMemberwiseInitialized(bool preferDeclaredProperties) const { isBackingStorageForDeclaredProperty(this)) return false; + // If this is a computed property with `init` accessor, it's + // memberwise initializable when it could be used to initialize + // other stored properties. + if (auto *init = getAccessor(AccessorKind::Init)) + return init->getAttrs().hasAttribute(); + // If this is a computed property, it's not memberwise initialized unless // the caller has asked for the declared properties and it is either a // `lazy` property or a property with an attached wrapper. From 34c8cf60f18bbe8dbe9a28c870b4ef1a3cfa363f Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Tue, 13 Jun 2023 13:56:30 -0700 Subject: [PATCH 10/10] [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. --- lib/SILGen/SILGenConstructor.cpp | 42 +++++++++------ lib/Sema/CodeSynthesis.cpp | 70 +++++++++++-------------- test/Interpreter/init_accessors.swift | 61 ++++++++++++++++++++++ test/decl/var/init_accessors.swift | 73 ++++++++++++++++++++++++++- 4 files changed, 187 insertions(+), 59 deletions(-) diff --git a/lib/SILGen/SILGenConstructor.cpp b/lib/SILGen/SILGenConstructor.cpp index 945441484c9b4..8631323e84b30 100644 --- a/lib/SILGen/SILGenConstructor.cpp +++ b/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 emittedInitAccessors; auto elti = elements.begin(), eltEnd = elements.end(); - for (VarDecl *field : decl->getStoredProperties()) { + + llvm::SmallPtrSet storedProperties; + { + auto properties = decl->getStoredProperties(); + storedProperties.insert(properties.begin(), properties.end()); + } + + for (auto *member : decl->getMembers()) { + auto *field = dyn_cast(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()); diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp index f918286f5f8ab..364c3cbade25d 100644 --- a/lib/Sema/CodeSynthesis.cpp +++ b/lib/Sema/CodeSynthesis.cpp @@ -303,14 +303,6 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl, std::multimap 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 usedInitProperties; for (auto member : decl->getMembers()) { auto var = dyn_cast(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(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()) { + initAccessor->getAttrs().getAttribute()) { 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()) { + initAccessor->getAttrs().getAttribute()) { 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; } } } diff --git a/test/Interpreter/init_accessors.swift b/test/Interpreter/init_accessors.swift index 69e5f52ea99fa..df8be55a75d2f 100644 --- a/test/Interpreter/init_accessors.swift +++ b/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) diff --git a/test/decl/var/init_accessors.swift b/test/decl/var/init_accessors.swift index ae44b7ff82ff1..6d8633bac190f 100644 --- a/test/decl/var/init_accessors.swift +++ b/test/decl/var/init_accessors.swift @@ -360,11 +360,26 @@ func test_memberwise_with_overlaps_dont_synthesize_inits() { // expected-error@-1 {{'Test3' 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 }