Skip to content

Strict safety more minor cleanups #80103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -8204,6 +8204,8 @@ NOTE(note_use_of_unsafe_conformance_is_unsafe,none,
(Type, const ValueDecl *))
NOTE(note_unsafe_storage,none,
"%kindbase1 involves unsafe type %2", (bool, const ValueDecl *, Type))
NOTE(note_unsafe_temporarily_escaping,none,
"'withoutActuallyEscaping' function of type %0 is unsafe", (Type))

GROUPED_WARNING(decl_unsafe_storage,StrictMemorySafety,none,
"%kindbase0 has storage involving unsafe types",
Expand Down Expand Up @@ -8240,9 +8242,9 @@ GROUPED_WARNING(unsafe_without_unsafe,StrictMemorySafety,none,
"expression uses unsafe constructs but is not marked with 'unsafe'", ())
GROUPED_WARNING(for_unsafe_without_unsafe,StrictMemorySafety,none,
"for-in loop uses unsafe constructs but is not marked with 'unsafe'", ())
WARNING(no_unsafe_in_unsafe,none,
GROUPED_WARNING(no_unsafe_in_unsafe,StrictMemorySafety,none,
"no unsafe operations occur within 'unsafe' expression", ())
WARNING(no_unsafe_in_unsafe_for,none,
GROUPED_WARNING(no_unsafe_in_unsafe_for,StrictMemorySafety,none,
"no unsafe operations occur within 'unsafe' for-in loop", ())
NOTE(make_subclass_unsafe,none,
"make class %0 @unsafe to allow unsafe overrides of safe superclass methods",
Expand Down
22 changes: 22 additions & 0 deletions include/swift/AST/UnsafeUse.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#define SWIFT_AST_UNSAFEUSE_H

#include "swift/AST/Decl.h"
#include "swift/AST/Expr.h"
#include "swift/AST/ProtocolConformance.h"
#include "swift/AST/ProtocolConformanceRef.h"
#include "swift/AST/Type.h"
Expand Down Expand Up @@ -54,6 +55,9 @@ class UnsafeUse {
CallToUnsafe,
/// A @preconcurrency import.
PreconcurrencyImport,
/// A use of withoutActuallyEscaping that lacks enforcement that the
/// value didn't actually escape.
TemporarilyEscaping,
};

private:
Expand Down Expand Up @@ -85,6 +89,8 @@ class UnsafeUse {
const void *location;
} entity;

MakeTemporarilyEscapableExpr *temporarilyEscaping;

const ImportDecl *importDecl;
} storage;

Expand Down Expand Up @@ -195,6 +201,12 @@ class UnsafeUse {
decl, type, location);
}

static UnsafeUse forTemporarilyEscaping(MakeTemporarilyEscapableExpr *expr) {
UnsafeUse result(TemporarilyEscaping);
result.storage.temporarilyEscaping = expr;
return result;
}

static UnsafeUse forPreconcurrencyImport(const ImportDecl *importDecl) {
UnsafeUse result(PreconcurrencyImport);
result.storage.importDecl = importDecl;
Expand Down Expand Up @@ -230,6 +242,9 @@ class UnsafeUse {
return SourceLoc(
llvm::SMLoc::getFromPointer((const char *)storage.entity.location));

case TemporarilyEscaping:
return storage.temporarilyEscaping->getLoc();

case PreconcurrencyImport:
return storage.importDecl->getLoc();
}
Expand All @@ -240,6 +255,7 @@ class UnsafeUse {
switch (getKind()) {
case Override:
case Witness:
case TemporarilyEscaping:
case PreconcurrencyImport:
// Cannot replace location.
return;
Expand Down Expand Up @@ -283,6 +299,7 @@ class UnsafeUse {
return storage.entity.decl;

case UnsafeConformance:
case TemporarilyEscaping:
return nullptr;

case PreconcurrencyImport:
Expand Down Expand Up @@ -315,6 +332,7 @@ class UnsafeUse {
case CallToUnsafe:
case UnsafeConformance:
case PreconcurrencyImport:
case TemporarilyEscaping:
return nullptr;
}
}
Expand All @@ -341,6 +359,9 @@ class UnsafeUse {
case ReferenceToUnsafeThroughTypealias:
case CallToUnsafe:
return storage.entity.type;

case TemporarilyEscaping:
return storage.temporarilyEscaping->getOpaqueValue()->getType();
}
}

Expand All @@ -365,6 +386,7 @@ class UnsafeUse {
case ReferenceToUnsafeStorage:
case ReferenceToUnsafeThroughTypealias:
case CallToUnsafe:
case TemporarilyEscaping:
case PreconcurrencyImport:
return ProtocolConformanceRef::forInvalid();
}
Expand Down
67 changes: 66 additions & 1 deletion lib/Sema/TypeCheckEffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,10 @@ class EffectsHandlingWalker : public ASTWalker {
break;
}
}
} else if (auto TE = dyn_cast<MakeTemporarilyEscapableExpr>(E)) {
recurse = asImpl().checkTemporarilyEscapable(TE);
}

// Error handling validation (via checkTopLevelEffects) happens after
// type checking. If an unchecked expression is still around, the code was
// invalid.
Expand Down Expand Up @@ -1090,6 +1093,15 @@ class Classification {
return result;
}

/// Return a potentially-unsafe classification by recording all of the
/// provided unsafe uses.
static Classification forUnsafe(ArrayRef<UnsafeUse> unsafeUses) {
Classification result;
for (const auto &unsafeUse : unsafeUses)
result.recordUnsafeUse(unsafeUse);
return result;
}

/// Return a classification for a given declaration reference.
static Classification
forDeclRef(ConcreteDeclRef declRef, ConditionalEffectKind conditionalKind,
Expand Down Expand Up @@ -1856,6 +1868,38 @@ class ApplyClassifier {
return result;
}

Classification classifyTemporarilyEscapable(MakeTemporarilyEscapableExpr *E) {
auto opaqueValue = E->getOpaqueValue();
if (!opaqueValue)
return Classification();

auto type = opaqueValue->getType();
if (!type)
return Classification();

auto fnType = type->getAs<AnyFunctionType>();
if (!fnType)
return Classification();

// Runtime safety checks for temporarily-escapable functions aren't
// always available.
switch (fnType->getRepresentation()) {
case FunctionTypeRepresentation::Swift:
// Swift performs runtime checking to ensure that the function did not
// escape.
return Classification();

case FunctionTypeRepresentation::Block:
return Classification::forUnsafe({UnsafeUse::forTemporarilyEscaping(E)});

case FunctionTypeRepresentation::Thin:
case FunctionTypeRepresentation::CFunctionPointer:
// Thin and C functions cannot carry any state with them, so escaping
// the pointers does not introduce a memory-safety issue.
return Classification();
}
}

private:
/// Classify a throwing or async function according to our local
/// knowledge of its implementation.
Expand Down Expand Up @@ -2073,6 +2117,10 @@ class ApplyClassifier {
return ShouldRecurse;
}

ShouldRecurse_t checkTemporarilyEscapable(MakeTemporarilyEscapableExpr *E) {
return ShouldRecurse;
}

ConditionalEffectKind checkExhaustiveDoBody(DoCatchStmt *S) {
// All errors thrown by the do body are caught, but any errors thrown
// by the catch bodies are bounded by the throwing kind of the do body.
Expand Down Expand Up @@ -2214,6 +2262,10 @@ class ApplyClassifier {
return ShouldRecurse;
}

ShouldRecurse_t checkTemporarilyEscapable(MakeTemporarilyEscapableExpr *E) {
return ShouldRecurse;
}

void visitExprPre(Expr *expr) { return; }
};

Expand Down Expand Up @@ -2327,6 +2379,11 @@ class ApplyClassifier {
return ShouldRecurse;
}

ShouldRecurse_t checkTemporarilyEscapable(MakeTemporarilyEscapableExpr *E) {
classification.merge(Self.classifyTemporarilyEscapable(E));
return ShouldRecurse;
}

void visitExprPre(Expr *expr) { return; }
};

Expand Down Expand Up @@ -3774,6 +3831,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
return ShouldRecurse;
}

ShouldRecurse_t checkTemporarilyEscapable(MakeTemporarilyEscapableExpr *E) {
auto classification = getApplyClassifier().
classifyTemporarilyEscapable(E);
checkEffectSite(E, /*requiresTry=*/false, classification);
return ShouldRecurse;
}

ConditionalEffectKind checkExhaustiveDoBody(DoCatchStmt *S) {
// This is a context where errors are handled.
ContextScope scope(*this, CurContext.withHandlesErrors());
Expand Down Expand Up @@ -4356,7 +4420,8 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
return;
}

Ctx.Diags.diagnose(E->getUnsafeLoc(), diag::no_unsafe_in_unsafe);
Ctx.Diags.diagnose(E->getUnsafeLoc(), diag::no_unsafe_in_unsafe)
.fixItRemove(E->getUnsafeLoc());
}

std::pair<SourceLoc, std::string>
Expand Down
8 changes: 8 additions & 0 deletions lib/Sema/TypeCheckUnsafe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@ void swift::diagnoseUnsafeUse(const UnsafeUse &use) {
return;
}

case UnsafeUse::TemporarilyEscaping: {
Type type = use.getType();
ASTContext &ctx = type->getASTContext();
ctx.Diags.diagnose(
use.getLocation(), diag::note_unsafe_temporarily_escaping, type);
return;
}

case UnsafeUse::PreconcurrencyImport: {
auto importDecl = cast<ImportDecl>(use.getDecl());
importDecl->diagnose(diag::preconcurrency_import_unsafe);
Expand Down
2 changes: 1 addition & 1 deletion test/Unsafe/safe.swift
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func unsafeFun() {
acceptBoolsUnsafeLabel(unsafe: unsafe, unsafe)

let color: Color
// expected-warning@+1{{no unsafe operations occur within 'unsafe' expression}}
// expected-warning@+1{{no unsafe operations occur within 'unsafe' expression}}{{11-18=}}
color = unsafe .red
_ = color

Expand Down
19 changes: 19 additions & 0 deletions test/Unsafe/unsafe_withoutactuallyescaping.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %target-typecheck-verify-swift -strict-memory-safety

// REQUIRES: objc_interop

func callMe(body: @escaping @convention(block) () -> Void) { }

func callMeSwiftly(body: @escaping () -> Void) { }

func testCall(body: @convention(block) () -> Void, swiftBody: () -> Void) {
// expected-warning@+2{{unsafe}}{{3-3=unsafe }}
// expected-note@+1{{'withoutActuallyEscaping' function of type '@convention(block) () -> Void' is unsafe}}
withoutActuallyEscaping(body) { nonescapingBody in
callMe(body: nonescapingBody)
}

withoutActuallyEscaping(swiftBody) { nonescapingBody in
callMeSwiftly(body: nonescapingBody)
}
}