Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 22, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions include/swift/Basic/StringExtras.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
54 changes: 53 additions & 1 deletion lib/Basic/StringExtras.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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";
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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;
}
2 changes: 1 addition & 1 deletion lib/ClangImporter/IAMInference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
47 changes: 17 additions & 30 deletions lib/ClangImporter/ImportName.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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())) {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/ClangImporter/ImportName.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 2 additions & 3 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

Expand Down
3 changes: 3 additions & 0 deletions test/ClangImporter/objc_async.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I'm not sure what real-life APIs you're looking at, but at least rationalizing based on this example, it may make the most sense to translate these APIs by prepending the information to the base name at all times: await is really taking the place of the completion handler, and it lives on the opposite side of the expression as compared to a trailing closure.

   server:atPriority:restartWithCompletionHandler:
//                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trailing closures are ergonomic

   await server.server("localhost", atPriorityRestart: 0.8)
// ^^^^^                                      ^^^^^^^ await ... wait for it ... restart

If the Objective-C importer were to translate these names by first logically shuffling this information to the front, then it seems to me like it'd be a consistently better result:

// Original
thing:frobnicateWithCompletionHandler:

// Rearrange to pull name information next to `await` use site
frobnicateWithCompletionHandler:thing:

// Eliminate "WithCompletionHandler"
frobnicateThing:

When the design for async/await is eventually shared, I hope you'll devote some text to translation from legacy APIs for discussion and possible enhancement, just like the review process that the original Objective-C-to-Swift translation went through.

@end

@protocol RefrigeratorDelegate<NSObject>
Expand Down