From 16104d8045d55c5b6ac2381764a07d91402f80f3 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 22 Oct 2020 13:01:23 -0700 Subject: [PATCH] [Concurrency] Don't lose name information from completion-handler arguments. When a completion handler parameter has a selector piece that ends with "WithCompletion(Handler)", prepend the text before that suffix to the base name or previous argument label, as appropriate. This ensures that we don't lose information from the name, particularly with delegate names. --- include/swift/Basic/StringExtras.h | 9 +++- lib/Basic/StringExtras.cpp | 54 ++++++++++++++++++- lib/ClangImporter/IAMInference.cpp | 2 +- lib/ClangImporter/ImportName.cpp | 47 ++++++---------- lib/ClangImporter/ImportName.h | 2 +- lib/Sema/MiscDiagnostics.cpp | 5 +- test/ClangImporter/objc_async.swift | 3 ++ .../usr/include/ObjCConcurrency.h | 2 + 8 files changed, 86 insertions(+), 38 deletions(-) diff --git a/include/swift/Basic/StringExtras.h b/include/swift/Basic/StringExtras.h index 4bc4fe7a8a9..4e837605d2c 100644 --- a/include/swift/Basic/StringExtras.h +++ b/include/swift/Basic/StringExtras.h @@ -442,7 +442,8 @@ public: /// /// \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 completionHandlerIndex, + Optional completionHandlerName, StringScratchSpace &scratch); +/// If the name has a completion-handler suffix, strip off that suffix. +Optional stripWithCompletionHandlerSuffix(StringRef name); + } // end namespace swift #endif // SWIFT_BASIC_STRINGEXTRAS_H diff --git a/lib/Basic/StringExtras.cpp b/lib/Basic/StringExtras.cpp index 51f8970dffc..3884a8d72de 100644 --- a/lib/Basic/StringExtras.cpp +++ b/lib/Basic/StringExtras.cpp @@ -1220,7 +1220,8 @@ bool swift::omitNeedlessWords(StringRef &baseName, bool returnsSelf, bool isProperty, const InheritedNameSet *allPropertyNames, - bool isAsync, + Optional completionHandlerIndex, + Optional 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 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; +} diff --git a/lib/ClangImporter/IAMInference.cpp b/lib/ClangImporter/IAMInference.cpp index 6cbf5ce8781..cdd88dbb60c 100644 --- a/lib/ClangImporter/IAMInference.cpp +++ b/lib/ClangImporter/IAMInference.cpp @@ -448,7 +448,7 @@ private: baseName = humbleBaseName.userFacingName(); bool didOmit = omitNeedlessWords(baseName, argStrs, "", "", "", paramTypeNames, false, - false, nullptr, false, scratch); + false, nullptr, None, None, scratch); SmallVector argLabels; for (auto str : argStrs) argLabels.push_back(getIdentifier(str)); diff --git a/lib/ClangImporter/ImportName.cpp b/lib/ClangImporter/ImportName.cpp index 078d11792dd..62ea21f89c3 100644 --- a/lib/ClangImporter/ImportName.cpp +++ b/lib/ClangImporter/ImportName.cpp @@ -804,7 +804,8 @@ static bool omitNeedlessWordsInFunctionName( ArrayRef params, clang::QualType resultType, const clang::DeclContext *dc, const SmallBitVector &nonNullArgs, Optional errorParamIndex, bool returnsSelf, bool isInstanceMethod, - Optional completionHandlerIndex, NameImporter &nameImporter) { + Optional completionHandlerIndex, + Optional 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 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 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 NameImporter::considerAsyncImport( const clang::ObjCMethodDecl *clangDecl, - StringRef &baseName, + StringRef baseName, SmallVectorImpl ¶mNames, ArrayRef params, bool isInitializer, bool hasCustomName, @@ -1207,12 +1191,14 @@ NameImporter::considerAsyncImport( // Determine whether the naming indicates that this is a completion // handler. - Optional 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); } @@ -1954,7 +1936,7 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D, (void)omitNeedlessWords(baseName, {}, "", propertyTypeName, contextTypeName, {}, /*returnsSelf=*/false, /*isProperty=*/true, allPropertyNames, - /*isAsync=*/false, scratch); + None, None, scratch); } } @@ -1971,6 +1953,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); } diff --git a/lib/ClangImporter/ImportName.h b/lib/ClangImporter/ImportName.h index 5f5d9a02734..a35fb6760e6 100644 --- a/lib/ClangImporter/ImportName.h +++ b/lib/ClangImporter/ImportName.h @@ -455,7 +455,7 @@ private: Optional considerAsyncImport(const clang::ObjCMethodDecl *clangDecl, - StringRef &baseName, + StringRef baseName, SmallVectorImpl ¶mNames, ArrayRef params, bool isInitializer, bool hasCustomName, diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index deb27a20ba1..ac3930d9384 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -4868,7 +4868,7 @@ Optional 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 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); } diff --git a/test/ClangImporter/objc_async.swift b/test/ClangImporter/objc_async.swift index 8d2e691d498..8d80d1a0b6c 100644 --- a/test/ClangImporter/objc_async.swift +++ b/test/ClangImporter/objc_async.swift @@ -21,6 +21,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) { diff --git a/test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h b/test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h index b968a5561e7..01a62d25ea5 100644 --- a/test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h +++ b/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