Fix objcImpl bug causing invalid @_hasStorage attributes in module interfaces

include/swift/AST/DiagnosticsSema.def

@@ -2216,6 +2216,9 @@ ERROR(invalid_decl_attribute,none,
 ERROR(attr_invalid_on_decl_kind,none,
       "'%0' attribute cannot be applied to %1 declarations",
       (DeclAttribute, DescriptiveDeclKind))
+ERROR(attr_invalid_in_context,none,
+      "'%0' attribute cannot be applied to declaration in %1",
+      (DeclAttribute, DescriptiveDeclKind))
 ERROR(invalid_decl_modifier,none,
       "%0 modifier cannot be applied to this declaration", (DeclAttribute))
 ERROR(attribute_does_not_apply_to_type,none,

include/swift/Frontend/ModuleInterfaceLoader.h

@@ -672,7 +672,8 @@ struct InterfaceSubContextDelegateImpl : InterfaceSubContextDelegate {
     return InterfaceSubContextDelegateImpl::diagnose(interfacePath, diagnosticLoc, SM, Diags, ID, std::move(Args)...);
   }
   void
-  inheritOptionsForBuildingInterface(const SearchPathOptions &SearchPathOpts,
+  inheritOptionsForBuildingInterface(FrontendOptions::ActionType requestedAction,
+                                     const SearchPathOptions &SearchPathOpts,
                                      const LangOptions &LangOpts,
                                      const ClangImporterOptions &clangImporterOpts,
                                      const CASOptions &casOpts,

lib/AST/ASTPrinter.cpp

@@ -1332,8 +1332,8 @@ void PrintAST::printAttributes(const Decl *D) {
 
       if (!Options.PrintForSIL) {
         // Don't print @_hasStorage if the value is simply stored, or the
-        // decl is resilient.
-        if (vd->isResilient() ||
+        // decl is resilient or in an `@objc @implementation` extension.
+        if (vd->isResilient() || isInObjCImpl(vd) ||
             (vd->getImplInfo().isSimpleStored() &&
              !hasLessAccessibleSetter(vd)))
           Options.ExcludeAttrList.push_back(DeclAttrKind::HasStorage);

lib/Frontend/ModuleInterfaceLoader.cpp

@@ -1654,6 +1654,7 @@ void ModuleInterfaceLoader::collectVisibleTopLevelModuleNames(
 }
 
 void InterfaceSubContextDelegateImpl::inheritOptionsForBuildingInterface(
+    FrontendOptions::ActionType requestedAction,
     const SearchPathOptions &SearchPathOpts, const LangOptions &LangOpts,
     const ClangImporterOptions &clangImporterOpts, const CASOptions &casOpts,
     bool suppressRemarks, RequireOSSAModules_t RequireOSSAModules) {
@@ -1741,9 +1742,12 @@ void InterfaceSubContextDelegateImpl::inheritOptionsForBuildingInterface(
   }
 
   // Inhibit warnings from the genericSubInvocation since we are assuming the user
-  // is not in a position to address them.
-  genericSubInvocation.getDiagnosticOptions().SuppressWarnings = true;
-  GenericArgs.push_back("-suppress-warnings");
+  // is not in a position to address them. (Unless we're verifying, in which
+  // case they might.)
+  if (requestedAction != FrontendOptions::ActionType::Typecheck) {
+    genericSubInvocation.getDiagnosticOptions().SuppressWarnings = true;
+    GenericArgs.push_back("-suppress-warnings");
+  }
   // Inherit the parent invocation's setting on whether to suppress remarks
   if (suppressRemarks) {
     genericSubInvocation.getDiagnosticOptions().SuppressRemarks = true;
@@ -1871,8 +1875,8 @@ InterfaceSubContextDelegateImpl::InterfaceSubContextDelegateImpl(
     RequireOSSAModules_t requireOSSAModules)
     : SM(SM), Diags(Diags), ArgSaver(Allocator) {
   genericSubInvocation.setMainExecutablePath(LoaderOpts.mainExecutablePath);
-  inheritOptionsForBuildingInterface(searchPathOpts, langOpts,
-                                     clangImporterOpts, casOpts,
+  inheritOptionsForBuildingInterface(LoaderOpts.requestedAction, searchPathOpts,
+                                     langOpts, clangImporterOpts, casOpts,
                                      Diags->getSuppressRemarks(),
                                      requireOSSAModules);
   // Configure front-end input.

lib/Sema/TypeCheckStorage.cpp

@@ -3748,8 +3748,12 @@ bool HasStorageRequest::evaluate(Evaluator &evaluator,
     return false;
 
   // @_hasStorage implies that it... has storage.
-  if (var->getAttrs().hasAttribute<HasStorageAttr>())
-    return true;
+  if (var->getAttrs().hasAttribute<HasStorageAttr>()) {
+    // Except in contexts where it would be invalid. This is diagnosed in
+    // StorageImplInfoRequest.
+    return !isa<ProtocolDecl, ExtensionDecl, EnumDecl>(
+               storage->getDeclContext()->getImplementedObjCContext());
+  }
 
   // Protocol requirements never have storage.
   if (isa<ProtocolDecl>(storage->getDeclContext()))
@@ -3843,10 +3847,21 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
         : StorageIsMutable);
   }
 
-  if (auto *var = dyn_cast<VarDecl>(storage)) {
+  // If we're in an @implementation extension, we care about the semantics of
+  // the decl it implements.
+  auto *DC = storage->getDeclContext()->getImplementedObjCContext();
+
+  if (auto attr = storage->getParsedAttrs().getAttribute<HasStorageAttr>()) {
+    // If we see `@_hasStorage` in a context with no stored properties, diagnose
+    // and ignore it.
+    if (isa<ExtensionDecl, EnumDecl, ProtocolDecl>(DC)) {
+      storage->diagnose(diag::attr_invalid_in_context, attr,
+                        DC->getAsDecl()->getDescriptiveKind())
+        .warnInSwiftInterface(storage->getDeclContext());
+
     // Allow the @_hasStorage attribute to override all the accessors we parsed
     // when making the final classification.
-    if (var->getParsedAttrs().hasAttribute<HasStorageAttr>()) {
+    } else if (auto *var = dyn_cast<VarDecl>(storage)) {
       // The SIL rules for @_hasStorage are slightly different from the non-SIL
       // rules. In SIL mode, @_hasStorage marks that the type is simply stored,
       // and the only thing that determines mutability is the existence of the
@@ -3941,7 +3956,6 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
   bool hasMutableAddress = storage->getParsedAccessor(AccessorKind::MutableAddress);
   bool hasInit = storage->getParsedAccessor(AccessorKind::Init);
 
-  auto *DC = storage->getDeclContext();
   // 'get', 'read', and a non-mutable addressor are all exclusive.
   ReadImplKind readImpl;
   if (storage->getParsedAccessor(AccessorKind::Get)) {

test/ModuleInterface/has_storage_attr_bad.swiftinterface

@@ -0,0 +1,9 @@
+// swift-interface-format-version: 1.0
+// swift-module-flags:
+
+// RUN: %target-swift-typecheck-module-from-interface(%s) -module-name Test -verify -verify-ignore-unknown
+// REQUIRES: OS=macosx
+
+extension Array {
+  @_hasStorage public var foo: Int { get set } // expected-warning {{'@_hasStorage' attribute cannot be applied to declaration in extension}}
+}

test/ModuleInterface/objc_implementation.swift

@@ -21,6 +21,9 @@ import Foundation
 // @objc should be omitted on extensions
 // NEGATIVE-NOT: @objc{{.*}} extension
 
+// Stored properties in objcImpl extensions shouldn't have @_hasStorage
+// NEGATIVE-NOT: @_hasStorage
+
 //
 // @_objcImplementation class
 //
@@ -42,6 +45,11 @@ import Foundation
   // CHECK-DAG: final public var implProperty2: ObjectiveC.NSObject? { get set }
   public final var implProperty2: NSObject?
 
+  // CHECK-DAG: final public var implProperty3: ObjectiveC.NSObject? {
+  public final var implProperty3: NSObject? {
+    didSet { }
+  }
+
   // CHECK-NOT: func mainMethod
   @objc public func mainMethod(_: Int32) { print(implProperty) }
 

test/SILOptimizer/keypath-folding-crash.sil

@@ -30,7 +30,7 @@ import SwiftShims
 import Builtin
 import CModule
 
-extension ObjClass {
+@objc @implementation extension ObjClass {
   @objc @_hasStorage dynamic var o: NSObject { get set }
 }
 

test/attr/attributes.swift

@@ -308,6 +308,9 @@ class ExclusivityAttrClass {
 class HasStorage {
   @_hasStorage var x : Int = 42  // ok, _hasStorage is allowed here
 }
+extension HasStorage {
+  @_hasStorage var y : Int { 24 } // expected-error {{'@_hasStorage' attribute cannot be applied to declaration in extension}}
+}
 
 @_show_in_interface protocol _underscored {}
 @_show_in_interface class _notapplicable {} // expected-error {{may only be used on 'protocol' declarations}}