Merge pull request #65912 from kavon/5.9-noncopyable-fixes

[5.9🍒] Batch of small noncopyable type fixes

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -7138,6 +7138,8 @@ ERROR(moveonly_cannot_conform_to_type, none,
      (DescriptiveDeclKind, DeclName, Type))
 ERROR(moveonly_parameter_missing_ownership, none,
       "noncopyable parameter must specify its ownership", ())
+ERROR(moveonly_parameter_subscript_unsupported, none,
+      "subscripts cannot have noncopyable parameters yet", ())
 NOTE(moveonly_parameter_ownership_suggestion, none,
       "add '%0' %1", (StringRef, StringRef))
 ERROR(ownership_specifier_copyable,none,

lib/AST/Type.cpp

@@ -189,7 +189,7 @@ bool TypeBase::isMarkerExistential() {
 }
 
 bool TypeBase::isPureMoveOnly() {
-  if (auto *nom = getNominalOrBoundGenericNominal())
+  if (auto *nom = getAnyNominal())
     return nom->isMoveOnly();
 
   // if any components of the tuple are move-only, then the tuple is move-only.

lib/Sema/TypeCheckDecl.cpp

@@ -912,8 +912,9 @@ IsFinalRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
 }
 
 bool IsMoveOnlyRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
-  // For now only do this for nominal type decls.
-  if (isa<NominalTypeDecl>(decl)) {
+  // TODO: isPureMoveOnly and isMoveOnly and other checks are all spread out
+  // and need to be merged together.
+  if (isa<ClassDecl>(decl) || isa<StructDecl>(decl) || isa<EnumDecl>(decl)) {
       if (decl->getAttrs().hasAttribute<MoveOnlyAttr>()) {
         if (!decl->getASTContext().supportsMoveOnlyTypes())
             decl->diagnose(diag::moveOnly_requires_lexical_lifetimes);

lib/Sema/TypeCheckStmt.cpp

@@ -1262,7 +1262,8 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     // check the kind of type this discard statement appears within.
     if (!diagnosed) {
       auto *nominalDecl = fn->getDeclContext()->getSelfNominalTypeDecl();
-      Type nominalType = nominalDecl->getDeclaredType();
+      Type nominalType =
+          fn->mapTypeIntoContext(nominalDecl->getDeclaredInterfaceType());
 
       // must be noncopyable
       if (!nominalType->isPureMoveOnly()) {
@@ -1288,17 +1289,16 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
         // if the modules differ, so that you can discard a @frozen type from a
         // resilient module. But for now the proposal simply says that it has to
         // be the same module, which is probably better for everyone.
-        auto *typeDecl = nominalType->getAnyNominal();
         auto *fnModule = fn->getModuleContext();
-        auto *typeModule = typeDecl->getModuleContext();
+        auto *typeModule = nominalDecl->getModuleContext();
         if (fnModule != typeModule) {
           ctx.Diags.diagnose(DS->getDiscardLoc(), diag::discard_wrong_module,
                              nominalType);
           diagnosed = true;
         } else {
           assert(
-              !typeDecl->isResilient(fnModule, ResilienceExpansion::Maximal) &&
-              "trying to discard a type resilient to us!");
+              !nominalDecl->isResilient(fnModule, ResilienceExpansion::Maximal)
+                  && "trying to discard a type resilient to us!");
         }
       }
     }
@@ -1314,13 +1314,14 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
     if (!diagnosed) {
       bool isSelf = false;
       auto *checkE = DS->getSubExpr();
+      assert(fn->getImplicitSelfDecl() && "no self?");
 
       // Look through a load. Only expected if we're in an init.
       if (auto *load = dyn_cast<LoadExpr>(checkE))
           checkE = load->getSubExpr();
 
       if (auto DRE = dyn_cast<DeclRefExpr>(checkE))
-        isSelf = DRE->getDecl()->getName().isSimpleName("self");
+        isSelf = DRE->getDecl() == fn->getImplicitSelfDecl();
 
       if (!isSelf) {
         ctx.Diags

lib/Sema/TypeCheckType.cpp

@@ -2306,31 +2306,39 @@ bool TypeResolver::diagnoseMoveOnlyMissingOwnership(
   if (options.contains(TypeResolutionFlags::HasOwnership))
     return false;
 
-  // Do not run this on SIL files since there is currently a bug where we are
-  // trying to parse it in SILBoxes.
-  //
-  // To track what we want to do long term: rdar://105635373.
+  // Don't diagnose in SIL; ownership is already required there.
   if (options.contains(TypeResolutionFlags::SILType))
     return false;
 
-  diagnose(repr->getLoc(),
-           diag::moveonly_parameter_missing_ownership);
+  //////////////////
+  // At this point, we know we have a noncopyable parameter that is missing an
+  // ownership specifier, so we need to emit an error
+
+  // We don't yet support any ownership specifiers for parameters of subscript
+  // decls, give a tailored error message saying you simply can't use a
+  // noncopyable type here.
+  if (options.hasBase(TypeResolverContext::SubscriptDecl)) {
+    diagnose(repr->getLoc(), diag::moveonly_parameter_subscript_unsupported);
+  } else {
+    // general error diagnostic
+    diagnose(repr->getLoc(),
+             diag::moveonly_parameter_missing_ownership);
 
-  diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
-           "borrowing", "for an immutable reference")
-      .fixItInsert(repr->getStartLoc(), "borrowing ");
+    diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
+             "borrowing", "for an immutable reference")
+        .fixItInsert(repr->getStartLoc(), "borrowing ");
 
-  diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
-           "inout", "for a mutable reference")
-      .fixItInsert(repr->getStartLoc(), "inout ");
+    diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
+             "inout", "for a mutable reference")
+        .fixItInsert(repr->getStartLoc(), "inout ");
 
-  diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
-           "consuming", "to take the value from the caller")
-      .fixItInsert(repr->getStartLoc(), "consuming ");
+    diagnose(repr->getLoc(), diag::moveonly_parameter_ownership_suggestion,
+             "consuming", "to take the value from the caller")
+        .fixItInsert(repr->getStartLoc(), "consuming ");
+  }
 
   // to avoid duplicate diagnostics
   repr->setInvalid();
-
   return true;
 }
 

test/Interpreter/moveonly_discard.swift

@@ -9,8 +9,7 @@ func print_closing_MFD() { print("closing MaybeFileDescriptor") }
 
 enum E: Error { case err }
 
-@_moveOnly
-struct FileDescriptor {
+struct FileDescriptor: ~Copyable {
   var fd: Int
   static var nextFD: Int = 0
 
@@ -59,7 +58,7 @@ struct FileDescriptor {
   }
 }
 
-@_moveOnly enum MaybeFileDescriptor {
+enum MaybeFileDescriptor: ~Copyable {
   case some(FileDescriptor)
   case nothing
 
@@ -83,6 +82,14 @@ struct FileDescriptor {
   }
 }
 
+struct SillyEmptyGeneric<T>: ~Copyable {
+  consuming func identity(_ t: T) -> T {
+    discard self
+    return t
+  }
+  deinit { fatalError("ran unexpectedly!") }
+}
+
 func main() {
   let _ = {
     let x = FileDescriptor() // 0
@@ -161,6 +168,13 @@ func main() {
     // CHECK: closing file descriptor: 13
   }()
 
+  let _ = {
+    let x = SillyEmptyGeneric<[Int]>()
+    let z = [1, 2]
+    let ans = x.identity(z)
+    assert(z == ans)
+  }()
+
 }
 
 main()

test/Sema/discard.swift

@@ -161,3 +161,34 @@ enum NoDeinitEnum: ~Copyable {
     discard self // expected-error {{'discard' has no effect for type 'NoDeinitEnum' unless it has a deinitializer}}{{5-18=}}
   }
 }
+
+struct HasGenericNotStored<T>: ~Copyable { // expected-note 2{{arguments to generic parameter 'T' ('Int' and 'T') are expected to be equal}}
+  consuming func discard() { discard self }
+
+  consuming func bad_discard1() {
+    discard HasGenericNotStored<Int>()
+    // expected-error@-1 {{cannot convert value of type 'HasGenericNotStored<Int>' to expected discard type 'HasGenericNotStored<T>'}}
+    // expected-error@-2 {{you can only discard 'self'}}
+  }
+
+  consuming func bad_discard2() {
+    let `self` = HasGenericNotStored<Int>()
+    discard `self`
+    // expected-error@-1 {{cannot convert value of type 'HasGenericNotStored<Int>' to expected discard type 'HasGenericNotStored<T>'}}
+    // expected-error@-2 {{you can only discard 'self'}}{{13-19=self}}
+  }
+
+  func identity(_ t: T) -> T { return t }
+  deinit{}
+}
+
+struct Court: ~Copyable {
+  let x: Int
+
+  consuming func discard(_ other: consuming Court) {
+    let `self` = other
+    discard `self` // expected-error {{you can only discard 'self'}}{{13-19=self}}
+  }
+
+  deinit { print("deinit of \(self.x)") }
+}

test/Sema/discard_trivially_destroyed.swift

@@ -98,3 +98,15 @@ struct AllOK: ~Copyable {
   consuming func doDiscard() { discard self }
   deinit {}
 }
+
+struct HasGenericStored<T>: ~Copyable {
+  let t: T // expected-note {{type 'T' cannot be trivially destroyed}}
+  consuming func discard() { discard self } // expected-error {{can only 'discard' type 'HasGenericStored<T>' if it contains trivially-destroyed stored properties at this time}}
+  deinit{}
+}
+
+struct HasAny: ~Copyable {
+  var t: Any // expected-note {{type 'Any' cannot be trivially destroyed}}
+  consuming func discard() { discard self } // expected-error {{can only 'discard' type 'HasAny' if it contains trivially-destroyed stored properties at this time}}
+  deinit{}
+}

test/Sema/moveonly_require_ownership_specifier.swift

@@ -84,3 +84,12 @@ func takeInstantiated(_ x: NoncopyableWrapper<Int>) {}
 // expected-note@-2 {{add 'borrowing' for an immutable reference}}{{28-28=borrowing }}
 // expected-note@-3 {{add 'inout' for a mutable reference}}{{28-28=inout }}
 // expected-note@-4 {{add 'consuming' to take the value from the caller}}{{28-28=consuming }}
+
+struct O: ~Copyable {}
+
+public struct M: ~Copyable {
+  subscript(_ i: O) -> Int { // expected-error {{subscripts cannot have noncopyable parameters}}
+    get { fatalError() }
+    set { }
+  }
+}