From fe7049ed13abd21fb6d1aa398a891d82f3cd584d Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Tue, 27 Feb 2024 15:01:20 -0800 Subject: [PATCH] SIL: More accurate for type lowering whether a type is trivial based on conditional Copyable requirements. We want a conditionally-copyable type to still be classified as trivial in cases where it's bitwise-copyable, has a trivial deinit, and is Copyable. The previous implementation here only checked at the declaration level whether a type was Copyable or not; get a more accurate answer by consulting the combination of information in the substituted type and abstraction pattern we have available during type lowering so that we classify definitely-copyable substitutions of a conditionally-copyable type as trivial. Should fix rdar://123654553 and rdar://123658878. --- include/swift/AST/Module.h | 29 +++++++++- include/swift/AST/Requirement.h | 6 ++ include/swift/SIL/AbstractionPattern.h | 1 + lib/AST/ConformanceLookup.cpp | 20 ++++++- lib/AST/Requirement.cpp | 37 ++++++++++-- lib/SIL/IR/AbstractionPattern.cpp | 77 +++++++++++++++++++++++++ lib/SIL/IR/TypeLowering.cpp | 6 +- test/SILGen/typelowering_inverses.swift | 54 +++++++++++++++-- 8 files changed, 215 insertions(+), 15 deletions(-) diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h index 8dccdf6c447ca..22b8f1a2b974b 100644 --- a/include/swift/AST/Module.h +++ b/include/swift/AST/Module.h @@ -846,6 +846,7 @@ class ModuleDecl bool allowMissing = false); /// Global conformance lookup, checks conditional requirements. + /// Requires a contextualized type. /// /// \param type The type for which we are computing conformance. Must not /// contain type parameters. @@ -859,8 +860,32 @@ class ModuleDecl /// \returns An invalid conformance if the search failed, otherwise an /// abstract, concrete or pack conformance, depending on the lookup type. ProtocolConformanceRef checkConformance(Type type, ProtocolDecl *protocol, - // Note: different default than above - bool allowMissing = true); + // Note: different default from lookupConformance + bool allowMissing = true); + + /// Global conformance lookup, checks conditional requirements. + /// Accepts interface types without context. If the conformance cannot be + /// definitively established without the missing context, returns \c nullopt. + /// + /// \param type The type for which we are computing conformance. Must not + /// contain type parameters. + /// + /// \param protocol The protocol to which we are computing conformance. + /// + /// \param allowMissing When \c true, the resulting conformance reference + /// might include "missing" conformances, which are synthesized for some + /// protocols as an error recovery mechanism. + /// + /// \returns An invalid conformance if the search definitively failed. An + /// abstract, concrete or pack conformance, depending on the lookup type, + /// if the search succeeded. `std::nullopt` if the type could have + /// conditionally conformed depending on the context of the interface types. + std::optional + checkConformanceWithoutContext(Type type, + ProtocolDecl *protocol, + // Note: different default from lookupConformance + bool allowMissing = true); + /// Look for the conformance of the given existential type to the given /// protocol. diff --git a/include/swift/AST/Requirement.h b/include/swift/AST/Requirement.h index 55a9d301121b0..0e94a04c66375 100644 --- a/include/swift/AST/Requirement.h +++ b/include/swift/AST/Requirement.h @@ -223,6 +223,12 @@ enum class CheckRequirementsResult : uint8_t { /// not contain any type parameters. CheckRequirementsResult checkRequirements(ArrayRef requirements); +/// Check if each substituted requirement is satisfied. If the requirement +/// contains type parameters, and the answer would depend on the context of +/// those type parameters, then `nullopt` is returned. +std::optional +checkRequirementsWithoutContext(ArrayRef requirements); + /// Check if each requirement is satisfied after applying the given /// substitutions. The substitutions must replace all type parameters that /// appear in the requirement with concrete types or archetypes. diff --git a/include/swift/SIL/AbstractionPattern.h b/include/swift/SIL/AbstractionPattern.h index b9a46d488213f..3c2572a13a2eb 100644 --- a/include/swift/SIL/AbstractionPattern.h +++ b/include/swift/SIL/AbstractionPattern.h @@ -1014,6 +1014,7 @@ class AbstractionPattern { bool requiresClass() const; LayoutConstraint getLayoutConstraint() const; + bool isNoncopyable(CanType substTy) const; /// Return the Swift type which provides structure for this /// abstraction pattern. diff --git a/lib/AST/ConformanceLookup.cpp b/lib/AST/ConformanceLookup.cpp index 4509432107387..c88008ab273c5 100644 --- a/lib/AST/ConformanceLookup.cpp +++ b/lib/AST/ConformanceLookup.cpp @@ -796,8 +796,20 @@ LookupConformanceInModuleRequest::evaluate( ProtocolConformanceRef ModuleDecl::checkConformance(Type type, ProtocolDecl *proto, bool allowMissing) { - assert(!type->hasTypeParameter()); + assert(!type->hasTypeParameter() + && "must take a contextual type. if you really are ok with an " + "indefinite answer (and usually YOU ARE NOT), then consider whether " + "you really, definitely are ok with an indefinite answer, and " + "use `checkConformanceWithoutContext` instead"); + + // With no type parameter in the type, we should always get a definite answer + // from the underlying test. + return checkConformanceWithoutContext(type, proto, allowMissing).value(); +} +std::optional +ModuleDecl::checkConformanceWithoutContext(Type type, ProtocolDecl *proto, + bool allowMissing) { auto lookupResult = lookupConformance(type, proto, allowMissing); if (lookupResult.isInvalid()) { return ProtocolConformanceRef::forInvalid(); @@ -807,7 +819,11 @@ ModuleDecl::checkConformance(Type type, ProtocolDecl *proto, // If we have a conditional requirements that we need to check, do so now. if (!condReqs.empty()) { - switch (checkRequirements(condReqs)) { + auto reqResult = checkRequirementsWithoutContext(condReqs); + if (!reqResult.has_value()) { + return std::nullopt; + } + switch (*reqResult) { case CheckRequirementsResult::Success: break; diff --git a/lib/AST/Requirement.cpp b/lib/AST/Requirement.cpp index 5d9b3ba49990a..ca2cb79567240 100644 --- a/lib/AST/Requirement.cpp +++ b/lib/AST/Requirement.cpp @@ -246,7 +246,9 @@ int Requirement::compare(const Requirement &other) const { return compareProtos; } -CheckRequirementsResult swift::checkRequirements(ArrayRef requirements) { +static std::optional +checkRequirementsImpl(ArrayRef requirements, + bool allowTypeParameters) { SmallVector worklist(requirements.begin(), requirements.end()); bool hadSubstFailure = false; @@ -258,12 +260,20 @@ CheckRequirementsResult swift::checkRequirements(ArrayRef requireme #ifndef NDEBUG { auto firstType = req.getFirstType(); - assert(!firstType->hasTypeParameter()); + assert((allowTypeParameters || !firstType->hasTypeParameter()) + && "must take a contextual type. if you really are ok with an " + "indefinite answer (and usually YOU ARE NOT), then consider whether " + "you really, definitely are ok with an indefinite answer, and " + "use `checkRequirementsWithoutContext` instead"); assert(!firstType->hasTypeVariable()); if (req.getKind() != RequirementKind::Layout) { auto secondType = req.getSecondType(); - assert(!secondType->hasTypeParameter()); + assert((allowTypeParameters || !secondType->hasTypeParameter()) + && "must take a contextual type. if you really are ok with an " + "indefinite answer (and usually YOU ARE NOT), then consider whether " + "you really, definitely are ok with an indefinite answer, and " + "use `checkRequirementsWithoutContext` instead"); assert(!secondType->hasTypeVariable()); } } @@ -276,6 +286,12 @@ CheckRequirementsResult swift::checkRequirements(ArrayRef requireme break; case CheckRequirementResult::RequirementFailure: + // If a requirement failure was caused by a context-free type parameter, + // then we can't definitely know whether it would have satisfied the + // requirement without context. + if (req.getFirstType()->isTypeParameter()) { + return std::nullopt; + } return CheckRequirementsResult::RequirementFailure; case CheckRequirementResult::SubstitutionFailure: @@ -290,6 +306,19 @@ CheckRequirementsResult swift::checkRequirements(ArrayRef requireme return CheckRequirementsResult::Success; } +CheckRequirementsResult +swift::checkRequirements(ArrayRef requirements) { + // This entry point requires that there are no type parameters in any of the + // requirements, so the underlying check should always produce a result. + return checkRequirementsImpl(requirements, /*allow type parameters*/ false) + .value(); +} + +std::optional +swift::checkRequirementsWithoutContext(ArrayRef requirements) { + return checkRequirementsImpl(requirements, /*allow type parameters*/ true); +} + CheckRequirementsResult swift::checkRequirements( ModuleDecl *module, ArrayRef requirements, TypeSubstitutionFn substitutions, SubstOptions options) { @@ -332,4 +361,4 @@ void InverseRequirement::expandDefaults( SourceLoc()}); } } -} \ No newline at end of file +} diff --git a/lib/SIL/IR/AbstractionPattern.cpp b/lib/SIL/IR/AbstractionPattern.cpp index 7288ec44cc315..d7de0f6551626 100644 --- a/lib/SIL/IR/AbstractionPattern.cpp +++ b/lib/SIL/IR/AbstractionPattern.cpp @@ -284,6 +284,83 @@ LayoutConstraint AbstractionPattern::getLayoutConstraint() const { } } +bool AbstractionPattern::isNoncopyable(CanType substTy) const { + auto copyable + = substTy->getASTContext().getProtocol(KnownProtocolKind::Copyable); + + auto isDefinitelyCopyable = [&](CanType t) -> bool { + auto result = copyable->getParentModule() + ->checkConformanceWithoutContext(substTy, copyable, + /*allowMissing=*/false); + return result.has_value() && !result.value().isInvalid(); + }; + + // If the substituted type definitely conforms, that's authoritative. + if (isDefinitelyCopyable(substTy)) { + return false; + } + + // If the substituted type is fully concrete, that's it. If there are unbound + // type variables in the type, then we may have to account for the upper + // abstraction bound from the abstraction pattern. + if (!substTy->hasTypeParameter()) { + return true; + } + + switch (getKind()) { + case Kind::Opaque: { + // The abstraction pattern doesn't provide any more specific bounds. + return true; + } + case Kind::Type: + case Kind::Discard: + case Kind::ClangType: { + // See whether the abstraction pattern's context gives us an upper bound + // that ensures the type is copyable. + auto type = getType(); + if (hasGenericSignature() && getType()->hasTypeParameter()) { + type = GenericEnvironment::mapTypeIntoContext( + getGenericSignature().getGenericEnvironment(), getType()) + ->getReducedType(getGenericSignature()); + } + + return !isDefinitelyCopyable(type); + } + case Kind::Tuple: { + // A tuple is noncopyable if any element is. + if (doesTupleVanish()) { + return getVanishingTupleElementPatternType().value() + .isNoncopyable(substTy); + } + auto substTupleTy = cast(substTy); + + for (unsigned i = 0, e = getNumTupleElements(); i < e; ++i) { + if (getTupleElementType(i).isNoncopyable(substTupleTy.getElementType(i))){ + return true; + } + } + return false; + } + // Functions are, at least for now, always copyable. + case Kind::CurriedObjCMethodType: + case Kind::PartialCurriedObjCMethodType: + case Kind::CFunctionAsMethodType: + case Kind::CurriedCFunctionAsMethodType: + case Kind::PartialCurriedCFunctionAsMethodType: + case Kind::ObjCMethodType: + case Kind::ObjCCompletionHandlerArgumentsType: + case Kind::CXXMethodType: + case Kind::CurriedCXXMethodType: + case Kind::PartialCurriedCXXMethodType: + case Kind::OpaqueFunction: + case Kind::OpaqueDerivativeFunction: + return false; + + case Kind::Invalid: + llvm_unreachable("asking invalid abstraction pattern"); + } +} + bool AbstractionPattern::matchesTuple(CanType substType) const { switch (getKind()) { case Kind::Invalid: diff --git a/lib/SIL/IR/TypeLowering.cpp b/lib/SIL/IR/TypeLowering.cpp index c49e75eb43470..619b548a6157c 100644 --- a/lib/SIL/IR/TypeLowering.cpp +++ b/lib/SIL/IR/TypeLowering.cpp @@ -2353,7 +2353,7 @@ namespace { return handleReference(classType, properties); } - + // WARNING: when the specification of trivial types changes, also update // the isValueTrivial() API used by SILCombine. TypeLowering *visitAnyStructType(CanType structType, @@ -2433,7 +2433,7 @@ namespace { properties = applyLifetimeAnnotation(D->getLifetimeAnnotation(), properties); - if (D->canBeCopyable() != TypeDecl::CanBeInvertible::Always) { + if (origType.isNoncopyable(structType)) { properties.setNonTrivial(); properties.setLexical(IsLexical); if (properties.isAddressOnly()) @@ -2530,7 +2530,7 @@ namespace { properties = applyLifetimeAnnotation(D->getLifetimeAnnotation(), properties); - if (D->canBeCopyable() != TypeDecl::CanBeInvertible::Always) { + if (origType.isNoncopyable(enumType)) { properties.setNonTrivial(); properties.setLexical(IsLexical); if (properties.isAddressOnly()) diff --git a/test/SILGen/typelowering_inverses.swift b/test/SILGen/typelowering_inverses.swift index 384e5508aae44..bbc5d45108769 100644 --- a/test/SILGen/typelowering_inverses.swift +++ b/test/SILGen/typelowering_inverses.swift @@ -1,7 +1,5 @@ // RUN: %target-swift-frontend -emit-silgen -enable-experimental-feature NoncopyableGenerics -disable-availability-checking -module-name main %s | %FileCheck %s - - struct NC: ~Copyable {} struct RudeStruct: Copyable { @@ -34,13 +32,13 @@ func check(_ t: RudeEnum) {} // CHECK: sil hidden [ossa] @$s4main5checkyyAA8RudeEnumOyAA2NCVGF : $@convention(thin) (RudeEnum) -> () { func check(_ t: RudeEnum) {} -// CHECK: sil hidden [ossa] @$s4main5checkyyAA18CondCopyableStructVySiGF : $@convention(thin) (@guaranteed CondCopyableStruct) -> () { +// CHECK: sil hidden [ossa] @$s4main5checkyyAA18CondCopyableStructVySiGF : $@convention(thin) (CondCopyableStruct) -> () { func check(_ t: CondCopyableStruct) {} // CHECK: sil hidden [ossa] @$s4main5checkyyAA18CondCopyableStructVyAA2NCVGF : $@convention(thin) (@guaranteed CondCopyableStruct) -> () { func check(_ t: borrowing CondCopyableStruct) {} -// CHECK: sil hidden [ossa] @$s4main5checkyyAA16CondCopyableEnumOySiGF : $@convention(thin) (@guaranteed CondCopyableEnum) -> () { +// CHECK: sil hidden [ossa] @$s4main5checkyyAA16CondCopyableEnumOySiGF : $@convention(thin) (CondCopyableEnum) -> () { func check(_ t: CondCopyableEnum) {} // CHECK: sil hidden [ossa] @$s4main5checkyyAA16CondCopyableEnumOyAA2NCVGF : $@convention(thin) (@guaranteed CondCopyableEnum) -> () { @@ -51,3 +49,51 @@ func check(_ t: CondCopyableEnum) {} // CHECK: sil hidden [ossa] @$s4main13check_noClashyyAA16CondCopyableEnumOyxGlF : $@convention(thin) (@in_guaranteed CondCopyableEnum) -> () { func check_noClash(_ t: borrowing CondCopyableEnum) {} + +struct MyStruct: ~Copyable { + var x: T +} + +extension MyStruct: Copyable where T: Copyable {} + +enum MyEnum: ~Copyable { + case x(T) + case knoll +} + +extension MyEnum: Copyable where T: Copyable {} + +enum Trivial { + case a, b, c +} + +// CHECK-LABEL: sil{{.*}} @{{.*}}13trivialStruct +func trivialStruct() -> Int { + // CHECK: [[ALLOC:%.*]] = alloc_stack $MyStruct + // CHECK-NOT: destroy_addr [[ALLOC]] : + // CHECK: dealloc_stack [[ALLOC]] : + return MemoryLayout.size(ofValue: MyStruct(x: Trivial.a)) +} +// CHECK-LABEL: sil{{.*}} @{{.*}}11trivialEnum +func trivialEnum() -> Int { + // CHECK: [[ALLOC:%.*]] = alloc_stack $MyEnum + // CHECK-NOT: destroy_addr [[ALLOC]] : + // CHECK: dealloc_stack [[ALLOC]] : + return MemoryLayout.size(ofValue: MyEnum.x(Trivial.a)) +} + +struct MyAssortment { + var a: MyStruct + var b: MyEnum +} + +// CHECK-LABEL: sil{{.*}} @{{.*}}4frob +func frob(x: MyAssortment) -> Int { + // CHECK: [[ALLOC:%.*]] = alloc_stack $MyAssortment + // CHECK-NOT: destroy_addr [[ALLOC]] : + // CHECK: dealloc_stack [[ALLOC]] : + return MemoryLayout.size(ofValue: x) +} + +extension MyEnum: _BitwiseCopyable + where T: Copyable & _BitwiseCopyable {}