Diagnose access note @objc failures using remarks

This commit adds an ObjCReason::ExplicitlyObjCByAccessNote value which diagnoses invalid uses, but using remarks instead of errors so that the failures don’t block builds even with -warnings-as-errors enabled.

This commit also adds annotations to attr/attr_objc.swift to generate a ton of access note test cases from it. In this commit, many of these test cases don’t pass yet. Subsequent commits will fix these bugs.

Author: beccadax

include/swift/AST/Attr.h

@@ -118,7 +118,7 @@ class DeclAttribute : public AttributeBase {
       Invalid : 1
     );
 
-    SWIFT_INLINE_BITFIELD(ObjCAttr, DeclAttribute, 1+1+1,
+    SWIFT_INLINE_BITFIELD(ObjCAttr, DeclAttribute, 1+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,7 +128,10 @@ class DeclAttribute : public AttributeBase {
 
       /// Whether the @objc was inferred using Swift 3's deprecated inference
       /// rules.
-      Swift3Inferred : 1
+      Swift3Inferred : 1,
+
+      /// Whether the @objc was created by an access note.
+      AddedByAccessNote : 1
     );
 
     SWIFT_INLINE_BITFIELD(DynamicReplacementAttr, DeclAttribute, 1,
@@ -749,6 +752,7 @@ 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();
@@ -868,6 +872,17 @@ 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;

include/swift/AST/DiagnosticsSema.def

@@ -4820,7 +4820,7 @@ ERROR(objc_extension_not_class,none,
       "'@objc' can only be applied to an extension of a class", ())
 
 // If you change this, also change enum ObjCReason
-#define OBJC_ATTR_SELECT "select{marked @_cdecl|marked dynamic|marked @objc|marked @IBOutlet|marked @IBAction|marked @IBSegueAction|marked @NSManaged|a member of an @objc protocol|implicitly @objc|an @objc override|an implementation of an @objc requirement|marked @IBInspectable|marked @GKInspectable|in an @objc extension of a class (without @nonobjc)}"
+#define OBJC_ATTR_SELECT "select{marked @_cdecl|marked dynamic|marked @objc|marked @IBOutlet|marked @IBAction|marked @IBSegueAction|marked @NSManaged|a member of an @objc protocol|implicitly @objc|an @objc override|an implementation of an @objc requirement|marked @IBInspectable|marked @GKInspectable|in an @objc extension of a class (without @nonobjc)|marked @objc by an access note}"
 
 WARNING(attribute_meaningless_when_nonobjc,none,
         "'@%0' attribute is meaningless on a property that cannot be "

lib/AST/Attr.cpp

@@ -1332,6 +1332,7 @@ SourceLoc ObjCAttr::getRParenLoc() const {
 ObjCAttr *ObjCAttr::clone(ASTContext &context) const {
   auto attr = new (context) ObjCAttr(getName(), isNameImplicit());
   attr->setSwift3Inferred(isSwift3Inferred());
+  attr->setAddedByAccessNote(getAddedByAccessNote());
   return attr;
 }
 

lib/Sema/TypeCheckDeclObjC.cpp

@@ -54,6 +54,9 @@ swift::behaviorLimitForObjCReason(ObjCReason reason, ASTContext &ctx) {
       return DiagnosticBehavior::Unspecified;
     return DiagnosticBehavior::Ignore;
 
+  case ObjCReason::ExplicitlyObjCByAccessNote:
+    return DiagnosticBehavior::Remark;
+
   case ObjCReason::MemberOfObjCSubclass:
   case ObjCReason::MemberOfObjCMembersClass:
   case ObjCReason::ElementOfObjCEnum:
@@ -79,6 +82,7 @@ unsigned swift::getObjCDiagnosticAttrKind(ObjCReason reason) {
   case ObjCReason::ExplicitlyIBInspectable:
   case ObjCReason::ExplicitlyGKInspectable:
   case ObjCReason::MemberOfObjCExtension:
+  case ObjCReason::ExplicitlyObjCByAccessNote:
     return static_cast<unsigned>(reason);
 
   case ObjCReason::MemberOfObjCSubclass:
@@ -114,6 +118,10 @@ static void describeObjCReason(const ValueDecl *VD, ObjCReason Reason) {
                 cast<ProtocolDecl>(requirement->getDeclContext())
                   ->getName());
   }
+  else if (Reason == ObjCReason::ExplicitlyObjCByAccessNote) {
+    // FIXME: Look up the Reason string in the access note and emit a note that
+    // includes it
+  }
 }
 
 static void diagnoseTypeNotRepresentableInObjC(const DeclContext *DC,
@@ -1119,6 +1127,13 @@ static bool isMemberOfObjCMembersClass(const ValueDecl *VD) {
   return classDecl->checkAncestry(AncestryFlags::ObjCMembers);
 }
 
+static ObjCReason reasonForObjCAttr(const ObjCAttr *attr) {
+  if (attr->getAddedByAccessNote())
+    return ObjCReason::ExplicitlyObjCByAccessNote;
+
+  return ObjCReason::ExplicitlyObjC;
+}
+
 // A class is @objc if it does not have generic ancestry, and it either has
 // an explicit @objc attribute, or its superclass is @objc.
 static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
@@ -1191,7 +1206,7 @@ static Optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
       }
     }
 
-    return ObjCReason(ObjCReason::ExplicitlyObjC);
+    return reasonForObjCAttr(CD->getAttrs().getAttribute<ObjCAttr>());
   }
 
   if (ancestry.contains(AncestryFlags::ObjC)) {
@@ -1272,8 +1287,8 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
   };
 
   // explicitly declared @objc.
-  if (VD->getAttrs().hasAttribute<ObjCAttr>())
-    return ObjCReason(ObjCReason::ExplicitlyObjC);
+  if (auto attr = VD->getAttrs().getAttribute<ObjCAttr>())
+    return reasonForObjCAttr(attr);
   // Getter or setter for an @objc property or subscript.
   if (auto accessor = dyn_cast<AccessorDecl>(VD)) {
     if (accessor->getAccessorKind() == AccessorKind::Get ||
@@ -1486,18 +1501,18 @@ 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 = ObjCReason(ObjCReason::ExplicitlyObjC);
+      isObjC = reasonForObjCAttr(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 (enumElement->getAttrs().hasAttribute<ObjCAttr>())
-        isObjC = ObjCReason::ExplicitlyObjC;
+      if (auto attr = enumElement->getAttrs().getAttribute<ObjCAttr>())
+        isObjC = reasonForObjCAttr(attr);
       else
         isObjC = ObjCReason::ElementOfObjCEnum;
     }
   } else if (auto proto = dyn_cast<ProtocolDecl>(VD)) {
-    if (proto->getAttrs().hasAttribute<ObjCAttr>()) {
-      isObjC = ObjCReason(ObjCReason::ExplicitlyObjC);
+    if (auto attr = proto->getAttrs().getAttribute<ObjCAttr>()) {
+      isObjC = reasonForObjCAttr(attr);
 
       // If the protocol is @objc, it may only refine other @objc protocols.
       // FIXME: Revisit this restriction.

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1470,7 +1470,9 @@ static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
   ASTContext &ctx = VD->getASTContext();
 
   addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, [&]{
-    return ObjCAttr::create(ctx, note.ObjCName, false);
+    auto attr = ObjCAttr::create(ctx, note.ObjCName, false);
+    attr->setAddedByAccessNote();
+    return attr;
   });
 
   addOrRemoveAttr<DynamicAttr>(VD, notes, note.Dynamic, [&]{

lib/Sema/TypeCheckObjC.h

@@ -69,6 +69,9 @@ class ObjCReason {
     ExplicitlyGKInspectable,
     /// Is it a member of an @objc extension of a class.
     MemberOfObjCExtension,
+    /// Has an explicit '@objc' attribute added by an access note, rather than
+    /// written in source code.
+    ExplicitlyObjCByAccessNote,
 
     // These kinds do not appear in diagnostics.
 

test/attr/Inputs/access-note-gen.py

@@ -0,0 +1,153 @@
+#!/usr/bin/env python
+# coding=utf8
+
+"""
+Convert selected @objc attributes in a source file into access notes, removing
+the originals in the process.
+"""
+
+from __future__ import print_function
+
+import io
+import re
+import sys
+
+#
+# Entry point
+#
+
+
+def main():
+    if len(sys.argv) != 4:
+        print('Too few args to ' + sys.argv[0])
+        print('Usage: access-note-gen.py <input-file> <output-source-file> ' +
+              '<output-access-notes-file>')
+        sys.exit(1)
+
+    with io.open(sys.argv[1], mode='r', encoding='utf8') as input_file, \
+         io.open(sys.argv[2], mode='w', encoding='utf8') as output_file, \
+         io.open(sys.argv[3], mode='w', encoding='utf8') as access_notes_file:
+
+        # Add header to access notes file
+        access_notes_file.write(u"""\
+Reason: 'fancy tests'
+Notes:""")
+
+        # Loop over input lines, transforming them into output lines, writing access
+        # notes as a side effect.
+        for input_line in input_file:
+            # Look for access-note-move comments.
+            input_line = access_note_move_re.sub(replacer(move_at_objc_to_access_note,
+                                                          access_notes_file),
+                                                 input_line, count=1)
+
+            # Look for access-note-adjust comments.
+            input_line = adjust_comment_re.sub(replacer(adjust_comments),
+                                               input_line, count=1)
+
+            output_file.write(input_line)
+
+
+#
+# Offsets
+#
+
+"""Matches an @±N offset."""
+offset_re_fragment = r'[ \t]*(?:@([+-]\d+))?[ \t]*'
+
+
+def offsetify(*offsets):
+    """Sum line offsets matched by offset_re_fragment and convert them to strings
+       like @+3 or @-2."""
+
+    offset = sum([int(o) for o in offsets if o is not None])
+    if offset < 0:
+        return u"@-" + str(-offset)
+    elif offset > 0:
+        return u"@+" + str(offset)
+    else:
+        return u""
+
+
+#
+# Adjusting comments
+#
+
+"""Matches expected-warning/note/remark and its offset."""
+expected_other_diag_re = re.compile(r'expected-(warning|note|remark)' +
+                                    offset_re_fragment)
+
+"""Matches expected-error and its offset."""
+expected_error_re = re.compile(r'expected-error' + offset_re_fragment)
+
+"""Matches the string 'marked @objc'."""
+marked_objc_re = re.compile(r'marked @objc')
+
+"""Matches any non-none fix-it expectation."""
+fixit_re = re.compile(r'{{\d+-\d+=[^}]*}}')
+
+
+def adjust_comments(offset, comment_str):
+    """Replace expected-errors with expected-remarks, and make other adjustments
+       to diagnostics so that they reflect access notes."""
+
+    adjusted = expected_other_diag_re.sub(lambda m: u"expected-" + m.group(1) +
+                                                    offsetify(offset, m.group(2)),
+                                          comment_str)
+    adjusted = expected_error_re.sub(lambda m: u"expected-remark" +
+                                               offsetify(offset, m.group(1)),
+                                     adjusted)
+    adjusted = marked_objc_re.sub(u"marked @objc by an access note", adjusted)
+    adjusted = fixit_re.sub(u"{{none}}", adjusted)
+
+    return u"// [expectations adjusted] " + adjusted
+
+
+#
+# Writing attrs to access notes
+#
+
+def move_at_objc_to_access_note(access_notes_file, arg, offset, access_note_name):
+    """Write an @objc attribute into an access notes file, then return the
+       string that will replace the attribute and trailing comment."""
+
+    access_notes_file.write(u"""
+- Name: '{}'
+  ObjC: true""".format(access_note_name))
+    if arg:
+        access_notes_file.write(u"""
+  ObjCName: '{}'""".format(arg))
+
+    # Default to shifting expected diagnostics down 1 line.
+    if offset is None:
+        offset = 1
+
+    return u"// access-note-adjust" + offsetify(offset) + u" [attr moved] " + \
+           u"expected-remark{{access note for fancy tests adds attribute 'objc' to " + \
+           u"this }} expected-note{{add attribute explicitly to silence this warning}}"
+
+
+#
+# Matching lines
+#
+
+"""Matches '@objc(foo) // access-note-move{{access-note-name}}'
+   or '@objc // access-note-move{{access-note-name}}'"""
+access_note_move_re = re.compile(r'@objc(?:\(([\w:]+)\))?[ \t]*' +
+                                 r'//[ \t]*access-note-move' +
+                                 offset_re_fragment +
+                                 r'\{\{([^}]*)\}\}')
+
+"""Matches // access-note-adjust <comment>"""
+adjust_comment_re = re.compile(r'//[ \t]*access-note-adjust' + offset_re_fragment +
+                               r'(.*)')
+
+
+def replacer(fn, *args):
+    """Returns a lambda which calls fn with args, followed by the groups from
+       the match passed to the lambda."""
+
+    return lambda m: fn(*(args + m.groups()))
+
+
+main()