Merge pull request swiftlang#34394 from DougGregor/concurrency-objc-name-adjustments

[Concurrency] Don't lose name information from completion-handler arguments

Author: DougGregor

include/swift/Basic/StringExtras.h

@@ -442,7 +442,8 @@ class InheritedNameSet {
 ///
 /// \param allPropertyNames The set of property names in the enclosing context.
 ///
-/// \param isAsync Whether this is a function imported as 'async'.
+/// \param completionHandlerIndex For an 'async' function, the index of the
+/// completion handler in argNames.
 ///
 /// \param scratch Scratch space that will be used for modifications beyond
 /// just chopping names.
@@ -457,9 +458,13 @@ bool omitNeedlessWords(StringRef &baseName,
                        bool returnsSelf,
                        bool isProperty,
                        const InheritedNameSet *allPropertyNames,
-                       bool isAsync,
+                       Optional<unsigned> completionHandlerIndex,
+                       Optional<StringRef> completionHandlerName,
                        StringScratchSpace &scratch);
 
+/// If the name has a completion-handler suffix, strip off that suffix.
+Optional<StringRef> stripWithCompletionHandlerSuffix(StringRef name);
+
 } // end namespace swift
 
 #endif // SWIFT_BASIC_STRINGEXTRAS_H

lib/Basic/StringExtras.cpp

@@ -1220,7 +1220,8 @@ bool swift::omitNeedlessWords(StringRef &baseName,
                               bool returnsSelf,
                               bool isProperty,
                               const InheritedNameSet *allPropertyNames,
-                              bool isAsync,
+                              Optional<unsigned> completionHandlerIndex,
+                              Optional<StringRef> completionHandlerName,
                               StringScratchSpace &scratch) {
   bool anyChanges = false;
   OmissionTypeName resultType = returnsSelf ? contextType : givenResultType;
@@ -1290,6 +1291,7 @@ bool swift::omitNeedlessWords(StringRef &baseName,
 
   // If the base name of a method imported as "async" starts with the word
   // "get", drop the "get".
+  bool isAsync = completionHandlerIndex.hasValue();
   if (isAsync && camel_case::getFirstWord(baseName) == "get" &&
       baseName.size() > 3) {
     baseName = baseName.substr(3);
@@ -1301,6 +1303,15 @@ bool swift::omitNeedlessWords(StringRef &baseName,
       splitBaseName(baseName, argNames[0], paramTypes[0], firstParamName))
     anyChanges = true;
 
+  // If this is an asynchronous function where the completion handler is
+  // the first parameter, strip off WithCompletion(Handler) from the base name.
+  if (isAsync && *completionHandlerIndex == 0) {
+    if (auto newBaseName = stripWithCompletionHandlerSuffix(baseName)) {
+      baseName = *newBaseName;
+      anyChanges = true;
+    }
+  }
+
   // For a method imported as "async", drop the "Asynchronously" suffix from
   // the base name. It is redundant with 'async'.
   const StringRef asynchronously = "Asynchronously";
@@ -1310,6 +1321,21 @@ bool swift::omitNeedlessWords(StringRef &baseName,
     anyChanges = true;
   }
 
+  // If this is an asynchronous function where the completion handler is
+  // the second parameter, and the corresponding name has some additional
+  // information prior to WithCompletion(Handler), append that
+  // additional text to the base name.
+  if (isAsync && *completionHandlerIndex == 1 && completionHandlerName) {
+    if (auto extraParamText = stripWithCompletionHandlerSuffix(
+            *completionHandlerName)) {
+      SmallString<32> newBaseName;
+      newBaseName += baseName;
+      appendSentenceCase(newBaseName, *extraParamText);
+      baseName = scratch.copyString(newBaseName);
+      anyChanges = true;
+    }
+  }
+
   // Omit needless words based on parameter types.
   for (unsigned i = 0, n = argNames.size(); i != n; ++i) {
     // If there is no corresponding parameter, there is nothing to
@@ -1329,6 +1355,20 @@ bool swift::omitNeedlessWords(StringRef &baseName,
         name, paramTypes[i], role,
         role == NameRole::BaseName ? allPropertyNames : nullptr);
 
+    // If this is an asynchronous function where the completion handler is
+    // past the second parameter and has additional information in the name,
+    // add that information to the prior argument name.
+    if (isAsync && completionHandlerName && *completionHandlerIndex > 1 &&
+        *completionHandlerIndex == i + 1) {
+      if (auto extraParamText = stripWithCompletionHandlerSuffix(
+              *completionHandlerName)) {
+        SmallString<32> extendedName;
+        extendedName += newName;
+        appendSentenceCase(extendedName, *extraParamText);
+        newName = scratch.copyString(extendedName);
+      }
+    }
+
     if (name == newName) continue;
 
     // Record this change.
@@ -1342,3 +1382,15 @@ bool swift::omitNeedlessWords(StringRef &baseName,
 
   return lowercaseAcronymsForReturn();
 }
+
+Optional<StringRef> swift::stripWithCompletionHandlerSuffix(StringRef name) {
+  if (name.endswith("WithCompletionHandler")) {
+    return name.drop_back(strlen("WithCompletionHandler"));
+  }
+
+  if (name.endswith("WithCompletion")) {
+    return name.drop_back(strlen("WithCompletion"));
+  }
+
+  return None;
+}

lib/ClangImporter/IAMInference.cpp

@@ -448,7 +448,7 @@ class IAMInference {
     baseName = humbleBaseName.userFacingName();
     bool didOmit =
         omitNeedlessWords(baseName, argStrs, "", "", "", paramTypeNames, false,
-                          false, nullptr, false, scratch);
+                          false, nullptr, None, None, scratch);
     SmallVector<Identifier, 8> argLabels;
     for (auto str : argStrs)
       argLabels.push_back(getIdentifier(str));

lib/ClangImporter/ImportName.cpp

@@ -804,7 +804,8 @@ static bool omitNeedlessWordsInFunctionName(
     ArrayRef<const clang::ParmVarDecl *> params, clang::QualType resultType,
     const clang::DeclContext *dc, const SmallBitVector &nonNullArgs,
     Optional<unsigned> errorParamIndex, bool returnsSelf, bool isInstanceMethod,
-    Optional<unsigned> completionHandlerIndex, NameImporter &nameImporter) {
+    Optional<unsigned> completionHandlerIndex,
+    Optional<StringRef> completionHandlerName, NameImporter &nameImporter) {
   clang::ASTContext &clangCtx = nameImporter.getClangContext();
 
   // Collect the parameter type names.
@@ -854,8 +855,8 @@ static bool omitNeedlessWordsInFunctionName(
                            getClangTypeNameForOmission(clangCtx, resultType),
                            getClangTypeNameForOmission(clangCtx, contextType),
                            paramTypes, returnsSelf, /*isProperty=*/false,
-                           allPropertyNames, completionHandlerIndex.hasValue(),
-                           nameImporter.getScratch());
+                           allPropertyNames, completionHandlerIndex,
+                           completionHandlerName, nameImporter.getScratch());
 }
 
 /// Prepare global name for importing onto a swift_newtype.
@@ -1135,26 +1136,9 @@ Optional<ForeignErrorConvention::Info> NameImporter::considerErrorImport(
 /// Whether the given parameter name identifies a completion handler.
 static bool isCompletionHandlerParamName(StringRef paramName) {
   return paramName == "completionHandler" || paramName == "completion" ||
-      paramName == "withCompletionHandler";
+      paramName == "withCompletionHandler" || paramName == "withCompletion";
 }
 
-/// Whether the give base name implies that the first parameter is a completion
-/// handler.
-///
-/// \returns a trimmed base name when it does, \c None others
-static Optional<StringRef> isCompletionHandlerInBaseName(StringRef basename) {
-  if (basename.endswith("WithCompletionHandler")) {
-    return basename.drop_back(strlen("WithCompletionHandler"));
-  }
-
-  if (basename.endswith("WithCompletion")) {
-    return basename.drop_back(strlen("WithCompletion"));
-  }
-
-  return None;
-}
-
-
 // Determine whether the given type is a nullable NSError type.
 static bool isNullableNSErrorType(
     clang::ASTContext &clangCtx, clang::QualType type) {
@@ -1185,7 +1169,7 @@ static bool isNullableNSErrorType(
 Optional<ForeignAsyncConvention::Info>
 NameImporter::considerAsyncImport(
     const clang::ObjCMethodDecl *clangDecl,
-    StringRef &baseName,
+    StringRef baseName,
     SmallVectorImpl<StringRef> &paramNames,
     ArrayRef<const clang::ParmVarDecl *> params,
     bool isInitializer, bool hasCustomName,
@@ -1207,12 +1191,14 @@ NameImporter::considerAsyncImport(
 
   // Determine whether the naming indicates that this is a completion
   // handler.
-  Optional<StringRef> newBaseName;
   if (isCompletionHandlerParamName(
-          paramNames[completionHandlerParamNameIndex])) {
+          paramNames[completionHandlerParamNameIndex]) ||
+      (completionHandlerParamNameIndex > 0 &&
+       stripWithCompletionHandlerSuffix(
+           paramNames[completionHandlerParamNameIndex]))) {
     // The argument label itself has an appropriate name.
   } else if (!hasCustomName && completionHandlerParamIndex == 0 &&
-             (newBaseName = isCompletionHandlerInBaseName(baseName))) {
+             stripWithCompletionHandlerSuffix(baseName)) {
     // The base name implies that the first parameter is a completion handler.
   } else if (isCompletionHandlerParamName(
                  params[completionHandlerParamIndex]->getName())) {
@@ -1301,10 +1287,6 @@ NameImporter::considerAsyncImport(
   // Drop the completion handler parameter name.
   paramNames.erase(paramNames.begin() + completionHandlerParamNameIndex);
 
-  // Update the base name, if needed.
-  if (newBaseName && !hasCustomName)
-    baseName = *newBaseName;
-
   return ForeignAsyncConvention::Info(
       completionHandlerParamIndex, completionHandlerErrorParamIndex);
 }
@@ -1968,7 +1950,7 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
         (void)omitNeedlessWords(baseName, {}, "", propertyTypeName,
                                 contextTypeName, {}, /*returnsSelf=*/false,
                                 /*isProperty=*/true, allPropertyNames,
-                                /*isAsync=*/false, scratch);
+                                None, None, scratch);
       }
     }
 
@@ -1985,6 +1967,11 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
             [](const ForeignAsyncConvention::Info &info) {
               return info.CompletionHandlerParamIndex;
             }),
+          result.getAsyncInfo().map(
+            [&](const ForeignAsyncConvention::Info &info) {
+              return method->getDeclName().getObjCSelector().getNameForSlot(
+                  info.CompletionHandlerParamIndex);
+            }),
           *this);
     }
 

lib/ClangImporter/ImportName.h

@@ -455,7 +455,7 @@ class NameImporter {
 
   Optional<ForeignAsyncConvention::Info>
   considerAsyncImport(const clang::ObjCMethodDecl *clangDecl,
-                      StringRef &baseName,
+                      StringRef baseName,
                       SmallVectorImpl<StringRef> &paramNames,
                       ArrayRef<const clang::ParmVarDecl *> params,
                       bool isInitializer, bool hasCustomName,

lib/Sema/MiscDiagnostics.cpp

@@ -4868,7 +4868,7 @@ Optional<DeclName> TypeChecker::omitNeedlessWords(AbstractFunctionDecl *afd) {
                                 getTypeNameForOmission(contextType),
                                 paramTypes, returnsSelf, false,
                                 /*allPropertyNames=*/nullptr,
-                                afd->hasAsync(), scratch))
+                                None, None, scratch))
     return None;
 
   /// Retrieve a replacement identifier.
@@ -4923,8 +4923,7 @@ Optional<Identifier> TypeChecker::omitNeedlessWords(VarDecl *var) {
   OmissionTypeName contextTypeName = getTypeNameForOmission(contextType);
   if (::omitNeedlessWords(name, { }, "", typeName, contextTypeName, { },
                           /*returnsSelf=*/false, true,
-                          /*allPropertyNames=*/nullptr, /*isAsync=*/false,
-                          scratch)) {
+                          /*allPropertyNames=*/nullptr, None, None, scratch)) {
     return Context.getIdentifier(name);
   }
 

test/ClangImporter/objc_async.swift

@@ -22,6 +22,9 @@ func testSlowServer(slowServer: SlowServer) async throws {
 
   let _: String? = await try slowServer.fortune()
   let _: Int = await try slowServer.magicNumber(withSeed: 42)
+
+  await slowServer.serverRestart("localhost")
+  await slowServer.server("localhost", atPriorityRestart: 0.8)
 }
 
 func testSlowServerSynchronous(slowServer: SlowServer) {

test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h

@@ -16,6 +16,8 @@
 
 -(void)doSomethingConflicted:(NSString *)operation completionHandler:(void (^)(NSInteger))handler;
 -(NSInteger)doSomethingConflicted:(NSString *)operation;
+-(void)server:(NSString *)name restartWithCompletionHandler:(void (^)(void))block;
+-(void)server:(NSString *)name atPriority:(double)priority restartWithCompletionHandler:(void (^)(void))block;
 @end
 
 @protocol RefrigeratorDelegate<NSObject>