From 91ebe4485ca4a6f1846f1ef25aca1d8b164958f0 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Thu, 11 Mar 2021 17:41:41 -0500 Subject: [PATCH] GSB: Move signature verification from CanGenericSignature::getCanonical() to computeGenericSignature() Doing this when computing a canonical signature didn't really make sense because canonical signatures are not canonicalized any more strongly _with respect to the builder_; they just canonicalize their requirement types. Instead, let's do these checks after creating the signature in computeGenericSignature(). The old behavior had another undesirable property; since the canonicalization was done by registerGenericSignatureBuilder(), we would always build a new GSB from scratch for every signature we compute. The new location also means we do these checks for protocol requirement signatures as well. This flags an existing fixed crasher where we still emit bogus same-type requirements in the requirement signature, so I moved this test back into an unfixed state. --- include/swift/AST/GenericSignature.h | 10 +- lib/AST/GenericSignature.cpp | 137 ++---------------- lib/AST/GenericSignatureBuilder.cpp | 129 +++++++++++++++++ lib/Sema/TypeCheckDeclPrimary.cpp | 5 +- .../compiler_crashers_2/sr8968.swift | 48 ++++++ .../compiler_crashers_2_fixed/sr8968.swift | 46 ------ 6 files changed, 197 insertions(+), 178 deletions(-) create mode 100644 validation-test/compiler_crashers_2/sr8968.swift delete mode 100644 validation-test/compiler_crashers_2_fixed/sr8968.swift diff --git a/include/swift/AST/GenericSignature.h b/include/swift/AST/GenericSignature.h index fa1db48079e14..400060bdbe7b6 100644 --- a/include/swift/AST/GenericSignature.h +++ b/include/swift/AST/GenericSignature.h @@ -175,7 +175,7 @@ class CanGenericSignature : public GenericSignature { /// requirements, first canonicalizing the types. static CanGenericSignature getCanonical(TypeArrayView params, - ArrayRef requirements, bool skipValidation = false); + ArrayRef requirements); public: CanGenericSignature(std::nullptr_t) : GenericSignature(nullptr) {} @@ -342,6 +342,8 @@ class alignas(1 << TypeAlignInBits) GenericSignatureImpl final /// /// The type parameters must be known to not be concrete within the context. bool areSameTypeParameterInContext(Type type1, Type type2) const; + bool areSameTypeParameterInContext(Type type1, Type type2, + GenericSignatureBuilder &builder) const; /// Determine if \c sig can prove \c requirement, meaning that it can deduce /// T: Foo or T == U (etc.) with the information it knows. This includes @@ -360,12 +362,10 @@ class alignas(1 << TypeAlignInBits) GenericSignatureImpl final /// Return the canonical version of the given type under this generic /// signature. CanType getCanonicalTypeInContext(Type type) const; - bool isCanonicalTypeInContext(Type type) const; - - /// Return the canonical version of the given type under this generic - /// signature. CanType getCanonicalTypeInContext(Type type, GenericSignatureBuilder &builder) const; + + bool isCanonicalTypeInContext(Type type) const; bool isCanonicalTypeInContext(Type type, GenericSignatureBuilder &builder) const; diff --git a/lib/AST/GenericSignature.cpp b/lib/AST/GenericSignature.cpp index 0fd81519ba62e..9209ce1d437ed 100644 --- a/lib/AST/GenericSignature.cpp +++ b/lib/AST/GenericSignature.cpp @@ -176,23 +176,9 @@ bool GenericSignatureImpl::isCanonical() const { return getCanonicalSignature().getPointer() == this; } -#ifndef NDEBUG -/// Determine the canonical ordering of requirements. -static unsigned getRequirementKindOrder(RequirementKind kind) { - switch (kind) { - case RequirementKind::Conformance: return 2; - case RequirementKind::Superclass: return 0; - case RequirementKind::SameType: return 3; - case RequirementKind::Layout: return 1; - } - llvm_unreachable("unhandled kind"); -} -#endif - CanGenericSignature CanGenericSignature::getCanonical(TypeArrayView params, - ArrayRef requirements, - bool skipValidation) { + ArrayRef requirements) { // Canonicalize the parameters and requirements. SmallVector canonicalParams; canonicalParams.reserve(params.size()); @@ -205,116 +191,9 @@ CanGenericSignature::getCanonical(TypeArrayView params, for (auto &reqt : requirements) canonicalRequirements.push_back(reqt.getCanonical()); - (void)skipValidation; auto canSig = get(canonicalParams, canonicalRequirements, /*isKnownCanonical=*/true); -#ifndef NDEBUG - if (skipValidation) - return CanGenericSignature(canSig); - - PrettyStackTraceGenericSignature debugStack("canonicalizing", canSig); - - // Check that the signature is canonical. - for (unsigned idx : indices(canonicalRequirements)) { - debugStack.setRequirement(idx); - - const auto &reqt = canonicalRequirements[idx]; - - // Left-hand side must be canonical in its context. - // Check canonicalization of requirement itself. - switch (reqt.getKind()) { - case RequirementKind::Superclass: - assert(canSig->isCanonicalTypeInContext(reqt.getFirstType()) && - "Left-hand side is not canonical"); - assert(canSig->isCanonicalTypeInContext(reqt.getSecondType()) && - "Superclass type isn't canonical in its own context"); - break; - - case RequirementKind::Layout: - assert(canSig->isCanonicalTypeInContext(reqt.getFirstType()) && - "Left-hand side is not canonical"); - break; - - case RequirementKind::SameType: { - auto isCanonicalAnchor = [&](Type type) { - if (auto *dmt = type->getAs()) - return canSig->isCanonicalTypeInContext(dmt->getBase()); - return type->is(); - }; - - auto firstType = reqt.getFirstType(); - auto secondType = reqt.getSecondType(); - assert(isCanonicalAnchor(firstType)); - - if (reqt.getSecondType()->isTypeParameter()) { - assert(isCanonicalAnchor(secondType)); - assert(compareDependentTypes(firstType, secondType) < 0 && - "Out-of-order type parameters in same-type constraint"); - } else { - assert(canSig->isCanonicalTypeInContext(secondType) && - "Concrete same-type isn't canonical in its own context"); - } - break; - } - - case RequirementKind::Conformance: - assert(reqt.getFirstType()->isTypeParameter() && - "Left-hand side must be a type parameter"); - assert(isa(reqt.getSecondType().getPointer()) && - "Right-hand side of conformance isn't a protocol type"); - break; - } - - // From here on, we're only interested in requirements beyond the first. - if (idx == 0) continue; - - // Make sure that the left-hand sides are in nondecreasing order. - const auto &prevReqt = canonicalRequirements[idx-1]; - int compareLHS = - compareDependentTypes(prevReqt.getFirstType(), reqt.getFirstType()); - assert(compareLHS <= 0 && "Out-of-order left-hand sides"); - - // If we have two same-type requirements where the left-hand sides differ - // but fall into the same equivalence class, we can check the form. - if (compareLHS < 0 && reqt.getKind() == RequirementKind::SameType && - prevReqt.getKind() == RequirementKind::SameType && - canSig->areSameTypeParameterInContext(prevReqt.getFirstType(), - reqt.getFirstType())) { - // If it's a it's a type parameter, make sure the equivalence class is - // wired together sanely. - if (prevReqt.getSecondType()->isTypeParameter()) { - assert(prevReqt.getSecondType()->isEqual(reqt.getFirstType()) && - "same-type constraints within an equiv. class are out-of-order"); - } else { - // Otherwise, the concrete types must match up. - assert(prevReqt.getSecondType()->isEqual(reqt.getSecondType()) && - "inconsistent concrete same-type constraints in equiv. class"); - } - } - - // From here on, we only care about cases where the previous and current - // requirements have the same left-hand side. - if (compareLHS != 0) continue; - - // Check ordering of requirement kinds. - assert((getRequirementKindOrder(prevReqt.getKind()) <= - getRequirementKindOrder(reqt.getKind())) && - "Requirements for a given kind are out-of-order"); - - // From here on, we only care about the same requirement kind. - if (prevReqt.getKind() != reqt.getKind()) continue; - - assert(reqt.getKind() == RequirementKind::Conformance && - "Only conformance requirements can have multiples"); - - auto prevProto = prevReqt.getProtocolDecl(); - auto proto = reqt.getProtocolDecl(); - assert(TypeDecl::compare(prevProto, proto) < 0 && - "Out-of-order conformance requirements"); - } -#endif - return CanGenericSignature(canSig); } @@ -519,7 +398,19 @@ bool GenericSignatureImpl::areSameTypeParameterInContext(Type type1, if (type1.getPointer() == type2.getPointer()) return true; - auto &builder = *getGenericSignatureBuilder(); + return areSameTypeParameterInContext(type1, type2, + *getGenericSignatureBuilder()); +} + +bool GenericSignatureImpl::areSameTypeParameterInContext(Type type1, + Type type2, + GenericSignatureBuilder &builder) const { + assert(type1->isTypeParameter()); + assert(type2->isTypeParameter()); + + if (type1.getPointer() == type2.getPointer()) + return true; + auto equivClass1 = builder.resolveEquivalenceClass( type1, diff --git a/lib/AST/GenericSignatureBuilder.cpp b/lib/AST/GenericSignatureBuilder.cpp index 58e89fb78d26e..99b62d27bac9b 100644 --- a/lib/AST/GenericSignatureBuilder.cpp +++ b/lib/AST/GenericSignatureBuilder.cpp @@ -7948,6 +7948,129 @@ void GenericSignatureBuilder::addGenericSignature(GenericSignature sig) { addRequirement(reqt, FloatingRequirementSource::forAbstract(), nullptr); } +#ifndef NDEBUG + +/// Determine the canonical ordering of requirements. +static unsigned getRequirementKindOrder(RequirementKind kind) { + switch (kind) { + case RequirementKind::Conformance: return 2; + case RequirementKind::Superclass: return 0; + case RequirementKind::SameType: return 3; + case RequirementKind::Layout: return 1; + } + llvm_unreachable("unhandled kind"); +} + +static void checkGenericSignature(CanGenericSignature canSig, + GenericSignatureBuilder &builder) { + PrettyStackTraceGenericSignature debugStack("checking", canSig); + + auto canonicalRequirements = canSig->getRequirements(); + + // Check that the signature is canonical. + for (unsigned idx : indices(canonicalRequirements)) { + debugStack.setRequirement(idx); + + const auto &reqt = canonicalRequirements[idx]; + + // Left-hand side must be canonical in its context. + // Check canonicalization of requirement itself. + switch (reqt.getKind()) { + case RequirementKind::Superclass: + assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) && + "Left-hand side is not canonical"); + assert(canSig->isCanonicalTypeInContext(reqt.getSecondType(), builder) && + "Superclass type isn't canonical in its own context"); + break; + + case RequirementKind::Layout: + assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) && + "Left-hand side is not canonical"); + break; + + case RequirementKind::SameType: { + auto isCanonicalAnchor = [&](Type type) { + if (auto *dmt = type->getAs()) + return canSig->isCanonicalTypeInContext(dmt->getBase(), builder); + return type->is(); + }; + + auto firstType = reqt.getFirstType(); + auto secondType = reqt.getSecondType(); + assert(isCanonicalAnchor(firstType)); + + if (reqt.getSecondType()->isTypeParameter()) { + assert(isCanonicalAnchor(secondType)); + assert(compareDependentTypes(firstType, secondType) < 0 && + "Out-of-order type parameters in same-type constraint"); + } else { + assert(canSig->isCanonicalTypeInContext(secondType) && + "Concrete same-type isn't canonical in its own context"); + } + break; + } + + case RequirementKind::Conformance: + assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) && + "Left-hand side is not canonical"); + assert(reqt.getFirstType()->isTypeParameter() && + "Left-hand side must be a type parameter"); + assert(isa(reqt.getSecondType().getPointer()) && + "Right-hand side of conformance isn't a protocol type"); + break; + } + + // From here on, we're only interested in requirements beyond the first. + if (idx == 0) continue; + + // Make sure that the left-hand sides are in nondecreasing order. + const auto &prevReqt = canonicalRequirements[idx-1]; + int compareLHS = + compareDependentTypes(prevReqt.getFirstType(), reqt.getFirstType()); + assert(compareLHS <= 0 && "Out-of-order left-hand sides"); + + // If we have two same-type requirements where the left-hand sides differ + // but fall into the same equivalence class, we can check the form. + if (compareLHS < 0 && reqt.getKind() == RequirementKind::SameType && + prevReqt.getKind() == RequirementKind::SameType && + canSig->areSameTypeParameterInContext(prevReqt.getFirstType(), + reqt.getFirstType(), + builder)) { + // If it's a it's a type parameter, make sure the equivalence class is + // wired together sanely. + if (prevReqt.getSecondType()->isTypeParameter()) { + assert(prevReqt.getSecondType()->isEqual(reqt.getFirstType()) && + "same-type constraints within an equiv. class are out-of-order"); + } else { + // Otherwise, the concrete types must match up. + assert(prevReqt.getSecondType()->isEqual(reqt.getSecondType()) && + "inconsistent concrete same-type constraints in equiv. class"); + } + } + + // From here on, we only care about cases where the previous and current + // requirements have the same left-hand side. + if (compareLHS != 0) continue; + + // Check ordering of requirement kinds. + assert((getRequirementKindOrder(prevReqt.getKind()) <= + getRequirementKindOrder(reqt.getKind())) && + "Requirements for a given kind are out-of-order"); + + // From here on, we only care about the same requirement kind. + if (prevReqt.getKind() != reqt.getKind()) continue; + + assert(reqt.getKind() == RequirementKind::Conformance && + "Only conformance requirements can have multiples"); + + auto prevProto = prevReqt.getProtocolDecl(); + auto proto = reqt.getProtocolDecl(); + assert(TypeDecl::compare(prevProto, proto) < 0 && + "Out-of-order conformance requirements"); + } +} +#endif + GenericSignature GenericSignatureBuilder::computeGenericSignature( bool allowConcreteGenericParams, bool allowBuilderToMove) && { @@ -7961,6 +8084,12 @@ GenericSignature GenericSignatureBuilder::computeGenericSignature( // Form the generic signature. auto sig = GenericSignature::get(getGenericParams(), requirements); +#ifndef NDEBUG + if (!Impl->HadAnyError) { + checkGenericSignature(sig.getCanonicalSignature(), *this); + } +#endif + // When we can, move this generic signature builder to make it the canonical // builder, rather than constructing a new generic signature builder that // will produce the same thing. diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index c22425d25417e..5f9c4911fb802 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -2452,13 +2452,10 @@ class DeclChecker : public DeclVisitor { requirementsSig->print(llvm::errs()); llvm::errs() << "\n"; - // Note: One cannot canonicalize a requirement signature, because - // requirement signatures are necessarily missing requirements. llvm::errs() << "Canonical requirement signature: "; auto canRequirementSig = CanGenericSignature::getCanonical(requirementsSig->getGenericParams(), - requirementsSig->getRequirements(), - /*skipValidation=*/true); + requirementsSig->getRequirements()); canRequirementSig->print(llvm::errs()); llvm::errs() << "\n"; } diff --git a/validation-test/compiler_crashers_2/sr8968.swift b/validation-test/compiler_crashers_2/sr8968.swift new file mode 100644 index 0000000000000..b42be9911ef5e --- /dev/null +++ b/validation-test/compiler_crashers_2/sr8968.swift @@ -0,0 +1,48 @@ +// RUN: not --crash %target-swift-frontend -emit-ir %s + +// REQUIRES: asserts + +public class OFMAttachment : NativeInserting { + public typealias SnapshotType = Message.Attachment +} + +public protocol Snapshotting { + associatedtype NativeType: NativeInserting where NativeType.SnapshotType == Self + associatedtype RecordType: SnapshotRecord where RecordType.SnapshotType == Self + associatedtype ChangeType: SnapshotChange where ChangeType.SnapshotType == Self + + static var baseMessageName: String { get } +} + +public protocol NativeInserting { + associatedtype SnapshotType : Snapshotting where SnapshotType.NativeType == Self +} + +public protocol SnapshotRecord { + associatedtype SnapshotType : Snapshotting where SnapshotType.RecordType == Self + +} + +public protocol SnapshotChange { + associatedtype SnapshotType : Snapshotting where SnapshotType.ChangeType == Self +} + +public struct Message { + public enum Attachment : Snapshotting { + + public static var baseMessageName: String = "attachment" + + public typealias NativeType = OFMAttachment + public typealias RecordType = Record + public typealias ChangeType = Change + public struct Record : SnapshotRecord { + public typealias SnapshotType = Message.Attachment + } + + public struct Change : SnapshotChange { + public typealias SnapshotType = Message.Attachment + } + + } +} + diff --git a/validation-test/compiler_crashers_2_fixed/sr8968.swift b/validation-test/compiler_crashers_2_fixed/sr8968.swift deleted file mode 100644 index 3206e0dac9558..0000000000000 --- a/validation-test/compiler_crashers_2_fixed/sr8968.swift +++ /dev/null @@ -1,46 +0,0 @@ -// RUN: %target-swift-frontend -typecheck %s - -class OFMAttachment : NativeInserting { - typealias SnapshotType = Message.Attachment -} - -protocol Snapshotting { - associatedtype NativeType: NativeInserting where NativeType.SnapshotType == Self - associatedtype RecordType: SnapshotRecord where RecordType.SnapshotType == Self - associatedtype ChangeType: SnapshotChange where ChangeType.SnapshotType == Self - - static var baseMessageName: String { get } -} - -protocol NativeInserting { - associatedtype SnapshotType : Snapshotting where SnapshotType.NativeType == Self -} - -protocol SnapshotRecord { - associatedtype SnapshotType : Snapshotting where SnapshotType.RecordType == Self - -} - -protocol SnapshotChange { - associatedtype SnapshotType : Snapshotting where SnapshotType.ChangeType == Self -} - -struct Message { - enum Attachment : Snapshotting { - - static var baseMessageName: String = "attachment" - - typealias NativeType = OFMAttachment - typealias RecordType = Record - typealias ChangeType = Change - struct Record : SnapshotRecord { - typealias SnapshotType = Message.Attachment - } - - struct Change : SnapshotChange { - typealias SnapshotType = Message.Attachment - } - - } -} -