@safe functions, properties, and subscripts "cover" certain unsafe arguments

When calling an explicitly-@safe function or subscript, or accessing an
explicitly-@safe property, the direct arguments to that operation can be
considered safe if they are references to local variables or are references
to types.

This brings the implementation in line with the recent adjustments to the
proposal within the review.

Author: DougGregor

lib/Sema/TypeCheckEffects.cpp

@@ -1018,6 +1018,16 @@ class Classification {
 
     return result;
   }
+
+  /// Return a classification that is the same as this one, but drops the
+  /// 'unsafe' effect.
+  Classification withoutUnsafe() const {
+    Classification result(*this);
+    result.UnsafeKind = ConditionalEffectKind::None;
+    result.UnsafeUses.clear();
+    return result;
+  }
+
   /// Return a classification that promotes a typed throws effect to an
   /// untyped throws effect.
   Classification promoteToUntypedThrows() const {
@@ -3149,6 +3159,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
   llvm::DenseMap<Expr *, std::vector<DiagnosticInfo>> uncoveredAsync;
   llvm::DenseMap<Expr *, Expr *> parentMap;
 
+  /// Expressions that are assumed to be safe because they are being
+  /// passed directly into an explicitly `@safe` function.
+  llvm::DenseSet<const Expr *> assumedSafeArguments;
+
   /// Tracks all of the uncovered uses of unsafe constructs based on their
   /// anchor expression, so we can emit diagnostics at the end.
   llvm::MapVector<Expr *, std::vector<UnsafeUse>> uncoveredUnsafeUses;
@@ -3526,6 +3540,12 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
   ShouldRecurse_t checkWithSubstitutionMap(Expr *E, SubstitutionMap subs) {
     auto classification = Classification::forSubstitutionMap(
         subs, E->getLoc());
+
+    // If this expression is covered as a safe argument, drop the unsafe
+    // classification.
+    if (assumedSafeArguments.contains(E))
+      classification = classification.withoutUnsafe();
+
     checkEffectSite(E, /*requiresTry=*/false, classification);
     return ShouldRecurse;
   }
@@ -3535,13 +3555,25 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
       ArrayRef<ProtocolConformanceRef> conformances) {
     auto classification = Classification::forConformances(
         conformances, E->getLoc());
+
+    // If this expression is covered as a safe argument, drop the unsafe
+    // classification.
+    if (assumedSafeArguments.contains(E))
+      classification = classification.withoutUnsafe();
+
     checkEffectSite(E, /*requiresTry=*/false, classification);
     return ShouldRecurse;
   }
 
   ShouldRecurse_t checkType(Expr *E, TypeRepr *typeRepr, Type type) {
     SourceLoc loc = typeRepr ? typeRepr->getLoc() : E->getLoc();
     auto classification = Classification::forType(type, loc);
+
+    // If this expression is covered as a safe argument, drop the unsafe
+    // classification.
+    if (assumedSafeArguments.contains(E))
+      classification = classification.withoutUnsafe();
+
     checkEffectSite(E, /*requiresTry=*/false, classification);
     return ShouldRecurse;
   }
@@ -3646,6 +3678,36 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     CurContext = savedContext;
   }
 
+  /// Mark an argument as safe if it is of a form where a @safe declaration
+  /// covers it.
+  void markArgumentSafe(Expr *arg) {
+    auto argValue = arg->findOriginalValue();
+
+    // Reference to a local variable or parameter.
+    if (auto argDRE = dyn_cast<DeclRefExpr>(argValue)) {
+      if (auto argDecl = argDRE->getDecl())
+        if (argDecl->getDeclContext()->isLocalContext())
+          assumedSafeArguments.insert(argDRE);
+    }
+
+    // Metatype reference.
+    if (isa<TypeExpr>(argValue))
+      assumedSafeArguments.insert(argValue);
+  }
+
+  /// Mark any of the local variable references within the given argument
+  /// list as safe, so we don't diagnose unsafe uses of them.
+  void markArgumentsSafe(ArgumentList *argumentList) {
+    if (!argumentList)
+      return;
+
+    for (const auto &arg: *argumentList) {
+      if (auto argExpr = arg.getExpr()) {
+        markArgumentSafe(argExpr);
+      }
+    }
+  }
+
   ShouldRecurse_t checkApply(ApplyExpr *E) {
     // An apply expression is a potential throw site if the function throws.
     // But if the expression didn't type-check, suppress diagnostics.
@@ -3673,6 +3735,14 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
       E->setNoAsync(true);
     }
 
+    // If this is calling a @safe function, treat local variables as being
+    // safe.
+    if (auto calleeDecl = E->getCalledValue()) {
+      if (calleeDecl->getExplicitSafety() == ExplicitSafety::Safe) {
+        markArgumentsSafe(E->getArgs());
+      }
+    }
+
     // If current apply expression did not type-check, don't attempt
     // walking inside of it. This accounts for the fact that we don't
     // erase types without type variables to enable better code complication,
@@ -3698,6 +3768,22 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
       E->setThrows(throwDest);
     }
 
+    // If the declaration we're referencing is explicitly @safe, mark arguments
+    // as potentially being safe.
+    if (auto decl = E->getMember().getDecl()) {
+      if (decl->getExplicitSafety() == ExplicitSafety::Safe) {
+        // The base can be considered safe.
+        markArgumentSafe(E->getBase());
+
+        // Mark subscript arguments safe.
+        if (auto subscript = dyn_cast<SubscriptExpr>(E)) {
+          markArgumentsSafe(subscript->getArgs());
+        } else if (auto dynSubscript = dyn_cast<DynamicSubscriptExpr>(E)) {
+          markArgumentsSafe(dynSubscript->getArgs());
+        }
+      }
+    }
+
     return ShouldRecurse;
   }
 
@@ -3712,6 +3798,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
       if (!isEvaluated)
         classification = classification.onlyUnsafe();
 
+      // If we're treating this expression as a safe argument, drop 'unsafe'.
+      if (assumedSafeArguments.contains(expr))
+        classification = classification.withoutUnsafe();
+
       auto throwDest = checkEffectSite(
           expr, classification.hasThrows(), classification);
 

test/Unsafe/safe.swift

@@ -171,8 +171,7 @@ func testMyArray(ints: MyArray<Int>) {
     let bufferCopy = unsafe buffer
     _ = unsafe bufferCopy
 
-    // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}
-    print(buffer.safeCount) // expected-note{{reference to parameter 'buffer' involves unsafe type 'UnsafeBufferPointer<Int>'}}
+    print(buffer.safeCount)
     unsafe print(buffer.baseAddress!)
   }
 }

test/Unsafe/safe_argument_suppression.swift

@@ -0,0 +1,53 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-feature AllowUnsafeAttribute -enable-experimental-feature WarnUnsafe -print-diagnostic-groups
+
+// REQUIRES: swift_feature_AllowUnsafeAttribute
+// REQUIRES: swift_feature_WarnUnsafe
+
+@unsafe
+class NotSafe {
+  @safe var okay: Int { 0 }
+
+  @safe var safeSelf: NotSafe { unsafe self }
+
+  @safe func memberFunc(_: NotSafe) { }
+
+  @safe subscript(ns: NotSafe) -> Int { 5 }
+
+  @safe static func doStatically(_: NotSafe.Type) { }
+  
+  @safe static subscript(ns: NotSafe) -> Int { 5 }
+
+  @safe init(_: NotSafe) { }
+
+  func stillUnsafe() { }
+}
+
+@unsafe
+class NotSafeSubclass: NotSafe {
+}
+
+@safe func okayFunc(_ ns: NotSafe) { }
+
+@safe func testImpliedSafety(ns: NotSafe) {
+  _ = ns.okay
+  _ = ns.safeSelf.okay
+  ns.memberFunc(ns)
+  okayFunc(ns)
+  _ = ns[ns]
+
+  _ = NotSafe(ns)
+  _ = NotSafe[ns]
+  NotSafe.doStatically(NotSafe.self)
+
+  ns.stillUnsafe() // expected-warning{{expression uses unsafe constructs but is not marked with 'unsafe' [Unsafe]}}
+  // expected-note@-1{{reference to parameter 'ns' involves unsafe type 'NotSafe'}}
+  // expected-note@-2{{reference to unsafe instance method 'stillUnsafe()'}}
+}
+
+@safe func testImpliedSafetySubclass(ns: NotSafeSubclass) {
+  _ = ns.okay
+  _ = ns.safeSelf.okay
+  ns.memberFunc(ns)
+  okayFunc(ns)
+  _ = ns[ns]
+}