Share diagnoseAndRemoveAttr()

Merge together several helpers and code patterns for “diagnose/fix-it/invalidate bad attribute” into helper functions in TypeChecker.h.

This requires minor test changes in some places where we’re testing ObjC interop without importing Foundation; it’s otherwise NFC.

Author: beccadax

lib/Sema/BuilderTransform.cpp

@@ -1587,10 +1587,7 @@ Optional<BraceStmt *> TypeChecker::applyResultBuilderBodyTransform(
     }
 
     if (attr) {
-      ctx.Diags.diagnose(
-          attr->getLocation(), diag::result_builder_remove_attr)
-        .fixItRemove(attr->getRangeWithAt());
-      attr->setInvalid();
+      diagnoseAndRemoveAttr(func, attr, diag::result_builder_remove_attr);
     }
 
     // Note that one can remove all of the return statements.

lib/Sema/TypeCheckAttr.cpp

@@ -45,35 +45,6 @@
 using namespace swift;
 
 namespace {
-  /// This emits a diagnostic with a fixit to remove the attribute.
-  template<typename ...ArgTypes>
-  InFlightDiagnostic
-  diagnoseAndRemoveAttr(DiagnosticEngine &Diags, Decl *D, DeclAttribute *attr,
-                        ArgTypes &&...Args) {
-    attr->setInvalid();
-
-    assert(!D->hasClangNode() && "Clang importer propagated a bogus attribute");
-    if (!D->hasClangNode()) {
-      SourceLoc loc = attr->getLocation();
-#ifndef NDEBUG
-      if (!loc.isValid() && !attr->getAddedByAccessNote()) {
-        llvm::errs() << "Attribute '";
-        attr->print(llvm::errs());
-        llvm::errs() << "' has invalid location, failed to diagnose!\n";
-        assert(false && "Diagnosing attribute with invalid location");
-      }
-#endif
-      if (loc.isInvalid()) {
-        loc = D->getLoc();
-      }
-      if (loc.isValid()) {
-        return std::move(Diags.diagnose(loc, std::forward<ArgTypes>(Args)...)
-                            .fixItRemove(attr->getRangeWithAt()));
-      }
-    }
-    return InFlightDiagnostic();
-  }
-
 /// This visits each attribute on a decl.  The visitor should return true if
 /// the attribute is invalid and should be marked as such.
 class AttributeChecker : public AttributeVisitor<AttributeChecker> {
@@ -87,8 +58,8 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   template<typename ...ArgTypes>
   InFlightDiagnostic diagnoseAndRemoveAttr(DeclAttribute *attr,
                                            ArgTypes &&...Args) {
-    return ::diagnoseAndRemoveAttr(Ctx.Diags, D, attr,
-                                   std::forward<ArgTypes>(Args)...);
+    return swift::diagnoseAndRemoveAttr(D, attr,
+                                        std::forward<ArgTypes>(Args)...);
   }
 
   template <typename... ArgTypes>
@@ -955,8 +926,7 @@ static void diagnoseObjCAttrWithoutFoundation(ObjCAttr *attr, Decl *decl,
   auto &ctx = SF->getASTContext();
 
   if (!ctx.LangOpts.EnableObjCInterop) {
-    ctx.Diags.diagnose(attr->getLocation(), diag::objc_interop_disabled)
-      .fixItRemove(attr->getRangeWithAt())
+    diagnoseAndRemoveAttr(decl, attr, diag::objc_interop_disabled)
       .limitBehavior(behavior);
     return;
   }
@@ -4186,8 +4156,6 @@ resolveDifferentiableAttrOriginalFunction(DifferentiableAttr *attr) {
   auto *D = attr->getOriginalDeclaration();
   assert(D &&
          "Original declaration should be resolved by parsing/deserialization");
-  auto &ctx = D->getASTContext();
-  auto &diags = ctx.Diags;
   auto *original = dyn_cast<AbstractFunctionDecl>(D);
   if (auto *asd = dyn_cast<AbstractStorageDecl>(D)) {
     // If `@differentiable` attribute is declared directly on a
@@ -4211,7 +4179,7 @@ resolveDifferentiableAttrOriginalFunction(DifferentiableAttr *attr) {
       original = nullptr;
   // Diagnose if original `AbstractFunctionDecl` could not be resolved.
   if (!original) {
-    diagnoseAndRemoveAttr(diags, D, attr, diag::invalid_decl_attribute, attr);
+    diagnoseAndRemoveAttr(D, attr, diag::invalid_decl_attribute, attr);
     attr->setInvalid();
     return nullptr;
   }
@@ -4541,8 +4509,7 @@ IndexSubset *DifferentiableAttributeTypeCheckRequest::evaluate(
         {getterDecl, resolvedDiffParamIndices}, newAttr);
     // Reject duplicate `@differentiable` attributes.
     if (!insertion.second) {
-      diagnoseAndRemoveAttr(diags, D, attr,
-                            diag::differentiable_attr_duplicate);
+      diagnoseAndRemoveAttr(D, attr, diag::differentiable_attr_duplicate);
       diags.diagnose(insertion.first->getSecond()->getLocation(),
                      diag::differentiable_attr_duplicate_note);
       return nullptr;
@@ -4558,7 +4525,7 @@ IndexSubset *DifferentiableAttributeTypeCheckRequest::evaluate(
   auto insertion =
       ctx.DifferentiableAttrs.try_emplace({D, resolvedDiffParamIndices}, attr);
   if (!insertion.second && insertion.first->getSecond() != attr) {
-    diagnoseAndRemoveAttr(diags, D, attr, diag::differentiable_attr_duplicate);
+    diagnoseAndRemoveAttr(D, attr, diag::differentiable_attr_duplicate);
     diags.diagnose(insertion.first->getSecond()->getLocation(),
                    diag::differentiable_attr_duplicate_note);
     return nullptr;

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1160,10 +1160,6 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
     auto reason = objCReasonForObjCAttr(attr);
     auto behavior = behaviorLimitForObjCReason(reason, ctx);
 
-    SourceLoc attrLoc = attr->getLocation();
-    if (attrLoc.isInvalid())
-      attrLoc = CD->getLoc();
-
     if (ancestry.contains(AncestryFlags::Generic)) {
       if (attr->hasName() && !CD->isGenericContext()) {
         // @objc with a name on a non-generic subclass of a generic class is
@@ -1173,8 +1169,7 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
         return None;
       }
 
-      ctx.Diags.diagnose(attrLoc, diag::objc_for_generic_class)
-        .fixItRemove(attr->getRangeWithAt())
+      swift::diagnoseAndRemoveAttr(CD, attr, diag::objc_for_generic_class)
         .limitBehavior(behavior);
     }
 
@@ -1194,12 +1189,11 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
       auto platform = prettyPlatformString(targetPlatform(ctx.LangOpts));
       auto range = getMinOSVersionForClassStubs(target);
       auto *ancestor = getResilientAncestor(CD->getParentModule(), CD);
-      ctx.Diags.diagnose(attrLoc,
-                         diag::objc_for_resilient_class,
-                         ancestor->getName(),
-                         platform,
-                         range.getLowerEndpoint())
-        .fixItRemove(attr->getRangeWithAt())
+      swift::diagnoseAndRemoveAttr(CD, attr,
+                                   diag::objc_for_resilient_class,
+                                   ancestor->getName(),
+                                   platform,
+                                   range.getLowerEndpoint())
         .limitBehavior(behavior);
     }
 
@@ -1208,9 +1202,8 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
     if (ancestry.contains(AncestryFlags::ObjC) &&
         !ancestry.contains(AncestryFlags::ClangImported)) {
       if (ctx.LangOpts.EnableObjCAttrRequiresFoundation) {
-        ctx.Diags.diagnose(attrLoc,
-                           diag::invalid_objc_swift_rooted_class)
-          .fixItRemove(attr->getRangeWithAt())
+        swift::diagnoseAndRemoveAttr(CD, attr,
+                                     diag::invalid_objc_swift_rooted_class)
           .limitBehavior(behavior);
         // If the user has not spelled out a superclass, offer to insert
         // 'NSObject'. We could also offer to replace the existing superclass,
@@ -1229,8 +1222,7 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
       }
 
       if (!ctx.LangOpts.EnableObjCInterop) {
-        ctx.Diags.diagnose(attrLoc, diag::objc_interop_disabled)
-          .fixItRemove(attr->getRangeWithAt())
+        swift::diagnoseAndRemoveAttr(CD, attr, diag::objc_interop_disabled)
           .limitBehavior(behavior);
       }
     }

lib/Sema/TypeCheckStorage.cpp

@@ -2992,46 +2992,37 @@ static void finishProtocolStorageImplInfo(AbstractStorageDecl *storage,
   }
 }
 
-/// This emits a diagnostic with a fixit to remove the attribute.
-template<typename ...ArgTypes>
-void diagnoseAndRemoveAttr(Decl *D, DeclAttribute *attr,
-                           ArgTypes &&...Args) {
-  auto &ctx = D->getASTContext();
-  ctx.Diags.diagnose(attr->getLocation(), std::forward<ArgTypes>(Args)...)
-    .fixItRemove(attr->getRangeWithAt());
-}
-
 static void finishLazyVariableImplInfo(VarDecl *var,
                                        StorageImplInfo &info) {
   auto *attr = var->getAttrs().getAttribute<LazyAttr>();
 
   // It cannot currently be used on let's since we don't have a mutability model
   // that supports it.
   if (var->isLet())
-    diagnoseAndRemoveAttr(var, attr, diag::lazy_not_on_let);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::lazy_not_on_let);
 
   // lazy must have an initializer.
   if (!var->getParentInitializer())
-    diagnoseAndRemoveAttr(var, attr, diag::lazy_requires_initializer);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::lazy_requires_initializer);
 
   bool invalid = false;
 
   if (isa<ProtocolDecl>(var->getDeclContext())) {
-    diagnoseAndRemoveAttr(var, attr, diag::lazy_not_in_protocol);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::lazy_not_in_protocol);
     invalid = true;
   }
 
   // Lazy properties must be written as stored properties in the source.
   if (info.getReadImpl() != ReadImplKind::Stored &&
       (info.getWriteImpl() != WriteImplKind::Stored &&
        info.getWriteImpl() != WriteImplKind::StoredWithObservers)) {
-    diagnoseAndRemoveAttr(var, attr, diag::lazy_not_on_computed);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::lazy_not_on_computed);
     invalid = true;
   }
 
   // The pattern binding must only bind a single variable.
   if (!var->getParentPatternBinding()->getSingleVar())
-    diagnoseAndRemoveAttr(var, attr, diag::lazy_requires_single_var);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::lazy_requires_single_var);
 
   if (!invalid)
     info = StorageImplInfo::getMutableComputed();
@@ -3083,7 +3074,7 @@ static void finishNSManagedImplInfo(VarDecl *var,
   auto *attr = var->getAttrs().getAttribute<NSManagedAttr>();
 
   if (var->isLet())
-    diagnoseAndRemoveAttr(var, attr, diag::attr_NSManaged_let_property);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::attr_NSManaged_let_property);
 
   SourceFile *parentFile = var->getDeclContext()->getParentSourceFile();
 
@@ -3095,7 +3086,7 @@ static void finishNSManagedImplInfo(VarDecl *var,
     if (parentFile && parentFile->Kind == SourceFileKind::Interface)
       return;
 
-    diagnoseAndRemoveAttr(var, attr, diag::attr_NSManaged_not_stored, kind);
+    diagnoseAttrWithRemovalFixIt(var, attr, diag::attr_NSManaged_not_stored, kind);
   };
 
   // @NSManaged properties must be written as stored.

lib/Sema/TypeChecker.h

@@ -38,6 +38,9 @@
 
 namespace swift {
 
+class Decl;
+class DeclAttribute;
+class DiagnosticEngine;
 class ExportContext;
 class GenericSignatureBuilder;
 class NominalTypeDecl;
@@ -1317,6 +1320,45 @@ void bindSwitchCasePatternVars(DeclContext *dc, CaseStmt *stmt);
 AnyFunctionType *applyGlobalActorType(
     AnyFunctionType *fnType, ValueDecl *funcOrEnum, DeclContext *dc);
 
+/// Diagnose an error concerning an incorrect attribute and emit a fix-it
+/// offering to remove it.
+template<typename ...ArgTypes>
+InFlightDiagnostic
+diagnoseAttrWithRemovalFixIt(const Decl *D, const DeclAttribute *attr,
+                             ArgTypes &&...Args) {
+  assert(D);
+
+  if (D->hasClangNode() && (!attr || !attr->getAddedByAccessNote())) {
+    assert(false && "Clang importer propagated a bogus attribute");
+    return InFlightDiagnostic();
+  }
+
+  auto &Diags = D->getASTContext().Diags;
+
+  if (!attr || !attr->getLocation().isValid())
+    return Diags.diagnose(D, std::forward<ArgTypes>(Args)...);
+
+  return std::move(Diags.diagnose(attr->getLocation(),
+                                  std::forward<ArgTypes>(Args)...)
+                       .fixItRemove(attr->getRangeWithAt()));
+}
+
+/// Diagnose an error concerning an incorrect attribute, emit a fix-it offering
+/// to remove it, and mark it invalid so that it will be ignored by other parts
+/// of the compiler.
+template<typename ...ArgTypes>
+InFlightDiagnostic
+diagnoseAndRemoveAttr(const Decl *D, const DeclAttribute *attr,
+                      ArgTypes &&...Args) {
+  if (attr)
+    // FIXME: Due to problems with the design of DeclAttributes::getAttribute(),
+    // many callers try to pass us const DeclAttributes. This is a hacky
+    // workaround.
+    const_cast<DeclAttribute *>(attr)->setInvalid();
+
+  return diagnoseAttrWithRemovalFixIt(D, attr, std::forward<ArgTypes>(Args)...);
+}
+
 } // end namespace swift
 
 #endif

test/Index/kinds_objc.swift

@@ -1,6 +1,8 @@
 // RUN: %target-swift-ide-test -print-indexed-symbols -source-filename %s | %FileCheck %s
 // REQUIRES: objc_interop
 
+import Foundation
+
 @objc class TargetForIBAction {}
 // CHECK: [[@LINE-1]]:13 | class/Swift | TargetForIBAction | [[TargetForIBAction_USR:.*]] | Def |
 @objc class TargetForIBSegueAction {}