From 4903b67513c0b04a0ee756eecb24ff96283a1b67 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Mon, 12 Feb 2024 16:56:07 -0800 Subject: [PATCH 1/3] NFC: restore an assert I accidentally removed --- lib/Frontend/Frontend.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Frontend/Frontend.cpp b/lib/Frontend/Frontend.cpp index cef91349540ec..3c578b0043039 100644 --- a/lib/Frontend/Frontend.cpp +++ b/lib/Frontend/Frontend.cpp @@ -1498,8 +1498,8 @@ bool CompilerInstance::loadStdlibIfNeeded() { // If we failed to load, we should have already diagnosed. if (M->failedToLoad()) { -// assert(Diagnostics.hadAnyError() && -// "Module failed to load but nothing was diagnosed?"); + assert(Diagnostics.hadAnyError() && + "stdlib module failed to load but nothing was diagnosed?"); return true; } return false; From 98e6c7b9358f859cc64c76d3692efa5ea1999152 Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Mon, 12 Feb 2024 12:06:10 -0800 Subject: [PATCH 2/3] ConformanceLookup: filter ReferenceStorageType There's a few uses of ReferenceStorageTypes being substituted for generic parameters, at least in the test suite, such as `Optional<@sil_unmanaged ..>` and just plain `<@sil_unowned ..>`. Before, conformance lookup could (and did) give bogus answers when asked if the type satisfies any conformance requirements. Now with NoncopyableGenerics, we will interpret such conformance lookups as being asked of the referent type, ignoring the SIL ownership wrapping it. --- lib/AST/ConformanceLookup.cpp | 3 +++ lib/Sema/TypeCheckType.cpp | 8 -------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/AST/ConformanceLookup.cpp b/lib/AST/ConformanceLookup.cpp index 93111ee3f74e2..409d46b9dff6c 100644 --- a/lib/AST/ConformanceLookup.cpp +++ b/lib/AST/ConformanceLookup.cpp @@ -472,6 +472,9 @@ LookupConformanceInModuleRequest::evaluate( auto *protocol = desc.PD; ASTContext &ctx = mod->getASTContext(); + // Remove SIL reference ownership wrapper, if present. + type = type->getReferenceStorageReferent(); + // A dynamic Self type conforms to whatever its underlying type // conforms to. if (auto selfType = type->getAs()) diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp index b8b0ac4aec575..85c6cf2d5e4d7 100644 --- a/lib/Sema/TypeCheckType.cpp +++ b/lib/Sema/TypeCheckType.cpp @@ -1083,14 +1083,6 @@ Type TypeResolution::applyUnboundGenericArguments( if (didDiagnoseMoveOnlyGenericArgs(ctx, loc, resultType, genericArgs, dc)) return ErrorType::get(ctx); - if (options.contains(TypeResolutionFlags::SILType)) { - if (auto nominal = dyn_cast(decl)) { - if (nominal->isOptionalDecl()) { - skipRequirementsCheck = true; - } - } - } - // Get the substitutions for outer generic parameters from the parent // type. if (parentTy) { From 3a45393e17d2b1d645a264ff0a8f05ff3120ea8c Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Mon, 12 Feb 2024 21:32:31 -0800 Subject: [PATCH 3/3] NCGenerics: break cycle with SuperclassTypeRequest With NoncopyableGenerics, we get a cycle involving `SuperclassTypeRequest` with this program: public struct RawMarkupHeader {} final class RawMarkup: ManagedBuffer { } Because we generally don't support the following kind of relationship: class Base: P {} class Derived: Base {} This commit works around the root-cause, which is that Derived's synthesized conformance to Copyable gets superceded by the inherited one from Base. Instead of recording conformances in the ConformanceLookup table at all, create builtin conformances on the fly, since classes cannot be conditionally Copyable or Escapable. --- include/swift/AST/InverseMarking.h | 5 +++ include/swift/AST/ProtocolConformance.h | 21 ++++++++++++ lib/AST/ConformanceLookup.cpp | 43 +++++++++++++++++++++++++ lib/AST/ProtocolConformance.cpp | 29 +++++++++-------- lib/Sema/TypeCheckDeclPrimary.cpp | 24 ++++++++++++++ lib/Sema/TypeCheckInvertible.cpp | 39 ++++++---------------- test/Generics/inverse_generics.swift | 5 +++ 7 files changed, 123 insertions(+), 43 deletions(-) diff --git a/include/swift/AST/InverseMarking.h b/include/swift/AST/InverseMarking.h index d356afd23ced5..398a72905f7a8 100644 --- a/include/swift/AST/InverseMarking.h +++ b/include/swift/AST/InverseMarking.h @@ -57,6 +57,11 @@ struct InverseMarking { } operator bool() const { return isPresent(); } + // Is there any kind of explicit marking? + bool isAnyExplicit() const { + return is(Kind::Explicit) || is(Kind::LegacyExplicit); + } + SourceLoc getLoc() const { return loc; } void set(Kind k, SourceLoc l = SourceLoc()) { diff --git a/include/swift/AST/ProtocolConformance.h b/include/swift/AST/ProtocolConformance.h index 11e420dc26b8d..9516a48352208 100644 --- a/include/swift/AST/ProtocolConformance.h +++ b/include/swift/AST/ProtocolConformance.h @@ -1157,6 +1157,19 @@ class BuiltinProtocolConformance final : public RootProtocolConformance { return getBuiltinConformanceKind() == BuiltinConformanceKind::Missing; } + bool isInvalid() const { + switch (getBuiltinConformanceKind()) { + case BuiltinConformanceKind::Synthesized: + return false; + case BuiltinConformanceKind::Missing: + return true; + } + } + + SourceLoc getLoc() const { + return SourceLoc(); + } + /// Get any requirements that must be satisfied for this conformance to apply. llvm::Optional> getConditionalRequirementsIfAvailable() const { @@ -1191,6 +1204,10 @@ class BuiltinProtocolConformance final : public RootProtocolConformance { llvm_unreachable("builtin-conformances never have associated types"); } + bool hasWitness(ValueDecl *requirement) const { + llvm_unreachable("builtin-conformances never have requirement witnesses"); + } + /// Retrieve the type witness and type decl (if one exists) /// for the given associated type. TypeWitnessAndDecl @@ -1199,6 +1216,10 @@ class BuiltinProtocolConformance final : public RootProtocolConformance { llvm_unreachable("builtin-conformances never have associated types"); } + Witness getWitness(ValueDecl *requirement) const { + llvm_unreachable("builtin-conformances never have requirement witnesses"); + } + /// Given that the requirement signature of the protocol directly states /// that the given dependent type must conform to the given protocol, /// return its associated conformance. diff --git a/lib/AST/ConformanceLookup.cpp b/lib/AST/ConformanceLookup.cpp index 409d46b9dff6c..380b493afdd36 100644 --- a/lib/AST/ConformanceLookup.cpp +++ b/lib/AST/ConformanceLookup.cpp @@ -29,6 +29,7 @@ #include "swift/AST/DiagnosticsSema.h" #include "swift/AST/ExistentialLayout.h" #include "swift/AST/GenericEnvironment.h" +#include "swift/AST/InverseMarking.h" #include "swift/AST/NameLookup.h" #include "swift/AST/NameLookupRequests.h" #include "swift/AST/PackConformance.h" @@ -402,6 +403,41 @@ static ProtocolConformanceRef getBuiltinMetaTypeTypeConformance( return ProtocolConformanceRef::forMissingOrInvalid(type, protocol); } +static ProtocolConformanceRef +getBuiltinInvertibleProtocolConformance(NominalTypeDecl *nominal, + Type type, + ProtocolDecl *protocol) { + assert(isa(nominal)); + ASTContext &ctx = protocol->getASTContext(); + + auto ip = protocol->getInvertibleProtocolKind(); + switch (*ip) { + case InvertibleProtocolKind::Copyable: + // If move-only classes is enabled, we'll check the markings. + if (ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses)) { + auto marking = nominal->getMarking(*ip); + switch (marking.getInverse().getKind()) { + case InverseMarking::Kind::LegacyExplicit: + case InverseMarking::Kind::Explicit: + // An inverse ~Copyable prevents conformance. + return ProtocolConformanceRef::forInvalid(); + + case InverseMarking::Kind::Inferred: // ignore "inferred" inverse marking + case InverseMarking::Kind::None: + break; + } + } + break; + case InvertibleProtocolKind::Escapable: + // Always conforms. + break; + } + + return ProtocolConformanceRef( + ctx.getBuiltinConformance(type, protocol, + BuiltinConformanceKind::Synthesized)); +} + /// Synthesize a builtin type conformance to the given protocol, if /// appropriate. static ProtocolConformanceRef @@ -586,6 +622,13 @@ LookupConformanceInModuleRequest::evaluate( if (!nominal || isa(nominal)) return ProtocolConformanceRef::forMissingOrInvalid(type, protocol); + // We specially avoid recording conformances to invertible protocols in a + // class's conformance table. This prevents an evaluator cycle. + if (ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics) + && isa(nominal) + && protocol->getInvertibleProtocolKind()) + return getBuiltinInvertibleProtocolConformance(nominal, type, protocol); + // Expand conformances added by extension macros. // // FIXME: This expansion should only be done if the diff --git a/lib/AST/ProtocolConformance.cpp b/lib/AST/ProtocolConformance.cpp index 9c2fcf6778007..7b55de69287e4 100644 --- a/lib/AST/ProtocolConformance.cpp +++ b/lib/AST/ProtocolConformance.cpp @@ -102,9 +102,10 @@ switch (getKind()) { \ return cast(this)->Method Args; \ case ProtocolConformanceKind::Self: \ return cast(this)->Method Args; \ + case ProtocolConformanceKind::Builtin: \ + return cast(this)->Method Args; \ case ProtocolConformanceKind::Specialized: \ case ProtocolConformanceKind::Inherited: \ - case ProtocolConformanceKind::Builtin: \ llvm_unreachable("not a root conformance"); \ } \ llvm_unreachable("bad ProtocolConformanceKind"); @@ -1090,19 +1091,21 @@ void NominalTypeDecl::prepareConformanceTable() const { // Synthesize the unconditional conformances to invertible protocols. // For conditional ones, see findSynthesizedConformances . if (ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics)) { - bool missingOne = false; - for (auto ip : InvertibleProtocolSet::full()) { - auto invertible = getMarking(ip); - if (!invertible.getInverse() || bool(invertible.getPositive())) - addSynthesized(ctx.getProtocol(getKnownProtocolKind(ip))); - else - missingOne = true; - } - - // FIXME: rdar://122289155 (NCGenerics: convert Equatable, Hashable, and RawRepresentable to ~Copyable.) - if (missingOne) - return; + // Classes get their conformances during ModuleDecl::lookupConformance. + if (!isa(this)) { + bool missingOne = false; + for (auto ip : InvertibleProtocolSet::full()) { + auto invertible = getMarking(ip); + if (!invertible.getInverse() || bool(invertible.getPositive())) + addSynthesized(ctx.getProtocol(getKnownProtocolKind(ip))); + else + missingOne = true; + } + // FIXME: rdar://122289155 (NCGenerics: convert Equatable, Hashable, and RawRepresentable to ~Copyable.) + if (missingOne) + return; + } } else if (!canBeCopyable()) { return; // No synthesized conformances for move-only nominals. } diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index 3905ba039b53d..e44cf8f671abe 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -3183,6 +3183,29 @@ class DeclChecker : public DeclVisitor { } } + static void diagnoseInverseOnClass(ClassDecl *decl) { + auto &ctx = decl->getASTContext(); + + for (auto ip : InvertibleProtocolSet::full()) { + auto marking = decl->getMarking(ip); + + // Inferred inverses are already ignored for classes. + // FIXME: we can also diagnose @_moveOnly here if we use `isAnyExplicit` + if (!marking.getInverse().is(InverseMarking::Kind::Explicit)) + continue; + + // Allow ~Copyable when MoveOnlyClasses is enabled + if (ip == InvertibleProtocolKind::Copyable + && ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses)) + continue; + + + ctx.Diags.diagnose(marking.getInverse().getLoc(), + diag::inverse_on_class, + getProtocolName(getKnownProtocolKind(ip))); + } + } + /// check to see if a move-only type can ever conform to the given type. /// \returns true iff a diagnostic was emitted because it was not compatible static bool diagnoseIncompatibleWithMoveOnlyType(SourceLoc loc, @@ -3415,6 +3438,7 @@ class DeclChecker : public DeclVisitor { diagnoseIncompatibleProtocolsForMoveOnlyType(CD); + diagnoseInverseOnClass(CD); } void visitProtocolDecl(ProtocolDecl *PD) { diff --git a/lib/Sema/TypeCheckInvertible.cpp b/lib/Sema/TypeCheckInvertible.cpp index 02b3d3439a3a5..468b9a9399135 100644 --- a/lib/Sema/TypeCheckInvertible.cpp +++ b/lib/Sema/TypeCheckInvertible.cpp @@ -171,20 +171,14 @@ static bool checkInvertibleConformanceCommon(ProtocolConformance *conformance, auto &ctx = nom->getASTContext(); bool conforms = true; - // An explicit `~IP` prevents conformance if any of these are true: + // An explicit `~IP` prevents conformance if it appears on the same + // declaration that also declares the conformance. // - // 1. It appears on a class. - // 2. Appears on the same declaration that also declares the conformance. - // So, if the nominal has `~Copyable` but this conformance is - // written in an extension, then we do not raise an error. + // So, if the nominal has `~Copyable` but this conformance is + // written in an extension, then we do not raise an error. auto marking = nom->getMarking(ip); - if (marking.getInverse().getKind() == InverseMarking::Kind::Explicit) { - if (isa(nom)) { - ctx.Diags.diagnose(marking.getInverse().getLoc(), - diag::inverse_on_class, - getProtocolName(kp)); - conforms &= false; - } else if (conformance->getDeclContext() == nom) { + if (marking.getInverse().isAnyExplicit()) { + if (conformance->getDeclContext() == nom) { ctx.Diags.diagnose(marking.getInverse().getLoc(), diag::inverse_but_also_conforms, nom, getProtocolName(kp)); @@ -198,7 +192,7 @@ static bool checkInvertibleConformanceCommon(ProtocolConformance *conformance, // Protocols do not directly define any storage. if (isa(nom)) - llvm_unreachable("unexpected nominal to check Copyable conformance"); + llvm_unreachable("unexpected nominal to check invertible's conformance"); // A deinit prevents a struct or enum from conforming to Copyable. if (ip == InvertibleProtocolKind::Copyable) { @@ -343,6 +337,7 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator, if (!ip) llvm_unreachable("not an invertible protocol"); + assert(!isa(nominal) && "classes aren't handled here"); auto file = cast(nominal->getModuleScopeContext()); // Generates a conformance for the nominal to the protocol. @@ -403,20 +398,6 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator, return generateConformance(ext); }; - switch (*ip) { - case InvertibleProtocolKind::Copyable: - // If move-only classes is enabled, we'll check the markings. - if (ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses)) - break; - - LLVM_FALLTHROUGH; - case InvertibleProtocolKind::Escapable: - // Always derive unconditional IP conformance for classes - if (isa(nominal)) - return generateConformance(nominal); - break; - } - auto marking = nominal->getMarking(*ip); // Unexpected to have any positive marking for IP if we're deriving it. @@ -430,10 +411,8 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator, return nullptr; // No positive IP conformance will be inferred. case InverseMarking::Kind::Inferred: - if (!isa(nominal)) - return generateConditionalConformance(); + return generateConditionalConformance(); - LLVM_FALLTHROUGH; case InverseMarking::Kind::None: // All types already start with conformances to the invertible protocols in // this case, within `NominalTypeDecl::prepareConformanceTable`. diff --git a/test/Generics/inverse_generics.swift b/test/Generics/inverse_generics.swift index 650a438ea64ba..ca7ab4a4b60f5 100644 --- a/test/Generics/inverse_generics.swift +++ b/test/Generics/inverse_generics.swift @@ -169,7 +169,12 @@ protocol NeedsCopyable {} struct Silly: ~Copyable, Copyable {} // expected-error {{struct 'Silly' required to be 'Copyable' but is marked with '~Copyable'}} enum Sally: Copyable, ~Copyable, NeedsCopyable {} // expected-error {{enum 'Sally' required to be 'Copyable' but is marked with '~Copyable'}} + class NiceTry: ~Copyable, Copyable {} // expected-error {{classes cannot be '~Copyable'}} + // expected-error@-1 {{class 'NiceTry' required to be 'Copyable' but is marked with '~Copyable}} + +@_moveOnly class NiceTry2: Copyable {} // expected-error {{'@_moveOnly' attribute is only valid on structs or enums}} + // expected-error@-1 {{class 'NiceTry2' required to be 'Copyable' but is marked with '~Copyable'}} struct OopsConformance1: ~Copyable, NeedsCopyable {} // expected-error@-1 {{type 'OopsConformance1' does not conform to protocol 'NeedsCopyable'}}