From 1142fa3bdd0ca1233dafba9c689754ade1aab7a2 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 10 Jul 2025 08:48:37 -0700 Subject: [PATCH] [Strict memory safety] Eliminate spurious warnings with synthesized Codable When synthesizing code for Codable conformances involving unsafe types, make sure to wrap the resulting expressions in "unsafe" when strict memory safety is enabled. Tweak the warning-emission logic to suppress warnings about spurious "unsafe" expressions when the compiler generated the "unsafe" itself, so we don't spam the developer with warnings they can't fix. Also make the checking for other suppression considerations safe when there are no source locations, eliminating a potential assertion. Fixes rdar://153665692. --- lib/Sema/CSGen.cpp | 3 +- .../DerivedConformanceCodable.cpp | 48 ++++++++++++------- lib/Sema/TypeCheckEffects.cpp | 23 +++++---- lib/Sema/TypeCheckPattern.cpp | 2 +- test/Unsafe/codable_synthesis.swift | 26 ++++++++++ test/Unsafe/hashable_synthesis.swift | 18 +++++++ 6 files changed, 92 insertions(+), 28 deletions(-) create mode 100644 test/Unsafe/codable_synthesis.swift create mode 100644 test/Unsafe/hashable_synthesis.swift diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp index 411d9dfef68ad..5a20b9cd72188 100644 --- a/lib/Sema/CSGen.cpp +++ b/lib/Sema/CSGen.cpp @@ -4734,9 +4734,10 @@ generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc, (isAsync && ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete)) { SourceLoc loc = stmt->getUnsafeLoc(); + bool implicit = stmt->getUnsafeLoc().isInvalid(); if (loc.isInvalid()) loc = stmt->getForLoc(); - nextCall = new (ctx) UnsafeExpr(loc, nextCall, Type(), /*implicit=*/true); + nextCall = new (ctx) UnsafeExpr(loc, nextCall, Type(), implicit); } // The iterator type must conform to IteratorProtocol. diff --git a/lib/Sema/DerivedConformance/DerivedConformanceCodable.cpp b/lib/Sema/DerivedConformance/DerivedConformanceCodable.cpp index 8d9b5bd022931..1b88527d1dc61 100644 --- a/lib/Sema/DerivedConformance/DerivedConformanceCodable.cpp +++ b/lib/Sema/DerivedConformance/DerivedConformanceCodable.cpp @@ -772,10 +772,20 @@ lookupVarDeclForCodingKeysCase(DeclContext *conformanceDC, llvm_unreachable("Should have found at least 1 var decl"); } -static TryExpr *createEncodeCall(ASTContext &C, Type codingKeysType, - EnumElementDecl *codingKey, - Expr *containerExpr, Expr *varExpr, - bool useIfPresentVariant) { +/// If strict memory safety checking is enabled, wrap the expression in an +/// implicit "unsafe". +static Expr *wrapInUnsafeIfNeeded(ASTContext &ctx, Expr *expr) { + if (ctx.LangOpts.hasFeature(Feature::StrictMemorySafety, + /*allowMigration=*/true)) + return UnsafeExpr::createImplicit(ctx, expr->getStartLoc(), expr); + + return expr; +} + +static Expr *createEncodeCall(ASTContext &C, Type codingKeysType, + EnumElementDecl *codingKey, + Expr *containerExpr, Expr *varExpr, + bool useIfPresentVariant) { // CodingKeys.x auto *metaTyRef = TypeExpr::createImplicit(codingKeysType, C); auto *keyExpr = new (C) MemberRefExpr(metaTyRef, SourceLoc(), codingKey, @@ -794,7 +804,7 @@ static TryExpr *createEncodeCall(ASTContext &C, Type codingKeysType, // try container.encode(x, forKey: CodingKeys.x) auto *tryExpr = new (C) TryExpr(SourceLoc(), callExpr, Type(), /*Implicit=*/true); - return tryExpr; + return wrapInUnsafeIfNeeded(C, tryExpr); } /// Synthesizes the body for `func encode(to encoder: Encoder) throws`. @@ -929,7 +939,7 @@ deriveBodyEncodable_encode(AbstractFunctionDecl *encodeDecl, void *) { // try super.encode(to: container.superEncoder()) auto *tryExpr = new (C) TryExpr(SourceLoc(), callExpr, Type(), /*Implicit=*/true); - statements.push_back(tryExpr); + statements.push_back(wrapInUnsafeIfNeeded(C, tryExpr)); } auto *body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc(), @@ -1112,8 +1122,10 @@ deriveBodyEncodable_enum_encode(AbstractFunctionDecl *encodeDecl, void *) { // generate: switch self { } auto enumRef = - new (C) DeclRefExpr(ConcreteDeclRef(selfRef), DeclNameLoc(), - /*implicit*/ true, AccessSemantics::Ordinary); + wrapInUnsafeIfNeeded( + C, + new (C) DeclRefExpr(ConcreteDeclRef(selfRef), DeclNameLoc(), + /*implicit*/ true, AccessSemantics::Ordinary)); auto switchStmt = createEnumSwitch( C, funcDC, enumRef, enumDecl, codingKeysEnum, /*createSubpattern*/ true, @@ -1276,11 +1288,11 @@ static FuncDecl *deriveEncodable_encode(DerivedConformance &derived) { return encodeDecl; } -static TryExpr *createDecodeCall(ASTContext &C, Type resultType, - Type codingKeysType, - EnumElementDecl *codingKey, - Expr *containerExpr, - bool useIfPresentVariant) { +static Expr *createDecodeCall(ASTContext &C, Type resultType, + Type codingKeysType, + EnumElementDecl *codingKey, + Expr *containerExpr, + bool useIfPresentVariant) { auto methodName = useIfPresentVariant ? C.Id_decodeIfPresent : C.Id_decode; // Type.self @@ -1470,7 +1482,7 @@ deriveBodyDecodable_init(AbstractFunctionDecl *initDecl, void *) { varDecl->getName()); auto *assignExpr = new (C) AssignExpr(varExpr, SourceLoc(), tryExpr, /*Implicit=*/true); - statements.push_back(assignExpr); + statements.push_back(wrapInUnsafeIfNeeded(C, assignExpr)); } } @@ -1506,7 +1518,7 @@ deriveBodyDecodable_init(AbstractFunctionDecl *initDecl, void *) { // try super.init(from: container.superDecoder()) auto *tryExpr = new (C) TryExpr(SourceLoc(), callExpr, Type(), /*Implicit=*/true); - statements.push_back(tryExpr); + statements.push_back(wrapInUnsafeIfNeeded(C, tryExpr)); } else { // The explicit constructor name is a compound name taking no arguments. DeclName initName(C, DeclBaseName::createConstructor(), @@ -1538,7 +1550,7 @@ deriveBodyDecodable_init(AbstractFunctionDecl *initDecl, void *) { callExpr = new (C) TryExpr(SourceLoc(), callExpr, Type(), /*Implicit=*/true); - statements.push_back(callExpr); + statements.push_back(wrapInUnsafeIfNeeded(C, callExpr)); } } } @@ -1827,7 +1839,7 @@ deriveBodyDecodable_enum_init(AbstractFunctionDecl *initDecl, void *) { new (C) AssignExpr(selfRef, SourceLoc(), selfCaseExpr, /*Implicit=*/true); - caseStatements.push_back(assignExpr); + caseStatements.push_back(wrapInUnsafeIfNeeded(C, assignExpr)); } else { // Foo.bar(x:) SmallVector scratch; @@ -1845,7 +1857,7 @@ deriveBodyDecodable_enum_init(AbstractFunctionDecl *initDecl, void *) { new (C) AssignExpr(selfRef, SourceLoc(), caseCallExpr, /*Implicit=*/true); - caseStatements.push_back(assignExpr); + caseStatements.push_back(wrapInUnsafeIfNeeded(C, assignExpr)); } auto body = diff --git a/lib/Sema/TypeCheckEffects.cpp b/lib/Sema/TypeCheckEffects.cpp index f0502c3034c82..3af9f4a366411 100644 --- a/lib/Sema/TypeCheckEffects.cpp +++ b/lib/Sema/TypeCheckEffects.cpp @@ -4667,18 +4667,25 @@ class CheckEffectsCoverage : public EffectsHandlingWalker } void diagnoseRedundantUnsafe(UnsafeExpr *E) const { - // Silence this warning in the expansion of the _SwiftifyImport macro. - // This is a hack because it's tricky to determine when to insert "unsafe". - unsigned bufferID = - Ctx.SourceMgr.findBufferContainingLoc(E->getUnsafeLoc()); - if (auto sourceInfo = Ctx.SourceMgr.getGeneratedSourceInfo(bufferID)) { - if (sourceInfo->macroName == "_SwiftifyImport") - return; + // Ignore implicitly-generated "unsafe" expressions; they're allowed to be + // overly conservative. + if (E->isImplicit()) + return; + + SourceLoc loc = E->getUnsafeLoc(); + if (loc.isValid()) { + // Silence this warning in the expansion of the _SwiftifyImport macro. + // This is a hack because it's tricky to determine when to insert "unsafe". + unsigned bufferID = Ctx.SourceMgr.findBufferContainingLoc(loc); + if (auto sourceInfo = Ctx.SourceMgr.getGeneratedSourceInfo(bufferID)) { + if (sourceInfo->macroName == "_SwiftifyImport") + return; + } } if (auto *SVE = SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(E)) { // For an if/switch expression, produce a tailored warning. - Ctx.Diags.diagnose(E->getUnsafeLoc(), + Ctx.Diags.diagnose(loc, diag::effect_marker_on_single_value_stmt, "unsafe", SVE->getStmt()->getKind()) .highlight(E->getUnsafeLoc()); diff --git a/lib/Sema/TypeCheckPattern.cpp b/lib/Sema/TypeCheckPattern.cpp index 68fed74e6087a..7b32dc99b585f 100644 --- a/lib/Sema/TypeCheckPattern.cpp +++ b/lib/Sema/TypeCheckPattern.cpp @@ -791,7 +791,7 @@ ExprPatternMatchRequest::evaluate(Evaluator &evaluator, // If there was an "unsafe", put it outside of the match call. if (unsafeExpr) { - matchCall = UnsafeExpr::createImplicit(ctx, unsafeExpr->getLoc(), matchCall); + matchCall = new (ctx) UnsafeExpr(unsafeExpr->getLoc(), matchCall); } return {matchVar, matchCall}; diff --git a/test/Unsafe/codable_synthesis.swift b/test/Unsafe/codable_synthesis.swift new file mode 100644 index 0000000000000..282db90cd2c09 --- /dev/null +++ b/test/Unsafe/codable_synthesis.swift @@ -0,0 +1,26 @@ +// RUN: %target-typecheck-verify-swift -strict-memory-safety + +@unsafe public struct UnsafeStruct: Codable { + public var string: String +} + + +@unsafe public enum UnsafeEnum: Codable { +case something(Int) +} + +@safe public struct SafeStruct: Codable { + public var us: UnsafeStruct +} + +@safe public enum SafeEnum: Codable { +case something(UnsafeEnum) +} + +@unsafe public class C1: Codable { + public var string: String = "" +} + +@unsafe public class C2: C1 { + public var otherString: String = "" +} diff --git a/test/Unsafe/hashable_synthesis.swift b/test/Unsafe/hashable_synthesis.swift new file mode 100644 index 0000000000000..188f433462aad --- /dev/null +++ b/test/Unsafe/hashable_synthesis.swift @@ -0,0 +1,18 @@ +// RUN: %target-typecheck-verify-swift -strict-memory-safety + +@unsafe public struct UnsafeStruct: Hashable { + public var string: String +} + + +@unsafe public enum UnsafeEnum: Hashable { +case something(Int) +} + +@safe public struct SafeStruct: Hashable { + public var us: UnsafeStruct +} + +@safe public enum SafeEnum: Hashable { +case something(UnsafeEnum) +}