Omit needless words: don't prune "properties" of the context from the base name.

DougGregor · DougGregor · commit 106bf8015220 · 2015-11-16T15:27:38.000-08:00
The properties of a context indicate those things that are considered
"contained within" the context (among other things). This helps us
avoid producing overly-generic names when we identify a redundancy in
the base name. For example, NSView contains the following:

  var gestureRecognizers: [NSGestureRecognizer]
  func addGestureRecognizer(gestureRecognizer: NSGestureRecognizer)
  func removeGestureRecognizer(gestureRecognizer: NSGestureRecognizer)

Normally, omit-needless-words would prune the two method names down to
"add" and "remove", respectively, because they restate type
information. However, this pruning is not ideal, because a view isn't
primarily a collection of gesture recognizers.

Use the presence of the property "gestureRecognizers" to indicate that
we should not strip "gestureRecognizer" or "gestureRecognizers" from
the base names of methods within that class (or its subclasses).

Note that there is more work to do here to properly deal with API
evolution: a newly-added property shouldn't have any effect on
existing APIs. We should use availability information here, and only
consider properties introduced no later than the entity under
consideration.
diff --git a/include/swift/AST/ASTContext.h b/include/swift/AST/ASTContext.h
@@ -44,6 +44,7 @@
 namespace clang {
   class Decl;
   class MacroInfo;
+  class ObjCInterfaceDecl;
 }
 
 namespace swift {
@@ -65,6 +66,7 @@ namespace swift {
   class FunctionType;
   class ArchetypeType;
   class Identifier;
+  class InheritedNameSet;
   class ModuleDecl;
   class ModuleLoader;
   class NominalTypeDecl;
@@ -798,6 +800,14 @@ class ASTContext {
   void setArchetypeBuilder(CanGenericSignature sig,
                            ModuleDecl *mod,
                            std::unique_ptr<ArchetypeBuilder> builder);
+
+  /// Retrieve the inherited name set for the given class.
+  const InheritedNameSet *getAllPropertyNames(ClassDecl *classDecl);
+
+  /// Retrieve the inherited name set for the given Objective-C class.
+  const InheritedNameSet *getAllPropertyNames(
+                            clang::ObjCInterfaceDecl *classDecl);
+
 private:
   friend class Decl;
   Optional<RawComment> getRawComment(const Decl *D);
diff --git a/include/swift/Basic/StringExtras.h b/include/swift/Basic/StringExtras.h
@@ -22,6 +22,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Allocator.h"
 #include <iterator>
 #include <string>
@@ -365,6 +366,22 @@ class StringScratchSpace {
   StringRef copyString(StringRef string);
 };
 
+/// Describes a set of names with an inheritance relationship.
+class InheritedNameSet {
+  const InheritedNameSet *Parent;
+  llvm::StringSet<> Names;
+
+public:
+  /// Construct a new inherited name set with the given parent.
+  explicit InheritedNameSet(const InheritedNameSet *parent) : Parent(parent) { }
+
+  // Add a new name to the set.
+  void add(StringRef name);
+
+  /// Determine whether this set includes the given name.
+  bool contains(StringRef name) const;
+};
+
 /// Omit needless words for a declaration.
 ///
 /// \param baseName The base name of the declaration. This value may be
@@ -388,6 +405,8 @@ class StringScratchSpace {
 ///
 /// \param isProperty Whether this is the name of a property.
 ///
+/// \param allPropertyNames The set of property names in the enclosing context.
+///
 /// \param scratch Scratch space that will be used for modifications beyond
 /// just chopping names.
 ///
@@ -400,6 +419,7 @@ bool omitNeedlessWords(StringRef &baseName,
                        ArrayRef<OmissionTypeName> paramTypes,
                        bool returnsSelf,
                        bool isProperty,
+                       const InheritedNameSet *allPropertyNames,
                        StringScratchSpace &scratch);
 }
 
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -30,6 +30,8 @@
 #include "swift/AST/RawComment.h"
 #include "swift/AST/TypeCheckerDebugConsumer.h"
 #include "swift/Basic/SourceManager.h"
+#include "swift/Basic/StringExtras.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/Support/Allocator.h"
@@ -232,6 +234,16 @@ struct ASTContext::Implementation {
   llvm::DenseMap<std::pair<GenericSignature *, ModuleDecl *>,
                  std::unique_ptr<ArchetypeBuilder>> ArchetypeBuilders;
 
+  /// The set of property names that show up in the defining module of a
+  /// class.
+  llvm::DenseMap<const ClassDecl *, std::unique_ptr<InheritedNameSet>>
+    AllProperties;
+
+  /// The set of property names that show up in the defining module of
+  /// an Objective-C class.
+  llvm::DenseMap<const clang::ObjCInterfaceDecl *,
+                 std::unique_ptr<InheritedNameSet>> AllPropertiesObjC;
+
   /// \brief Structure that captures data that is segregated into different
   /// arenas.
   struct Arena {
@@ -3501,3 +3513,116 @@ void ASTContext::unregisterLazyArchetype(const ArchetypeType *archetype) {
   assert(known != Impl.LazyArchetypes.end());
   Impl.LazyArchetypes.erase(known);
 }
+
+const InheritedNameSet *ASTContext::getAllPropertyNames(ClassDecl *classDecl) {
+  // If this class was defined in Objective-C, perform the lookup based on
+  // the Objective-C class.
+  if (auto objcClass = dyn_cast_or_null<clang::ObjCInterfaceDecl>(
+                         classDecl->getClangDecl())) {
+    return getAllPropertyNames(
+             const_cast<clang::ObjCInterfaceDecl *>(objcClass));
+  }
+
+  // If we already have this information, return it.
+  auto known = Impl.AllProperties.find(classDecl);
+  if (known != Impl.AllProperties.end()) return known->second.get();
+
+  // Otherwise, get information from our superclass first.
+  if (auto resolver = getLazyResolver())
+    resolver->resolveSuperclass(classDecl);
+
+  const InheritedNameSet *parentSet = nullptr;
+  if (auto superclass = classDecl->getSuperclass()) {
+    if (auto superclassDecl = superclass->getClassOrBoundGenericClass()) {
+      parentSet = getAllPropertyNames(superclassDecl);
+    }
+  }
+
+  // Create the set of properties.
+  known = Impl.AllProperties.insert(
+            {classDecl, llvm::make_unique<InheritedNameSet>(parentSet) }).first;
+
+  // Local function to add properties from the given set.
+  auto addProperties = [&](DeclRange members) {
+    for (auto member : members) {
+      auto var = dyn_cast<VarDecl>(member);
+      if (!var || var->getName().empty()) continue;
+
+      known->second->add(var->getName().str());
+    }
+  };
+
+  // Collect property names from the class.
+  addProperties(classDecl->getMembers());
+
+  // Collect property names from all extensions in the same module as the class.
+  auto module = classDecl->getParentModule();
+  for (auto ext : classDecl->getExtensions()) {
+    if (ext->getParentModule() != module) continue;
+    addProperties(ext->getMembers());
+  }
+
+  return known->second.get();
+}
+
+const InheritedNameSet *ASTContext::getAllPropertyNames(
+                          clang::ObjCInterfaceDecl *classDecl) {
+  classDecl = classDecl->getCanonicalDecl();
+
+  // If we already have this information, return it.
+  auto known = Impl.AllPropertiesObjC.find(classDecl);
+  if (known != Impl.AllPropertiesObjC.end()) return known->second.get();
+
+  // Otherwise, get information from our superclass first.
+  const InheritedNameSet *parentSet = nullptr;
+  if (auto superclassDecl = classDecl->getSuperClass()) {
+    parentSet = getAllPropertyNames(superclassDecl);
+  }
+
+  // Create the set of properties.
+  known = Impl.AllPropertiesObjC.insert(
+            {classDecl, llvm::make_unique<InheritedNameSet>(parentSet) }).first;
+
+  // Local function to add properties from the given set.
+  auto addProperties = [&](clang::DeclContext::decl_range members) {
+    for (auto member : members) {
+      // Add Objective-C property names.
+      if (auto property = dyn_cast<clang::ObjCPropertyDecl>(member)) {
+        known->second->add(property->getName());
+        continue;
+      }
+
+      // Add no-parameter, non-void method names.
+      if (auto method = dyn_cast<clang::ObjCMethodDecl>(member)) {
+        if (method->getSelector().isUnarySelector() &&
+            !method->getReturnType()->isVoidType()) {
+          known->second->add(method->getSelector().getNameForSlot(0));
+          continue;
+        }
+      }
+    }
+  };
+
+  // Dig out the class definition.
+  auto classDef = classDecl->getDefinition();
+  if (!classDef) return known->second.get();
+
+  // Collect property names from the class definition.
+  addProperties(classDef->decls());
+
+  // Dig out the module that owns the class definition.
+  auto module = classDef->getImportedOwningModule();
+  if (module) module = module->getTopLevelModule();
+
+  // Collect property names from all categories and extensions in the same
+  // module as the class.
+  for (auto category : classDef->known_categories()) {
+    auto categoryModule = category->getImportedOwningModule();
+    if (categoryModule) categoryModule = categoryModule->getTopLevelModule();
+    if (module != categoryModule) continue;
+
+    addProperties(category->decls());
+  }
+
+  return known->second.get();
+}
diff --git a/lib/Basic/StringExtras.cpp b/lib/Basic/StringExtras.cpp
@@ -421,6 +421,20 @@ StringRef StringScratchSpace::copyString(StringRef string) {
   return StringRef(static_cast<char *>(memory), string.size());
 }
 
+void InheritedNameSet::add(StringRef name) {
+  Names.insert(name);
+}
+
+bool InheritedNameSet::contains(StringRef name) const {
+  auto set = this;
+  do {
+    if (set->Names.count(name) > 0) return true;
+    set = set->Parent;
+  } while (set);
+
+  return false;
+}
+
 /// Wrapper for camel_case::toLowercaseWord that uses string scratch space.
 static StringRef toLowercaseWord(StringRef string, StringScratchSpace &scratch){
   llvm::SmallString<32> scratchStr;
@@ -477,6 +491,7 @@ static bool isVacuousName(StringRef name) {
 static StringRef omitNeedlessWords(StringRef name,
                                    OmissionTypeName typeName,
                                    NameRole role,
+                                   const InheritedNameSet *allPropertyNames,
                                    StringScratchSpace &scratch) {
   // If we have no name or no type name, there is nothing to do.
   if (name.empty() || typeName.empty()) return name;
@@ -540,7 +555,7 @@ static StringRef omitNeedlessWords(StringRef name,
         = name.substr(0, nameWordRevIter.base().getPosition()-1);
       auto newShortenedNameWord
         = omitNeedlessWords(shortenedNameWord, typeName.CollectionElement,
-                            NameRole::Partial, scratch);
+                            NameRole::Partial, allPropertyNames, scratch);
       if (shortenedNameWord != newShortenedNameWord) {
         anyMatches = true;
         unsigned targetSize = newShortenedNameWord.size();
@@ -604,6 +619,38 @@ static StringRef omitNeedlessWords(StringRef name,
 
       case PartOfSpeech::Verb:
       case PartOfSpeech::Gerund:
+        // Don't prune redundant type information from the base name if
+        // there is a corresponding property (either singular or plural).
+        if (allPropertyNames && role == NameRole::BaseName) {
+          SmallString<16> localScratch;
+          auto removedText = name.substr(nameWordRevIter.base().getPosition());
+          auto removedName = camel_case::toLowercaseWord(removedText,
+                                                         localScratch);
+
+          // A property with exactly this name.
+          if (allPropertyNames->contains(removedName)) return name;
+
+          // From here on, we'll be working with scratch space.
+          if (removedName.data() != localScratch.data())
+            localScratch = removedName;
+
+          if (localScratch.back() == 'y') {
+            // If the last letter is a 'y', try 'ies'.
+            localScratch.pop_back();
+            localScratch += "ies";
+            if (allPropertyNames->contains(localScratch)) return name;
+          } else {
+            // Otherwise, add an 's' and try again.
+            localScratch += 's';
+            if (allPropertyNames->contains(localScratch)) return name;
+
+            // Alternatively, try to add 'es'.
+            localScratch.pop_back();
+            localScratch += "es";
+            if (allPropertyNames->contains(localScratch)) return name;
+          }
+        }
+
         // Strip off the part of the name that is redundant with
         // type information.
         name = name.substr(0, nameWordRevIter.base().getPosition());
@@ -693,6 +740,7 @@ bool swift::omitNeedlessWords(StringRef &baseName,
                               ArrayRef<OmissionTypeName> paramTypes,
                               bool returnsSelf,
                               bool isProperty,
+                              const InheritedNameSet *allPropertyNames,
                               StringScratchSpace &scratch) {
   bool anyChanges = false;
 
@@ -735,6 +783,7 @@ bool swift::omitNeedlessWords(StringRef &baseName,
                                 baseName,
                                 returnsSelf ? contextType : resultType,
                                 NameRole::Property,
+                                allPropertyNames,
                                 scratch);
       if (newBaseName != baseName) {
         baseName = newBaseName;
@@ -768,7 +817,11 @@ bool swift::omitNeedlessWords(StringRef &baseName,
 
     // Omit needless words from the name.
     StringRef name = role == NameRole::BaseName ? baseName : argNames[i];
-    StringRef newName = ::omitNeedlessWords(name, paramTypes[i], role, scratch);
+    StringRef newName = ::omitNeedlessWords(name, paramTypes[i], role,
+                                            role == NameRole::BaseName 
+                                              ? allPropertyNames
+                                              : nullptr,
+                                            scratch);
 
     // Did the name change?
     if (name != newName)
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -1365,9 +1365,19 @@ ClangImporter::Implementation::importName(const clang::NamedDecl *D,
                                   objcProperty->getType());
         StringScratchSpace scratch;
         StringRef name = result.str();
+
+        // Find the property names.
+        const InheritedNameSet *allPropertyNames = nullptr;
+        if (!contextType.isNull()) {
+          if (auto objcPtrType = contextType->getAsObjCInterfacePointerType())
+            if (auto objcClassDecl = objcPtrType->getInterfaceDecl())
+              allPropertyNames = SwiftContext.getAllPropertyNames(
+                                   objcClassDecl);
+        }
+
         if (omitNeedlessWords(name, { }, "", propertyTypeName, contextTypeName,
                               { }, /*returnsSelf=*/false, /*isProperty=*/true,
-                              scratch)) {
+                              allPropertyNames, scratch)) {
           result = SwiftContext.getIdentifier(name);
         }
       }
diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp
@@ -2088,12 +2088,21 @@ DeclName ClangImporter::Implementation::omitNeedlessWordsInFunctionName(
   // Omit needless words.
   StringRef baseName = name.getBaseName().str();
   StringScratchSpace scratch;
+
+  // Find the property names.
+  const InheritedNameSet *allPropertyNames = nullptr;
+  auto contextType = getClangDeclContextType(dc);
+  if (!contextType.isNull()) {
+    if (auto objcPtrType = contextType->getAsObjCInterfacePointerType())
+      if (auto objcClassDecl = objcPtrType->getInterfaceDecl())
+        allPropertyNames = SwiftContext.getAllPropertyNames(objcClassDecl);
+  }
+
   if (!omitNeedlessWords(baseName, argNames, firstParamName,
                          getClangTypeNameForOmission(resultType),
-                         getClangTypeNameForOmission(
-                           getClangDeclContextType(dc)),
+                         getClangTypeNameForOmission(contextType),
                          paramTypes, returnsSelf, /*isProperty=*/false,
-                         scratch))
+                         allPropertyNames, scratch))
     return name;
 
   /// Retrieve a replacement identifier.
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
diff --git a/test/IDE/print_omit_needless_words.swift b/test/IDE/print_omit_needless_words.swift
diff --git a/test/Inputs/clang-importer-sdk/usr/include/AppKit.h b/test/Inputs/clang-importer-sdk/usr/include/AppKit.h
