Skip to content

Commit fd24d29

Browse files
DougGregorDougGregor
authored
Merge pull request #80357 from DougGregor/strict-safety-improvements
Strict safety improvements
2 parents d36b067 + a5df17e commit fd24d29

File tree

16 files changed

+139
-39
lines changed

16 files changed

+139
-39
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8233,7 +8233,7 @@ NOTE(sending_function_result_with_sending_param_note, none,
82338233
())
82348234

82358235
//------------------------------------------------------------------------------
8236-
// MARK: Strict Safety Diagnostics
8236+
// MARK: Strict Memory Safety Diagnostics
82378237
//------------------------------------------------------------------------------
82388238
NOTE(note_reference_to_unsafe_decl,none,
82398239
"%select{reference|call}0 to unsafe %kind1",
@@ -8272,6 +8272,9 @@ NOTE(decl_storage_mark_safe,none,
82728272
"add '@safe' if this type encapsulates the unsafe storage in "
82738273
"a safe interface", ())
82748274

8275+
ERROR(safe_and_unsafe_attr,none,
8276+
"%kindbase0 cannot be both @safe and @unsafe", (const Decl *))
8277+
82758278
GROUPED_WARNING(unsafe_superclass,StrictMemorySafety,none,
82768279
"%kindbase0 has superclass involving unsafe type %1",
82778280
(const ValueDecl *, Type))

‎lib/AST/Decl.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,10 +1229,6 @@ ExplicitSafety Decl::getExplicitSafety() const {
12291229
if (auto extSafety = getExplicitSafetyFromAttrs(ext))
12301230
return *extSafety;
12311231
}
1232-
1233-
if (auto enclosingNominal = enclosingDC->getSelfNominalTypeDecl())
1234-
if (auto nominalSafety = getExplicitSafetyFromAttrs(enclosingNominal))
1235-
return *nominalSafety;
12361232
}
12371233

12381234
// If an extension extends an unsafe nominal type, it's unsafe.
@@ -1242,11 +1238,12 @@ ExplicitSafety Decl::getExplicitSafety() const {
12421238
return ExplicitSafety::Unsafe;
12431239
}
12441240

1245-
// If this is a pattern binding declaration, check whether the first
1246-
// variable is @unsafe.
1241+
// If this is a pattern binding declaration, check whether there is a
1242+
// safety-related attribute on the first variable we find.
12471243
if (auto patternBinding = dyn_cast<PatternBindingDecl>(this)) {
1248-
if (auto var = patternBinding->getSingleVar())
1249-
return var->getExplicitSafety();
1244+
for (auto index : range(patternBinding->getNumPatternEntries()))
1245+
if (auto var = patternBinding->getAnchoringVarDecl(index))
1246+
return var->getExplicitSafety();
12501247
}
12511248

12521249
return ExplicitSafety::Unspecified;

‎lib/Sema/PreCheckTarget.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1680,6 +1680,11 @@ void PreCheckTarget::markAnyValidSingleValueStmts(Expr *E) {
16801680
while (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(E))
16811681
E = IIO->getSubExpr();
16821682
}
1683+
1684+
// Look through "unsafe" expressions.
1685+
if (auto UE = dyn_cast<UnsafeExpr>(E))
1686+
E = UE->getSubExpr();
1687+
16831688
return dyn_cast<AssignExpr>(E);
16841689
};
16851690

‎lib/Sema/TypeCheckAttr.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,6 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
192192
IGNORED_ATTR(AllowFeatureSuppression)
193193
IGNORED_ATTR(PreInverseGenerics)
194194
IGNORED_ATTR(Safe)
195-
IGNORED_ATTR(Unsafe)
196195
#undef IGNORED_ATTR
197196

198197
void visitABIAttr(ABIAttr *attr) {
@@ -445,6 +444,7 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
445444
void visitLifetimeAttr(LifetimeAttr *attr);
446445
void visitAddressableSelfAttr(AddressableSelfAttr *attr);
447446
void visitAddressableForDependenciesAttr(AddressableForDependenciesAttr *attr);
447+
void visitUnsafeAttr(UnsafeAttr *attr);
448448
};
449449

450450
} // end anonymous namespace
@@ -8140,6 +8140,15 @@ AttributeChecker::visitAddressableForDependenciesAttr(
81408140
}
81418141
}
81428142

8143+
void AttributeChecker::visitUnsafeAttr(UnsafeAttr *attr) {
8144+
if (auto safeAttr = D->getAttrs().getAttribute<SafeAttr>()) {
8145+
D->diagnose(diag::safe_and_unsafe_attr, D)
8146+
.highlight(attr->getRange())
8147+
.highlight(safeAttr->getRange())
8148+
.warnInSwiftInterface(D->getDeclContext());
8149+
}
8150+
}
8151+
81438152
namespace {
81448153

81458154
class ClosureAttributeChecker

‎lib/Sema/TypeCheckEffects.cpp

Lines changed: 74 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,10 @@ class EffectsHandlingWalker : public ASTWalker {
668668
recurse = asImpl().checkThrow(thr);
669669
} else if (auto forEach = dyn_cast<ForEachStmt>(S)) {
670670
recurse = asImpl().checkForEach(forEach);
671+
} else if (auto labeled = dyn_cast<LabeledConditionalStmt>(S)) {
672+
asImpl().noteLabeledConditionalStmt(labeled);
671673
}
674+
672675
if (!recurse)
673676
return Action::SkipNode(S);
674677

@@ -690,6 +693,8 @@ class EffectsHandlingWalker : public ASTWalker {
690693
}
691694

692695
void visitExprPre(Expr *expr) { asImpl().visitExprPre(expr); }
696+
697+
void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) { }
693698
};
694699

695700
/// A potential reason why something might have an effect.
@@ -3420,6 +3425,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
34203425
/// passed directly into an explicitly `@safe` function.
34213426
llvm::DenseSet<const Expr *> assumedSafeArguments;
34223427

3428+
/// Keeps track of the expressions that were synthesized as initializers for
3429+
/// the "if let x" shorthand syntax.
3430+
llvm::SmallPtrSet<const Expr *, 4> synthesizedIfLetInitializers;
3431+
34233432
/// Tracks all of the uncovered uses of unsafe constructs based on their
34243433
/// anchor expression, so we can emit diagnostics at the end.
34253434
llvm::MapVector<Expr *, std::vector<UnsafeUse>> uncoveredUnsafeUses;
@@ -4429,7 +4438,67 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
44294438
Ctx.Diags.diagnose(E->getUnsafeLoc(), diag::no_unsafe_in_unsafe)
44304439
.fixItRemove(E->getUnsafeLoc());
44314440
}
4432-
4441+
4442+
void noteLabeledConditionalStmt(LabeledConditionalStmt *stmt) {
4443+
// Make a note of any initializers that are the synthesized right-hand side
4444+
// for an "if let x".
4445+
for (const auto &condition: stmt->getCond()) {
4446+
switch (condition.getKind()) {
4447+
case StmtConditionElement::CK_Availability:
4448+
case StmtConditionElement::CK_Boolean:
4449+
case StmtConditionElement::CK_HasSymbol:
4450+
continue;
4451+
4452+
case StmtConditionElement::CK_PatternBinding:
4453+
break;
4454+
}
4455+
4456+
auto init = condition.getInitializer();
4457+
if (!init)
4458+
continue;
4459+
4460+
auto pattern = condition.getPattern();
4461+
if (!pattern)
4462+
continue;
4463+
4464+
auto optPattern = dyn_cast<OptionalSomePattern>(pattern);
4465+
if (!optPattern)
4466+
continue;
4467+
4468+
auto var = optPattern->getSubPattern()->getSingleVar();
4469+
if (!var)
4470+
continue;
4471+
4472+
// If the right-hand side has the same location as the variable, it was
4473+
// synthesized.
4474+
if (var->getLoc().isValid() &&
4475+
var->getLoc() == init->getStartLoc() &&
4476+
init->getStartLoc() == init->getEndLoc())
4477+
synthesizedIfLetInitializers.insert(init);
4478+
}
4479+
}
4480+
4481+
/// Determine whether this is the synthesized right-hand-side when we have
4482+
/// expanded an "if let x" into its semantic equivalent, "if let x = x".
4483+
VarDecl *isShorthandIfLetSyntax(const Expr *expr) const {
4484+
// Check whether this is referencing a variable.
4485+
VarDecl *var = nullptr;
4486+
if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
4487+
var = dyn_cast_or_null<VarDecl>(declRef->getDecl());
4488+
} else if (auto memberRef = dyn_cast<MemberRefExpr>(expr)) {
4489+
var = dyn_cast_or_null<VarDecl>(memberRef->getMember().getDecl());
4490+
}
4491+
4492+
if (!var)
4493+
return nullptr;
4494+
4495+
// If we identified this as one of the bindings, return the variable.
4496+
if (synthesizedIfLetInitializers.contains(expr))
4497+
return var;
4498+
4499+
return nullptr;
4500+
}
4501+
44334502
std::pair<SourceLoc, std::string>
44344503
getFixItForUncoveredSite(const Expr *anchor, StringRef keyword) const {
44354504
SourceLoc insertLoc = anchor->getStartLoc();
@@ -4442,13 +4511,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
44424511
insertLoc = tryExpr->getSubExpr()->getStartLoc();
44434512
// Supply a tailored fixIt including the identifier if we are
44444513
// looking at a shorthand optional binding.
4445-
} else if (anchor->isImplicit()) {
4446-
if (auto declRef = dyn_cast<DeclRefExpr>(anchor))
4447-
if (auto var = dyn_cast_or_null<VarDecl>(declRef->getDecl())) {
4448-
insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
4449-
insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
4450-
anchor->getStartLoc());
4451-
}
4514+
} else if (auto var = isShorthandIfLetSyntax(anchor)) {
4515+
insertText = (" = " + keyword).str() + " " + var->getNameStr().str();
4516+
insertLoc = Lexer::getLocForEndOfToken(Ctx.Diags.SourceMgr,
4517+
anchor->getStartLoc());
44524518
}
44534519
return std::make_pair(insertLoc, insertText);
44544520
}

‎stdlib/public/core/CTypes.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,7 @@ public typealias CBool = Bool
157157
@frozen
158158
@unsafe
159159
public struct OpaquePointer {
160-
@unsafe @usableFromInline
161-
@safe
160+
@usableFromInline
162161
internal var _rawValue: Builtin.RawPointer
163162

164163
@usableFromInline @_transparent

‎test/Macros/SwiftifyImport/CountedBy/SimpleSpan.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
// REQUIRES: swift_swift_parser
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
4-
5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
6-
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
4+
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
75

86
@_SwiftifyImport(.countedBy(pointer: .param(1), count: "len"), .nonescaping(pointer: .param(1)))
97
func myFunc(_ ptr: UnsafePointer<CInt>, _ len: CInt) {

‎test/Macros/SwiftifyImport/CountedBy/SimpleSpanWithReturn.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
// REQUIRES: swift_swift_parser
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
4-
5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
6-
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
4+
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
75

86
@_SwiftifyImport(.countedBy(pointer: .param(1), count: "len"), .nonescaping(pointer: .param(1)))
97
func myFunc(_ ptr: UnsafePointer<CInt>, _ len: CInt) -> CInt {

‎test/Macros/SwiftifyImport/CountedBy/SpanAndUnsafeBuffer.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
44

5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
6-
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
5+
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
76

87
@_SwiftifyImport(.countedBy(pointer: .param(1), count: "len1"), .countedBy(pointer: .param(3), count: "len2"), .nonescaping(pointer: .param(1)))
98
func myFunc(_ ptr1: UnsafePointer<CInt>, _ len1: CInt, _ ptr2: UnsafePointer<CInt>, _ len2: CInt) {

‎test/Macros/SwiftifyImport/SizedBy/Opaque.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions -verify 2>&1 | %FileCheck --match-full-lines %s
44

5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
5+
// FIXME: Waiting for optional to handle nonescapable types
66
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
77

88
@_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"))

‎test/Macros/SwiftifyImport/SizedBy/SimpleRawSpan.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
// REQUIRES: swift_swift_parser
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
4-
5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
6-
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
4+
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
75

86
@_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"), .nonescaping(pointer: .param(1)))
97
func myFunc(_ ptr: UnsafeRawPointer, _ size: CInt) {

‎test/Macros/SwiftifyImport/SizedBy/SimpleRawSpanWithReturn.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
// REQUIRES: swift_swift_parser
22

33
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -dump-macro-expansions 2>&1 | %FileCheck --match-full-lines %s
4-
5-
// rdar://145899513 ([StrictMemorySafety] Call to RawSpan::withUnsafeBytes not recognized as unsafe, while call to Span::withUnsafeBufferPointer is)
6-
// RUN: not %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
4+
// RUN: %target-swift-frontend %s -swift-version 5 -module-name main -disable-availability-checking -typecheck -plugin-path %swift-plugin-dir -strict-memory-safety -warnings-as-errors
75

86
@_SwiftifyImport(.sizedBy(pointer: .param(1), size: "size"), .nonescaping(pointer: .param(1)))
97
func myFunc(_ ptr: UnsafeRawPointer, _ size: CInt) -> CInt {

‎test/Unsafe/safe.swift

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,3 +241,33 @@ func testMyArray(ints: MyArray<Int>) {
241241
unsafe print(buffer.unsafeCount)
242242
}
243243
}
244+
245+
func testUnsafeLHS() {
246+
@unsafe var value: Int = 0
247+
unsafe value = switch unsafe value {
248+
case 0: 1
249+
default: 0
250+
}
251+
}
252+
253+
@safe
254+
struct UnsafeWrapTest {
255+
var pointer: UnsafeMutablePointer<Int>?
256+
257+
func test() {
258+
if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
259+
// expected-note@-1{{reference to property 'pointer' involves unsafe type 'UnsafeMutablePointer<Int>'}}
260+
_ = unsafe pointer
261+
}
262+
}
263+
264+
func otherTest(pointer: UnsafeMutablePointer<Int>?) {
265+
if let pointer { // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{19-19= = unsafe pointer}}
266+
// expected-note@-1{{reference to parameter 'pointer' involves unsafe type 'UnsafeMutablePointer<Int>}}
267+
_ = unsafe pointer
268+
}
269+
}
270+
}
271+
272+
@safe @unsafe
273+
struct ConfusedStruct { } // expected-error{{struct 'ConfusedStruct' cannot be both @safe and @unsafe}}

‎test/Unsafe/safe_argument_suppression.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ class NotSafeSubclass: NotSafe {
3838

3939
ns.stillUnsafe() // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe' [StrictMemorySafety]}}
4040
// expected-note@-1{{reference to parameter 'ns' involves unsafe type 'NotSafe'}}
41-
// expected-note@-2{{reference to unsafe instance method 'stillUnsafe()'}}
41+
// expected-note@-2{{reference to instance method 'stillUnsafe()' involves unsafe type 'NotSafe'}}
4242
}
4343

4444
@safe func testImpliedSafetySubclass(ns: NotSafeSubclass) {

‎test/Unsafe/unsafe.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ extension ConformsToMultiP: MultiP {
4949
// expected-note@-1{{unsafe type 'UnsafeSuper' cannot satisfy safe associated type 'Ptr'}}
5050
@unsafe func f() -> UnsafeSuper {
5151
.init() // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe'}}
52-
// expected-note@-1{{reference to unsafe initializer 'init()'}}
52+
// expected-note@-1{{reference to initializer 'init()' involves unsafe type 'UnsafeSuper'}}
5353
}
5454
}
5555

@@ -156,7 +156,7 @@ func testMe(
156156
// expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{7-7=unsafe }}
157157
_ = unsafeVar // expected-note{{reference to unsafe var 'unsafeVar'}}
158158
// expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{3-3=unsafe }}
159-
unsafeSuper.f() // expected-note{{reference to unsafe instance method 'f()'}}
159+
unsafeSuper.f() // expected-note{{reference to instance method 'f()' involves unsafe type 'UnsafeSuper'}}
160160
// expected-note@-1{{reference to parameter 'unsafeSuper' involves unsafe type 'UnsafeSuper'}}
161161

162162
// expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}{{7-7=unsafe }}

‎test/expr/unary/async_await.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ func testAsyncExprWithoutAwait() async {
232232
if let result = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19=await }}
233233
// expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
234234
// expected-note@-2 {{reference to async let 'result' is 'async'}}
235-
if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{10-10=await }}
235+
if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{16-16= = await result}}
236236
// expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
237237
// expected-note@-2 {{reference to async let 'result' is 'async'}}
238238
let a = f("a") // expected-error {{expression is 'async' but is not marked with 'await'}} {{11-11=await }}

0 commit comments

Comments
 (0)