[APINotes] Add support for distinguishing class vs. instance properties.

DougGregor · DougGregor · commit 7bde131b05f1 · 2016-09-27T12:34:02.000-07:00
Fixes rdar://problem/28455756.
diff --git a/include/clang/APINotes/APINotesReader.h b/include/clang/APINotes/APINotesReader.h
@@ -74,10 +74,13 @@ class APINotesReader {
   ///
   /// \param contextID The ID that references the context we are looking for.
   /// \param name The name of the property we're looking for.
+  /// \param isInstance Whether we are looking for an instance property (vs.
+  /// a class property).
   ///
   /// \returns Information about the property, if known.
   Optional<ObjCPropertyInfo> lookupObjCProperty(ContextID contextID,
-                                                StringRef name);
+                                                StringRef name,
+                                                bool isInstance);
 
   /// Look for information regarding the given Objective-C method in
   /// the given context.
@@ -147,6 +150,7 @@ class APINotesReader {
 
     /// Visit an Objective-C property.
     virtual void visitObjCProperty(ContextID contextID, StringRef name,
+                                   bool isInstance,
                                    const ObjCPropertyInfo &info);
 
     /// Visit a global variable.
diff --git a/include/clang/APINotes/APINotesWriter.h b/include/clang/APINotes/APINotesWriter.h
@@ -66,6 +66,7 @@ class APINotesWriter {
   /// \param name The name of this property.
   /// \param info Information about this property.
   void addObjCProperty(ContextID contextID, StringRef name,
+                       bool isInstanceProperty,
                        const ObjCPropertyInfo &info);
 
   /// Add information about a specific Objective-C method.
diff --git a/lib/APINotes/APINotesFormat.h b/lib/APINotes/APINotesFormat.h
@@ -36,7 +36,7 @@ const uint16_t VERSION_MAJOR = 0;
 /// API notes file minor version number.
 ///
 /// When the format changes IN ANY WAY, this number should be incremented.
-const uint16_t VERSION_MINOR = 13;  // Function/method parameters
+const uint16_t VERSION_MINOR = 14;  // Objective-C class properties
 
 using IdentifierID = PointerEmbeddedInt<unsigned, 31>;
 using IdentifierIDField = BCVBR<16>;
@@ -291,4 +291,6 @@ namespace llvm {
   };
 }
 
+}
+
 #endif // LLVM_CLANG_API_NOTES_FORMAT_H
diff --git a/lib/APINotes/APINotesReader.cpp b/lib/APINotes/APINotesReader.cpp
@@ -175,8 +175,8 @@ namespace {
   /// Used to deserialize the on-disk Objective-C property table.
   class ObjCPropertyTableInfo {
   public:
-    // (context ID, name ID)
-    using internal_key_type = std::pair<unsigned, unsigned>; 
+    // (context ID, name ID, isInstance)
+    using internal_key_type = std::tuple<unsigned, unsigned, char>; 
     using external_key_type = internal_key_type;
     using data_type = ObjCPropertyInfo;
     using hash_value_type = size_t;
@@ -208,7 +208,8 @@ namespace {
     static internal_key_type ReadKey(const uint8_t *data, unsigned length) {
       auto classID = endian::readNext<uint32_t, little, unaligned>(data);
       auto nameID = endian::readNext<uint32_t, little, unaligned>(data);
-      return { classID, nameID };
+      char isInstance = endian::readNext<uint8_t, little, unaligned>(data);
+      return std::make_tuple(classID, nameID, isInstance);
     }
     
     static data_type ReadData(internal_key_type key, const uint8_t *data,
@@ -1510,15 +1511,18 @@ auto APINotesReader::lookupObjCProtocol(StringRef name)
 
 Optional<ObjCPropertyInfo> APINotesReader::lookupObjCProperty(
                              ContextID contextID,
-                             StringRef name) {
+                             StringRef name,
+                             bool isInstance) {
   if (!Impl.ObjCPropertyTable)
     return None;
 
   Optional<IdentifierID> propertyID = Impl.getIdentifier(name);
   if (!propertyID)
     return None;
 
-  auto known = Impl.ObjCPropertyTable->find({contextID.Value, *propertyID});
+  auto known = Impl.ObjCPropertyTable->find(std::make_tuple(contextID.Value,
+                                                            *propertyID,
+                                                            (char)isInstance));
   if (known == Impl.ObjCPropertyTable->end())
     return None;
 
@@ -1639,6 +1643,7 @@ void APINotesReader::Visitor::visitObjCMethod(ContextID contextID,
 
 void APINotesReader::Visitor::visitObjCProperty(ContextID contextID,
                                                 StringRef name,
+                                                bool isInstance,
                                                 const ObjCPropertyInfo &info) { }
 
 void APINotesReader::Visitor::visitGlobalVariable(
@@ -1723,10 +1728,11 @@ void APINotesReader::visit(Visitor &visitor) {
   // Visit properties.
   if (Impl.ObjCPropertyTable) {
     for (auto key : Impl.ObjCPropertyTable->keys()) {
-      ContextID contextID(key.first);
-      auto name = identifiers[key.second];
+      ContextID contextID(std::get<0>(key));
+      auto name = identifiers[std::get<1>(key)];
+      char isInstance = std::get<2>(key);
       auto info = *Impl.ObjCPropertyTable->find(key);
-      visitor.visitObjCProperty(contextID, name, info);
+      visitor.visitObjCProperty(contextID, name, isInstance, info);
     }
   }
 
diff --git a/lib/APINotes/APINotesWriter.cpp b/lib/APINotes/APINotesWriter.cpp
@@ -58,8 +58,9 @@ class APINotesWriter::Implementation {
 
   /// Information about Objective-C properties.
   ///
-  /// Indexed by the context ID and property name.
-  llvm::DenseMap<std::pair<unsigned, unsigned>, ObjCPropertyInfo> 
+  /// Indexed by the context ID, property name, and whether this is an
+  /// instance property.
+  llvm::DenseMap<std::tuple<unsigned, unsigned, char>, ObjCPropertyInfo> 
     ObjCProperties;
 
   /// Information about Objective-C methods.
@@ -430,7 +431,8 @@ namespace {
   /// Used to serialize the on-disk Objective-C property table.
   class ObjCPropertyTableInfo {
   public:
-    using key_type = std::pair<unsigned, unsigned>; // (class ID, name ID)
+    // (class ID, name ID, isInstance)
+    using key_type = std::tuple<unsigned, unsigned, char>;
     using key_type_ref = key_type;
     using data_type = ObjCPropertyInfo;
     using data_type_ref = const data_type &;
@@ -444,7 +446,7 @@ namespace {
     std::pair<unsigned, unsigned> EmitKeyDataLength(raw_ostream &out,
                                                     key_type_ref key,
                                                     data_type_ref data) {
-      uint32_t keyLength = sizeof(uint32_t) + sizeof(uint32_t);
+      uint32_t keyLength = sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint8_t);
       uint32_t dataLength = getVariableInfoSize(data);
       endian::Writer<little> writer(out);
       writer.write<uint16_t>(keyLength);
@@ -454,8 +456,9 @@ namespace {
 
     void EmitKey(raw_ostream &out, key_type_ref key, unsigned len) {
       endian::Writer<little> writer(out);
-      writer.write<uint32_t>(key.first);
-      writer.write<uint32_t>(key.second);
+      writer.write<uint32_t>(std::get<0>(key));
+      writer.write<uint32_t>(std::get<1>(key));
+      writer.write<uint8_t>(std::get<2>(key));
     }
 
     void EmitData(raw_ostream &out, key_type_ref key, data_type_ref data,
@@ -1060,10 +1063,11 @@ ContextID APINotesWriter::addObjCProtocol(StringRef name,
   return ContextID(known->second.first);
 }
 void APINotesWriter::addObjCProperty(ContextID contextID, StringRef name,
+                                     bool isInstance,
                                      const ObjCPropertyInfo &info) {
   IdentifierID nameID = Impl.getIdentifier(name);
-  assert(!Impl.ObjCProperties.count({contextID.Value, nameID}));
-  Impl.ObjCProperties[{contextID.Value, nameID}] = info;
+  assert(!Impl.ObjCProperties.count({contextID.Value, nameID, isInstance}));
+  Impl.ObjCProperties[{contextID.Value, nameID, isInstance}] = info;
 }
 
 void APINotesWriter::addObjCMethod(ContextID contextID,
diff --git a/lib/APINotes/APINotesYAMLCompiler.cpp b/lib/APINotes/APINotesYAMLCompiler.cpp
@@ -193,6 +193,7 @@ namespace {
 
   struct Property {
     StringRef Name;
+    llvm::Optional<MethodKind> Kind;
     llvm::Optional<NullabilityKind> Nullability;
     AvailabilityItem Availability;
     bool SwiftPrivate = false;
@@ -348,6 +349,7 @@ namespace llvm {
     struct MappingTraits<Property> {
       static void mapping(IO &io, Property& p) {
         io.mapRequired("Name",            p.Name);
+        io.mapOptional("PropertyKind",    p.Kind);
         io.mapOptional("Nullability",     p.Nullability, 
                                           AbsentNullability);
         io.mapOptional("Availability",    p.Availability.Mode);
@@ -693,11 +695,20 @@ namespace {
       }
 
       // Write all properties.
-      llvm::StringSet<> knownProperties;
+      llvm::StringSet<> knownInstanceProperties;
+      llvm::StringSet<> knownClassProperties;
       for (const auto &prop : cl.Properties) {
         // Check for duplicate property definitions.
-        if (!knownProperties.insert(prop.Name).second) {
-          emitError("duplicate definition of property '" + cl.Name + "." +
+        if ((!prop.Kind || *prop.Kind == MethodKind::Instance) &&
+            !knownInstanceProperties.insert(prop.Name).second) {
+          emitError("duplicate definition of instance property '" + cl.Name +
+                    "." + prop.Name + "'");
+          continue;
+        }
+
+        if ((!prop.Kind || *prop.Kind == MethodKind::Class) &&
+            !knownClassProperties.insert(prop.Name).second) {
+          emitError("duplicate definition of class property '" + cl.Name + "." +
                     prop.Name + "'");
           continue;
         }
@@ -711,7 +722,14 @@ namespace {
         pInfo.SwiftName = prop.SwiftName;
         if (prop.Nullability)
           pInfo.setNullabilityAudited(*prop.Nullability);
-        Writer->addObjCProperty(clID, prop.Name, pInfo);
+        if (prop.Kind) {
+          Writer->addObjCProperty(clID, prop.Name,
+                                  *prop.Kind == MethodKind::Instance, pInfo);
+        } else {
+          // Add both instance and class properties with this name.
+          Writer->addObjCProperty(clID, prop.Name, true, pInfo);
+          Writer->addObjCProperty(clID, prop.Name, false, pInfo);
+        }
       }
     }
 
@@ -1037,9 +1055,11 @@ namespace {
     }
 
     virtual void visitObjCProperty(ContextID contextID, StringRef name,
+                                   bool isInstance,
                                    const ObjCPropertyInfo &info) {
       Property property;
       property.Name = name;
+      property.Kind = isInstance ? MethodKind::Instance : MethodKind::Class;
       handleCommon(property, info);
 
       // FIXME: No way to represent "not audited for nullability".
@@ -1109,6 +1129,11 @@ namespace {
   };
 }
 
+/// Produce a flattened, numeric value for optional method/property kinds.
+static unsigned flattenPropertyKind(llvm::Optional<MethodKind> kind) {
+  return kind ? (*kind == MethodKind::Instance ? 2 : 1) : 0;
+}
+
 bool api_notes::decompileAPINotes(std::unique_ptr<llvm::MemoryBuffer> input,
                                   llvm::raw_ostream &os) {
   // Try to read the file.
@@ -1150,7 +1175,10 @@ bool api_notes::decompileAPINotes(std::unique_ptr<llvm::MemoryBuffer> input,
     // Sort properties.
     std::sort(record.Properties.begin(), record.Properties.end(),
               [](const Property &lhs, const Property &rhs) -> bool {
-                return lhs.Name < rhs.Name;
+                return lhs.Name < rhs.Name ||
+                  (lhs.Name == rhs.Name &&
+                   flattenPropertyKind(lhs.Kind) < 
+                     flattenPropertyKind(rhs.Kind));
               });
 
     // Sort methods.
diff --git a/lib/Sema/SemaAPINotes.cpp b/lib/Sema/SemaAPINotes.cpp
@@ -491,8 +491,12 @@ void Sema::ProcessAPINotes(Decl *D) {
       if (api_notes::APINotesReader *Reader
             = APINotes.findAPINotes(D->getLocation())) {
         if (auto Context = GetContext(Reader)) {
+          bool isInstanceProperty =
+            (Property->getPropertyAttributesAsWritten() &
+               ObjCPropertyDecl::OBJC_PR_class) == 0;
           if (auto Info = Reader->lookupObjCProperty(*Context,
-                                                     Property->getName())) {
+                                                     Property->getName(),
+                                                     isInstanceProperty)) {
             ::ProcessAPINotes(*this, Property, *Info);
           }
         }
diff --git a/lib/Sema/SemaObjCProperty.cpp b/lib/Sema/SemaObjCProperty.cpp
@@ -636,15 +636,15 @@ ObjCPropertyDecl *Sema::CreatePropertyDecl(Scope *S,
     PDecl->setInvalidDecl();
   }
 
-  ProcessDeclAttributes(S, PDecl, FD.D);
-
   // Regardless of setter/getter attribute, we save the default getter/setter
   // selector names in anticipation of declaration of setter/getter methods.
   PDecl->setGetterName(GetterSel);
   PDecl->setSetterName(SetterSel);
   PDecl->setPropertyAttributesAsWritten(
                           makePropertyAttributesAsWritten(AttributesAsWritten));
 
+  ProcessDeclAttributes(S, PDecl, FD.D);
+
   if (Attributes & ObjCDeclSpec::DQ_PR_readonly)
     PDecl->setPropertyAttributes(ObjCPropertyDecl::OBJC_PR_readonly);
 
diff --git a/test/APINotes/Inputs/APINotes/SomeKit.apinotes b/test/APINotes/Inputs/APINotes/SomeKit.apinotes
@@ -20,6 +20,14 @@ Classes:
         AvailabilityMsg: "wouldn't work anyway"
       - Name: internalProperty
         Nullability: N
+      - Name: nonnullAInstance
+        PropertyKind:    Instance
+        Nullability:     N
+      - Name: nonnullAClass
+        PropertyKind:    Class
+        Nullability:     N
+      - Name: nonnullABoth
+        Nullability:     N
   - Name: B
     Availability: none
     AvailabilityMsg: "just don't"
diff --git a/test/APINotes/Inputs/Frameworks/SomeKit.framework/APINotes/SomeKit.apinotes b/test/APINotes/Inputs/Frameworks/SomeKit.framework/APINotes/SomeKit.apinotes
@@ -12,8 +12,17 @@ Classes:
         Nullability:      [ N, S ]
     Properties:
       - Name: intValue
+        PropertyKind:    Instance
         Availability: none
         AvailabilityMsg: "wouldn't work anyway"
+      - Name: nonnullAInstance
+        PropertyKind:    Instance
+        Nullability:     N
+      - Name: nonnullAClass
+        PropertyKind:    Class
+        Nullability:     N
+      - Name: nonnullABoth
+        Nullability:     N
   - Name: B
     Availability: none
     AvailabilityMsg: "just don't"
diff --git a/test/APINotes/Inputs/Frameworks/SomeKit.framework/Headers/SomeKit.h b/test/APINotes/Inputs/Frameworks/SomeKit.framework/Headers/SomeKit.h
@@ -24,4 +24,15 @@ __attribute__((objc_root_class))
 +(instancetype)processInfo;
 @end
 
+@interface A(NonNullProperties)
+@property (nonatomic, readwrite, retain) A *nonnullAInstance;
+@property (class, nonatomic, readwrite, retain) A *nonnullAInstance;
+
+@property (nonatomic, readwrite, retain) A *nonnullAClass;
+@property (class, nonatomic, readwrite, retain) A *nonnullAClass;
+
+@property (nonatomic, readwrite, retain) A *nonnullABoth;
+@property (class, nonatomic, readwrite, retain) A *nonnullABoth;
+@end
+
 #endif
diff --git a/test/APINotes/Inputs/roundtrip.apinotes b/test/APINotes/Inputs/roundtrip.apinotes
@@ -90,12 +90,14 @@ Classes:
         SwiftName:       'beginDragginSession(_:event:source:)'
     Properties:      
       - Name:            enclosingScrollView
+        PropertyKind:    Instance
         Nullability:     O
         Availability:    available
         AvailabilityMsg: ''
         SwiftPrivate:    false
         SwiftName:       enclosing
       - Name:            makeBackingLayer
+        PropertyKind:    Class
         Nullability:     N
         Availability:    available
         AvailabilityMsg: ''
diff --git a/test/APINotes/nullability.m b/test/APINotes/nullability.m
@@ -3,12 +3,20 @@
 
 #import <SomeKit/SomeKit.h>
 
-
 int main() {
   A *a;
 
   [a transform: 0 integer: 0]; // expected-warning{{null passed to a callee that requires a non-null argument}}
 
+  [a setNonnullAInstance: 0]; // expected-warning{{null passed to a callee that requires a non-null argument}}
+  [A setNonnullAInstance: 0]; // no warning
+  
+  [a setNonnullAClass: 0]; // no warning
+  [A setNonnullAClass: 0]; // expected-warning{{null passed to a callee that requires a non-null argument}}
+
+  [a setNonnullABoth: 0]; // expected-warning{{null passed to a callee that requires a non-null argument}}
+  [A setNonnullABoth: 0]; // expected-warning{{null passed to a callee that requires a non-null argument}}
+
   return 0;
 }
 
diff --git a/test/APINotes/yaml-reader-errors.c b/test/APINotes/yaml-reader-errors.c
@@ -34,7 +34,7 @@ AvailabilityMsg: iOSOnly
         Nullability:     N
         Availability:    iOS
         AvailabilityMsg: iOSOnly
-# CHECK: duplicate definition of property 'UIFont.familyName'
+# CHECK: duplicate definition of instance property 'UIFont.familyName'
       - Name:            familyName
         Nullability:     N
         Availability:    iOS

Original file line number	Diff line number	Diff line change
`@@ -491,8 +491,12 @@ void Sema::ProcessAPINotes(Decl *D) {`
`491`	`491`	`if (api_notes::APINotesReader *Reader`
`492`	`492`	`= APINotes.findAPINotes(D->getLocation())) {`
`493`	`493`	`if (auto Context = GetContext(Reader)) {`
	`494`	`+ bool isInstanceProperty =`
	`495`	`+ (Property->getPropertyAttributesAsWritten() &`
	`496`	`+ ObjCPropertyDecl::OBJC_PR_class) == 0;`
`494`	`497`	`if (auto Info = Reader->lookupObjCProperty(*Context,`
`495`		`- Property->getName())) {`
	`498`	`+ Property->getName(),`
	`499`	`+ isInstanceProperty)) {`
`496`	`500`	`::ProcessAPINotes(this, Property, Info);`
`497`	`501`	`}`
`498`	`502`	`}`