Soften general attr diagnostics for access notes

beccadax · beccadax · commit 1f134c1160d9 · 2021-03-24T14:55:06.000-07:00
diff --git a/include/swift/AST/Attr.h b/include/swift/AST/Attr.h
@@ -110,15 +110,18 @@ class DeclAttribute : public AttributeBase {
   union {
     uint64_t OpaqueBits;
 
-    SWIFT_INLINE_BITFIELD_BASE(DeclAttribute, bitmax(NumDeclAttrKindBits,8)+1+1,
+    SWIFT_INLINE_BITFIELD_BASE(DeclAttribute, bitmax(NumDeclAttrKindBits,8)+1+1+1,
       Kind : bitmax(NumDeclAttrKindBits,8),
       // Whether this attribute was implicitly added.
       Implicit : 1,
 
-      Invalid : 1
+      Invalid : 1,
+
+      /// Whether the attribute was created by an access note.
+      AddedByAccessNote : 1
     );
 
-    SWIFT_INLINE_BITFIELD(ObjCAttr, DeclAttribute, 1+1+1+1,
+    SWIFT_INLINE_BITFIELD(ObjCAttr, DeclAttribute, 1+1+1,
       /// Whether this attribute has location information that trails the main
       /// record, which contains the locations of the parentheses and any names.
       HasTrailingLocationInfo : 1,
@@ -128,10 +131,7 @@ class DeclAttribute : public AttributeBase {
 
       /// Whether the @objc was inferred using Swift 3's deprecated inference
       /// rules.
-      Swift3Inferred : 1,
-
-      /// Whether the @objc was created by an access note.
-      AddedByAccessNote : 1
+      Swift3Inferred : 1
     );
 
     SWIFT_INLINE_BITFIELD(DynamicReplacementAttr, DeclAttribute, 1,
@@ -194,6 +194,7 @@ class DeclAttribute : public AttributeBase {
     Bits.DeclAttribute.Kind = static_cast<unsigned>(DK);
     Bits.DeclAttribute.Implicit = Implicit;
     Bits.DeclAttribute.Invalid = false;
+    Bits.DeclAttribute.AddedByAccessNote = false;
   }
 
 private:
@@ -332,6 +333,18 @@ class DeclAttribute : public AttributeBase {
 
   bool isValid() const { return !isInvalid(); }
 
+
+  /// Determine whether this attribute was added by an access note. If it was,
+  /// the compiler will generally recover from failures involving this attribute
+  /// as though it is not present.
+  bool getAddedByAccessNote() const {
+    return Bits.DeclAttribute.AddedByAccessNote;
+  }
+
+  void setAddedByAccessNote(bool accessNote = true) {
+    Bits.DeclAttribute.AddedByAccessNote = accessNote;
+  }
+
   /// Returns the address of the next pointer field.
   /// Used for object deserialization.
   DeclAttribute **getMutableNext() {
@@ -752,7 +765,6 @@ class ObjCAttr final : public DeclAttribute,
     Bits.ObjCAttr.HasTrailingLocationInfo = false;
     Bits.ObjCAttr.ImplicitName = implicitName;
     Bits.ObjCAttr.Swift3Inferred = false;
-    Bits.ObjCAttr.AddedByAccessNote = false;
 
     if (name) {
       NameData = name->getOpaqueValue();
@@ -872,17 +884,6 @@ class ObjCAttr final : public DeclAttribute,
     Bits.ObjCAttr.Swift3Inferred = inferred;
   }
 
-  /// Determine whether this attribute was added by an access note. If it is,
-  /// the compiler will not treat it as a hard error if the attribute is
-  /// invalid.
-  bool getAddedByAccessNote() const {
-    return Bits.ObjCAttr.AddedByAccessNote;
-  }
-
-  void setAddedByAccessNote(bool accessNote = true) {
-    Bits.ObjCAttr.AddedByAccessNote = accessNote;
-  }
-
   /// Clear the name of this entity.
   void clearName() {
     NameData = nullptr;
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -47,13 +47,16 @@ using namespace swift;
 namespace {
   /// This emits a diagnostic with a fixit to remove the attribute.
   template<typename ...ArgTypes>
-  void diagnoseAndRemoveAttr(DiagnosticEngine &Diags, Decl *D,
-                             DeclAttribute *attr, ArgTypes &&...Args) {
+  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()) {
+      if (!loc.isValid() && !attr->getAddedByAccessNote()) {
         llvm::errs() << "Attribute '";
         attr->print(llvm::errs());
         llvm::errs() << "' has invalid location, failed to diagnose!\n";
@@ -64,12 +67,11 @@ namespace {
         loc = D->getLoc();
       }
       if (loc.isValid()) {
-        Diags.diagnose(loc, std::forward<ArgTypes>(Args)...)
-          .fixItRemove(attr->getRangeWithAt());
+        return std::move(Diags.diagnose(loc, std::forward<ArgTypes>(Args)...)
+                            .fixItRemove(attr->getRangeWithAt()));
       }
     }
-
-    attr->setInvalid();
+    return InFlightDiagnostic();
   }
 
 /// This visits each attribute on a decl.  The visitor should return true if
@@ -83,9 +85,10 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
 
   /// This emits a diagnostic with a fixit to remove the attribute.
   template<typename ...ArgTypes>
-  void diagnoseAndRemoveAttr(DeclAttribute *attr, ArgTypes &&...Args) {
-    ::diagnoseAndRemoveAttr(Ctx.Diags, D, attr,
-                            std::forward<ArgTypes>(Args)...);
+  InFlightDiagnostic diagnoseAndRemoveAttr(DeclAttribute *attr,
+                                           ArgTypes &&...Args) {
+    return ::diagnoseAndRemoveAttr(Ctx.Diags, D, attr,
+                                   std::forward<ArgTypes>(Args)...);
   }
 
   template <typename... ArgTypes>
@@ -936,7 +939,8 @@ static bool checkObjCDeclContext(Decl *D) {
   return false;
 }
 
-static void diagnoseObjCAttrWithoutFoundation(ObjCAttr *attr, Decl *decl) {
+static void diagnoseObjCAttrWithoutFoundation(ObjCAttr *attr, Decl *decl,
+                                              DiagnosticBehavior behavior) {
   auto *SF = decl->getDeclContext()->getParentSourceFile();
   assert(SF);
 
@@ -948,7 +952,8 @@ static void diagnoseObjCAttrWithoutFoundation(ObjCAttr *attr, Decl *decl) {
 
   if (!ctx.LangOpts.EnableObjCInterop) {
     ctx.Diags.diagnose(attr->getLocation(), diag::objc_interop_disabled)
-      .fixItRemove(attr->getRangeWithAt());
+      .fixItRemove(attr->getRangeWithAt())
+      .limitBehavior(behavior);
     return;
   }
 
@@ -968,10 +973,14 @@ static void diagnoseObjCAttrWithoutFoundation(ObjCAttr *attr, Decl *decl) {
   ctx.Diags.diagnose(attr->getLocation(),
                      diag::attr_used_without_required_module, attr,
                      ctx.Id_Foundation)
-    .highlight(attr->getRangeWithAt());
+    .highlight(attr->getRangeWithAt())
+    .limitBehavior(behavior);
 }
 
 void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
+  auto reason = objCReasonForObjCAttr(attr);
+  auto behavior = behaviorLimitForObjCReason(reason, Ctx);
+
   // Only certain decls can be ObjC.
   Optional<Diag<>> error;
   if (isa<ClassDecl>(D) ||
@@ -1007,7 +1016,7 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
   }
 
   if (error) {
-    diagnoseAndRemoveAttr(attr, *error);
+    diagnoseAndRemoveAttr(attr, *error).limitBehavior(behavior);
     return;
   }
 
@@ -1026,7 +1035,8 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
           Lexer::getLocForEndOfToken(Ctx.SourceMgr, firstNameLoc);
         diagnose(firstNameLoc, diag::objc_name_req_nullary,
                  D->getDescriptiveKind())
-          .fixItRemoveChars(afterFirstNameLoc, attr->getRParenLoc());
+          .fixItRemoveChars(afterFirstNameLoc, attr->getRParenLoc())
+          .limitBehavior(behavior);
         const_cast<ObjCAttr *>(attr)->setName(
           ObjCSelector(Ctx, 0, objcName->getSelectorPieces()[0]),
           /*implicit=*/false);
@@ -1035,7 +1045,8 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
       diagnose(attr->getLParenLoc(),
                isa<SubscriptDecl>(D)
                  ? diag::objc_name_subscript
-                 : diag::objc_name_deinit);
+                 : diag::objc_name_deinit)
+          .limitBehavior(behavior);
       const_cast<ObjCAttr *>(attr)->clearName();
     } else {
       auto func = cast<AbstractFunctionDecl>(D);
@@ -1070,19 +1081,21 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
                  numArgumentNames != 1,
                  numParameters,
                  numParameters != 1,
-                 func->hasThrows());
+                 func->hasThrows())
+            .limitBehavior(behavior);
         D->getAttrs().add(
           ObjCAttr::createUnnamed(Ctx, attr->AtLoc,  attr->Range.Start));
         D->getAttrs().removeAttribute(attr);
       }
     }
   } else if (isa<EnumElementDecl>(D)) {
     // Enum elements require names.
-    diagnoseAndRemoveAttr(attr, diag::objc_enum_case_req_name);
+    diagnoseAndRemoveAttr(attr, diag::objc_enum_case_req_name)
+        .limitBehavior(behavior);
   }
 
   // Diagnose an @objc attribute used without importing Foundation.
-  diagnoseObjCAttrWithoutFoundation(attr, D);
+  diagnoseObjCAttrWithoutFoundation(attr, D, behavior);
 }
 
 void AttributeChecker::visitNonObjCAttr(NonObjCAttr *attr) {
@@ -1132,7 +1145,7 @@ void AttributeChecker::visitOptionalAttr(OptionalAttr *attr) {
 void TypeChecker::checkDeclAttributes(Decl *D) {
   if (auto VD = dyn_cast<ValueDecl>(D))
     TypeChecker::applyAccessNote(VD);
-  
+
   AttributeChecker Checker(D);
   // We need to check all OriginallyDefinedInAttr relative to each other, so
   // collect them and check in batch later.
@@ -1177,13 +1190,20 @@ void TypeChecker::checkDeclAttributes(Decl *D) {
     default: break;
     }
 
+    DiagnosticBehavior behavior = attr->getAddedByAccessNote()
+                                ? DiagnosticBehavior::Remark
+                                : DiagnosticBehavior::Unspecified;
+
     if (!OnlyKind.empty())
       Checker.diagnoseAndRemoveAttr(attr, diag::attr_only_one_decl_kind,
-                                    attr, OnlyKind);
+                                    attr, OnlyKind)
+          .limitBehavior(behavior);
     else if (attr->isDeclModifier())
-      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_modifier, attr);
+      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_modifier, attr)
+          .limitBehavior(behavior);
     else
-      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_attribute, attr);
+      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_attribute, attr)
+          .limitBehavior(behavior);
   }
   Checker.checkOriginalDefinedInAttrs(D, ODIAttrs);
 }
diff --git a/lib/Sema/TypeCheckDeclObjC.cpp b/lib/Sema/TypeCheckDeclObjC.cpp
@@ -1127,7 +1127,7 @@ static bool isMemberOfObjCMembersClass(const ValueDecl *VD) {
   return classDecl->checkAncestry(AncestryFlags::ObjCMembers);
 }
 
-static ObjCReason reasonForObjCAttr(const ObjCAttr *attr) {
+ObjCReason swift::objCReasonForObjCAttr(const ObjCAttr *attr) {
   if (attr->getAddedByAccessNote())
     return ObjCReason::ExplicitlyObjCByAccessNote;
 
@@ -1206,7 +1206,7 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
       }
     }
 
-    return reasonForObjCAttr(CD->getAttrs().getAttribute<ObjCAttr>());
+    return objCReasonForObjCAttr(CD->getAttrs().getAttribute<ObjCAttr>());
   }
 
   if (ancestry.contains(AncestryFlags::ObjC)) {
@@ -1288,7 +1288,7 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
 
   // explicitly declared @objc.
   if (auto attr = VD->getAttrs().getAttribute<ObjCAttr>())
-    return reasonForObjCAttr(attr);
+    return objCReasonForObjCAttr(attr);
   // Getter or setter for an @objc property or subscript.
   if (auto accessor = dyn_cast<AccessorDecl>(VD)) {
     if (accessor->getAccessorKind() == AccessorKind::Get ||
@@ -1501,18 +1501,19 @@ bool IsObjCRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
     // Enums can be @objc so long as they have a raw type that is representable
     // as an arithmetic type in C.
     if (isEnumObjC(enumDecl))
-      isObjC = reasonForObjCAttr(enumDecl->getAttrs().getAttribute<ObjCAttr>());
+      isObjC = objCReasonForObjCAttr(
+                                 enumDecl->getAttrs().getAttribute<ObjCAttr>());
   } else if (auto enumElement = dyn_cast<EnumElementDecl>(VD)) {
     // Enum elements can be @objc so long as the containing enum is @objc.
     if (enumElement->getParentEnum()->isObjC()) {
       if (auto attr = enumElement->getAttrs().getAttribute<ObjCAttr>())
-        isObjC = reasonForObjCAttr(attr);
+        isObjC = objCReasonForObjCAttr(attr);
       else
         isObjC = ObjCReason::ElementOfObjCEnum;
     }
   } else if (auto proto = dyn_cast<ProtocolDecl>(VD)) {
     if (auto attr = proto->getAttrs().getAttribute<ObjCAttr>()) {
-      isObjC = reasonForObjCAttr(attr);
+      isObjC = objCReasonForObjCAttr(attr);
 
       // If the protocol is @objc, it may only refine other @objc protocols.
       // FIXME: Revisit this restriction.
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -1447,6 +1447,7 @@ static void addOrRemoveAttr(ValueDecl *VD, const AccessNotesFile &notes,
 
   if (*expected) {
     attr = willCreate();
+    attr->setAddedByAccessNote();
     VD->getAttrs().add(attr);
 
     SmallString<64> attrString;
@@ -1470,9 +1471,7 @@ static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
   ASTContext &ctx = VD->getASTContext();
 
   addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, [&]{
-    auto attr = ObjCAttr::create(ctx, note.ObjCName, false);
-    attr->setAddedByAccessNote();
-    return attr;
+    return ObjCAttr::create(ctx, note.ObjCName, false);
   });
 
   addOrRemoveAttr<DynamicAttr>(VD, notes, note.Dynamic, [&]{
diff --git a/lib/Sema/TypeCheckObjC.h b/lib/Sema/TypeCheckObjC.h
@@ -26,6 +26,7 @@ namespace swift {
 
 class AbstractFunctionDecl;
 class ASTContext;
+class ObjCAttr;
 class SubscriptDecl;
 class ValueDecl;
 class VarDecl;
@@ -121,6 +122,9 @@ class ObjCReason {
 DiagnosticBehavior
 behaviorLimitForObjCReason(ObjCReason reason, ASTContext &ctx);
 
+/// Returns the ObjCReason for this ObjCAttr to be attached to the declaration.
+ObjCReason objCReasonForObjCAttr(const ObjCAttr *attr);
+
 /// Return the %select discriminator for the OBJC_ATTR_SELECT macro used to
 /// complain about the correct attribute during @objc inference.
 unsigned getObjCDiagnosticAttrKind(ObjCReason reason);
