Check attributes in @abi attr

This commit compares the attributes on the decl inside the `@abi` attribute to those in the decl it’s attached to, diagnosing ABI-incompatible differences. It also rejects many attributes that don’t need to be specified in the `@abi` attribute, such as ObjC-ness, access control, or ABI-neutral traits like `@discardableResult`, so developers know to remove them.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -8376,6 +8376,27 @@ ERROR(attr_abi_incompatible_with_silgen_name,none,
       "the same purpose",
       (DescriptiveDeclKind))
 
+ERROR(attr_abi_missing_attr,none,
+      "missing '%0' %select{attribute|modifier}1 in '@abi'",
+      (StringRef, bool))
+ERROR(attr_abi_extra_attr,none,
+      "extra %select{|implicit }2'%0' %select{attribute|modifier}1 in '@abi'",
+      (StringRef, bool, /*isImplicit=*/bool))
+ERROR(attr_abi_forbidden_attr,none,
+      "unused '%0' %select{attribute|modifier}1 in '@abi'",
+      (StringRef, bool))
+REMARK(abi_attr_inferred_attribute,none,
+       "inferred '%0' in '@abi' to match %select{attribute|modifier}1 on API",
+       (StringRef, bool))
+
+ERROR(attr_abi_mismatched_attr,none,
+      "'%0' %select{attribute|modifier}1 in '@abi' should match '%2'",
+      (StringRef, bool, StringRef))
+NOTE(attr_abi_matching_attr_here,none,
+     "%select{should match|matches}0 %select{attribute|modifier}1 "
+     "%select{|implicitly added }2here",
+     (/*matches=*/bool, /*isModifier=*/bool, /*isImplicit=*/bool))
+
 ERROR(attr_abi_mismatched_type,none,
       "type %0 in '@abi' should match %1",
       (Type, Type))

include/swift/Basic/LangOptions.h

@@ -269,6 +269,9 @@ namespace swift {
     /// Emit a remark on early exit in explicit interface build
     bool EnableSkipExplicitInterfaceModuleBuildRemarks = false;
 
+    /// Emit a remark when \c \@abi infers an attribute or modifier.
+    bool EnableABIInferenceRemarks = false;
+
     ///
     /// Support for alternate usage modes
     ///

include/swift/Option/Options.td

@@ -468,6 +468,10 @@ def remark_module_serialization : Flag<["-"], "Rmodule-serialization">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
   HelpText<"Emit remarks about module serialization">;
 
+def remark_abi_inference : Flag<["-"], "Rabi-inference">,
+  Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Emit a remark when an '@abi' attribute adds an attribute or modifier to the ABI declaration based on its presence in the API">;
+
 def emit_tbd : Flag<["-"], "emit-tbd">,
   HelpText<"Emit a TBD file">,
   Flags<[FrontendOption, NoInteractiveOption, SupplementaryOutput]>;

lib/Frontend/CompilerInvocation.cpp

@@ -1422,6 +1422,8 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
 
   Opts.EnableSkipExplicitInterfaceModuleBuildRemarks = Args.hasArg(OPT_remark_skip_explicit_interface_build);
 
+  Opts.EnableABIInferenceRemarks = Args.hasArg(OPT_remark_abi_inference);
+
   if (Args.hasArg(OPT_experimental_skip_non_exportable_decls)) {
     // Only allow -experimental-skip-non-exportable-decls if either library
     // evolution is enabled (in which case the module's ABI is independent of

lib/Sema/TypeCheckAttr.cpp

@@ -6672,6 +6672,12 @@ static bool typeCheckDerivativeAttr(DerivativeAttr *attr) {
   // Note: Implementation must be idempotent because it may be called multiple
   // times for the same attribute.
   Decl *D = attr->getOriginalDeclaration();
+
+  // ABI-only decls can't have @derivative; bail out and let ABIDeclChecker
+  // diagnose this.
+  if (!ABIRoleInfo(D).providesAPI())
+    return false;
+
   auto &Ctx = D->getASTContext();
   auto &diags = Ctx.Diags;
   // `@derivative` attribute requires experimental differentiable programming

lib/Sema/TypeCheckAttrABI.cpp

@@ -20,6 +20,7 @@
 #include "swift/AST/Effects.h"
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ParameterList.h"
+#include "swift/AST/ASTPrinter.h"
 #include "swift/AST/Types.h"
 #include "swift/Basic/Assertions.h"
 #include "swift/Parse/Lexer.h"
@@ -129,6 +130,40 @@ void fixItReplaceKeywords(InFlightDiagnostic &diag,
   return fixItReplaceKeywords(diag, charRange, newText);
 }
 
+/// Returns a string representation of \p attr suitable to either replace an
+/// existing attribute or be inserted as a new attribute, depending on the
+/// value of \p toInsert .
+StringRef printAttr(DeclAttribute *attr,
+                    Decl *decl,
+                    SmallVectorImpl<char> &scratch,
+                    bool toInsert = false) {
+  auto &ctx = decl->getASTContext();
+  auto opts = PrintOptions::printForDiagnostics(AccessLevel::Private,
+                              ctx.TypeCheckerOpts.PrintFullConvention);
+  opts.PrintLongAttrsOnSeparateLines = false;
+
+  llvm::raw_svector_ostream os{scratch};
+  StreamPrinter printer{os};
+  attr->print(printer, opts, decl);
+
+  auto str = StringRef(scratch.begin(), scratch.size());
+  if (!toInsert)
+    str = str.trim(' ');
+  return str;
+}
+
+/// Emit \c diag::attr_abi_matching_attr_here in the best available location.
+void noteAttrHere(DeclAttribute *attr, Decl *decl, bool isMatch = false) {
+  auto &ctx = decl->getASTContext();
+  SourceLoc loc = attr->getLocation();
+  if (loc.isValid())
+    ctx.Diags.diagnose(loc, diag::attr_abi_matching_attr_here,
+                       isMatch, attr->isDeclModifier(), attr->isImplicit());
+  else
+    ctx.Diags.diagnose(decl, diag::attr_abi_matching_attr_here,
+                       isMatch, attr->isDeclModifier(), attr->isImplicit());
+}
+
 /// Get the best available \c SourceLoc representing the type in \p storage .
 SourceLoc getTypeLoc(AbstractStorageDecl *storage, Decl *owner = nullptr) {
   auto loc = storage->getTypeSourceRangeForDiagnostics().Start;
@@ -187,7 +222,11 @@ class ABIDeclChecker : public ASTComparisonVisitor<ABIDeclChecker> {
     bool didDiagnose = false;
 
     auto noteShouldMatch = [&](bool isModifier) {
-      ctx.Diags.diagnose(apiTypeLoc, diag::attr_abi_should_match_type_here);
+      if (isSelfParam)
+        ctx.Diags.diagnose(apiTypeLoc, diag::attr_abi_matching_attr_here,
+                           /*matches=*/false, isModifier, /*isImplicit=*/false);
+      else
+        ctx.Diags.diagnose(apiTypeLoc, diag::attr_abi_should_match_type_here);
     };
 
     // These assertions represent values that should have been normalized.
@@ -646,13 +685,187 @@ class ABIDeclChecker : public ASTComparisonVisitor<ABIDeclChecker> {
 
   // MARK: @abi checking - attributes
 
+  /// Are these attributes similar enough that they should be checked against
+  /// one another? At minimum this means they're of the same kind, but for some
+  /// attrs there are additional criteria.
+  bool canCompareAttrs(DeclAttribute *api, DeclAttribute *abi,
+                       Decl *apiDecl, Decl *abiDecl) {
+    if (api->getKind() != abi->getKind())
+      return false;
+
+    auto getAvailableDomain = [](Decl *D, DeclAttribute *A) {
+      return D->getSemanticAvailableAttr(cast<AvailableAttr>(A))->getDomain();
+    };
+
+    // Extra logic for specific attributes.
+    switch (api->getKind()) {
+    case DeclAttrKind::Expose:
+      return cast<ExposeAttr>(api)->getExposureKind()
+                  == cast<ExposeAttr>(abi)->getExposureKind();
+
+    case DeclAttrKind::Extern:
+      return cast<ExternAttr>(api)->getExternKind()
+                  == cast<ExternAttr>(abi)->getExternKind();
+
+    case DeclAttrKind::Available:
+      return getAvailableDomain(apiDecl, api)
+                  == getAvailableDomain(abiDecl, abi);
+      return true;
+
+    default:
+      break;
+    }
+
+    return true;
+  }
+
+  /// Check two attribute lists against one another.
+  /// 
+  /// This pairs up attributes which are sufficiently similar (as determined by
+  /// \c canCompareAttrs() ) and then checks them. Attributes which
+  /// have no counterpart are checked individually. 
   bool checkAttrs(DeclAttributes api, DeclAttributes abi,
                   Decl *apiDecl, Decl *abiDecl) {
     bool didDiagnose = false;
-    // TODO: Actually check attributes
+
+    // Collect all ABI attrs.
+    SmallVector<DeclAttribute*, 32> remainingABIDeclAttrs;
+    for (auto *abiDeclAttr : abi) {
+      remainingABIDeclAttrs.push_back(abiDeclAttr);
+    }
+
+    // Visit each API attr, pairing it with an ABI attr if possible.
+    // Note that this will visit even invalid attributes.
+    for (auto *apiDeclAttr : api) {
+      auto abiAttrIter = llvm::find_if(remainingABIDeclAttrs,
+                                        [&](DeclAttribute *abiDeclAttr) {
+        return abiDeclAttr && canCompareAttrs(apiDeclAttr, abiDeclAttr,
+                                              apiDecl, abiDecl);
+      });
+      DeclAttribute *abiDeclAttr = nullptr;
+      if (abiAttrIter != remainingABIDeclAttrs.end()) {
+        // Found a matching ABI attr. Claim and use it.
+        std::swap(abiDeclAttr, *abiAttrIter);
+      }
+      didDiagnose |= checkAttr(apiDeclAttr, abiDeclAttr, apiDecl, abiDecl);
+    }
+
+    // Visit leftover ABI attrs.
+    for (auto *abiDeclAttr : remainingABIDeclAttrs) {
+      if (abiDeclAttr)
+        didDiagnose |= checkAttr(nullptr, abiDeclAttr, apiDecl, abiDecl);
+    }
     return didDiagnose;
   }
 
+  /// Check a single attribute against its counterpart. If an attribute has no
+  /// counterpart, the counterpart may be \c nullptr ; either \p abi or \p abi
+  /// may be \c nullptr , but never both. 
+  bool checkAttr(DeclAttribute *api, DeclAttribute *abi,
+                 Decl *apiDecl, Decl *abiDecl) {
+    ASSERT(api || abi && "checkAttr() should have at least one attribute");
+
+    // If either attribute has already been diagnosed, don't check here.
+    if ((api && api->isInvalid()) || (abi && abi->isInvalid()))
+      return true;
+
+    auto kind = api ? api->getKind() : abi->getKind();
+    auto behaviors = DeclAttribute::getBehaviors(kind);
+
+    switch (behaviors & DeclAttribute::InABIAttrMask) {
+    case DeclAttribute::UnreachableInABIAttr:
+      ASSERT(abiAttr->canAppearOnDecl(apiDecl)
+                && "checking @abi on decl that can't have it???");
+      ASSERT(!abiAttr->canAppearOnDecl(apiDecl)
+                 && "unreachable-in-@abi attr on reachable decl???");
+
+      // If the asserts are disabled, fall through to no checking.
+      LLVM_FALLTHROUGH;
+
+    case DeclAttribute::UnconstrainedInABIAttr:
+      // No checking required.
+      return false;
+
+    case DeclAttribute::ForbiddenInABIAttr:
+      // Diagnose if ABI has attribute.
+      if (abi) {
+        // A shorthand `@available(foo 1, bar 2, *)` attribute gets parsed into
+        // several separate `AvailableAttr`s, each with the full range of the
+        // shorthand attribute. If we've already diagnosed one of them, don't
+        // diagnose the rest; otherwise, record that we've diagnosed this one.
+        if (isa<AvailableAttr>(abi) &&
+              !diagnosedAvailableAttrSourceLocs.insert(abi->getLocation()))
+          return true;
+
+        diagnoseAndRemoveAttr(abi, diag::attr_abi_forbidden_attr,
+                              abi->getAttrName(), abi->isDeclModifier());
+        return true;
+      }
+
+      return false;
+
+    case DeclAttribute::InferredInABIAttr:
+      if (!abi && api->canClone()) {
+        // Infer an identical attribute.
+        abi = api->clone(ctx);
+        abi->setImplicit(true);
+        abiDecl->getAttrs().add(abi);
+
+        if (ctx.LangOpts.EnableABIInferenceRemarks) {
+          SmallString<64> scratch;
+          auto abiAttrAsString = printAttr(abi, abiDecl, scratch);
+
+          abiDecl->diagnose(diag::abi_attr_inferred_attribute,
+                            abiAttrAsString, api->isDeclModifier());
+          noteAttrHere(api, apiDecl, /*isMatch=*/true);
+        }
+      }
+
+      // Other than the cloning behavior, Inferred behaves like Equivalent.
+      LLVM_FALLTHROUGH;
+
+    case DeclAttribute::EquivalentInABIAttr:
+      // Diagnose if API doesn't have attribute.
+      if (!api) {
+        diagnoseAndRemoveAttr(abi, diag::attr_abi_extra_attr,
+                              abi->getAttrName(), abi->isDeclModifier(),
+                              abi->isImplicit());
+        return true;
+      }
+
+      // Diagnose if ABI doesn't have attribute.
+      if (!abi) {
+        SmallString<64> scratch;
+        auto apiAttrAsString = printAttr(api, apiDecl, scratch,
+                                         /*toInsert=*/true);
+
+        ctx.Diags.diagnose(abiDecl, diag::attr_abi_missing_attr,
+                           api->getAttrName(), api->isDeclModifier())
+          .fixItInsert(abiDecl->getAttributeInsertionLoc(api->isDeclModifier()),
+                       apiAttrAsString);
+        noteAttrHere(api, apiDecl);
+        return true;
+      }
+
+      // Diagnose if two attributes are mismatched.
+      if (!api->isEquivalent(abi, apiDecl)) {
+        SmallString<64> scratch;
+        auto apiAttrAsString = printAttr(api, apiDecl, scratch);
+
+        ctx.Diags.diagnose(abi->getLocation(), diag::attr_abi_mismatched_attr,
+                           abi->getAttrName(), abi->isDeclModifier(),
+                           apiAttrAsString)
+          .fixItReplace(abi->getRangeWithAt(), apiAttrAsString);
+        noteAttrHere(api, apiDecl);
+        return true;
+      }
+
+      return false;
+    }
+
+    llvm_unreachable("unknown InABIAttrMask behavior");
+  }
+
   // MARK: @abi checking - types
 
   bool checkType(Type api, Type abi, SourceLoc apiLoc, SourceLoc abiLoc) {

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -2173,6 +2173,10 @@ void swift::diagnoseAttrsAddedByAccessNote(SourceFile &SF) {
 
 evaluator::SideEffect
 ApplyAccessNoteRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
+  // Access notes don't apply to ABI-only attributes.
+  if (!ABIRoleInfo(VD).providesAPI())
+    return {};
+
   AccessNotesFile &notes = VD->getModuleContext()->getAccessNotes();
   if (auto note = notes.lookup(VD))
     applyAccessNote(VD, *note.get(), notes);

test/attr/Inputs/attr_abi.h

@@ -0,0 +1,3 @@
+void implementation1(void);
+void implementation2(void);
+void implementation3(void);