Skip to content

[cxx-interop] Offer fix-it to re-write ".at(42)" as "[42]". #63673

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.

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 10 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 45 additions & 14 deletions include/swift/AST/DiagnosticsClangImporter.def
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,12 @@ NOTE(macro_not_imported_invalid_numeric_literal, none, "invalid numeric literal"
NOTE(macro_not_imported_unsupported_literal, none, "only numeric and string macro literals supported", ())
NOTE(macro_not_imported_nested_cast, none, "non-null nested casts not supported", ())

NOTE(macro_not_imported_function_like, none, "macro '%0' not imported: function like macros not supported", (StringRef))
NOTE(macro_not_imported_unsupported_structure, none, "macro '%0' not imported: structure not supported", (StringRef))
NOTE(macro_not_imported, none, "macro '%0' not imported", (StringRef))
NOTE(macro_not_imported_function_like, none, "macro '%0' unavailable: function like macros not supported", (StringRef))
NOTE(macro_not_imported_unsupported_structure, none, "macro '%0' unavailable: structure not supported", (StringRef))
NOTE(macro_not_imported, none, "macro '%0' unavailable (cannot import)", (StringRef))

NOTE(return_type_not_imported, none, "return type not imported", ())
NOTE(parameter_type_not_imported, none, "parameter %0 not imported", (const clang::NamedDecl*))
NOTE(return_type_not_imported, none, "return type unavailable (cannot import)", ())
NOTE(parameter_type_not_imported, none, "parameter %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(incomplete_interface, none, "interface %0 is incomplete", (const clang::NamedDecl*))
NOTE(incomplete_protocol, none, "protocol %0 is incomplete", (const clang::NamedDecl*))
NOTE(incomplete_record, none, "record '%0' is not defined (incomplete)", (StringRef))
Expand All @@ -170,13 +170,44 @@ NOTE(record_not_automatically_importable, none, "record '%0' is not "
"Refer to the C++ Interop User "
"Manual to classify this type.",
(StringRef, StringRef))
NOTE(projection_not_imported, none, "C++ method '%0' that returns unsafe "
"projection of type '%1' not imported",

NOTE(projection_value_not_imported, none, "C++ method '%0' that returns a value "
"of type '%1' is unavailable. ",
(StringRef, StringRef))
NOTE(projection_ptr_not_imported, none, "C++ method '%0' that returns a pointer "
"of type '%1' is unavailable. ",
(StringRef, StringRef))
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use DescriptiveDeclKind, Identifier in cases like this instead of spelling out method and quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's important that we explicitly say that this is a C++ method, so I'm not sure DescriptiveDeclKind will work.

NOTE(dont_use_iterator_api, none, "C++ method '%0' that returns an unsafe "
"iterator not imported: use Swift Sequence "
"APIs instead",
NOTE(projection_reference_not_imported, none, "C++ method '%0' that returns a reference "
"of type '%1' is unavailable. ",
(StringRef, StringRef))
NOTE(projection_may_return_interior_ptr, none, "C++ method '%0' may return an "
"interior pointer. ",
(StringRef))
NOTE(mark_self_contained, none, "Mark type '%0' as 'self_contained' in C++ to "
"make methods that use it available in Swift. ",
(StringRef))
NOTE(mark_safe_to_import, none, "Mark method '%0' as 'safe_to_import' in C++ to "
"make it available in Swift. ",
(StringRef))

NOTE(at_to_subscript, none, "Do you want to replace it with a call "
"to the subscript operator?",
())
NOTE(get_swift_iterator, none, "Do you want to make a Swift iterator instead?",
())
NOTE(replace_with_nil, none, "Do you want to compare against 'nil' instead?'",
())
NOTE(get_first_element, none, "Do you want to get the first element instead?",
())
NOTE(get_last_element, none, "Do you want to get the last element instead?",
())

NOTE(iterator_method_unavailable, none, "C++ method '%0' that returns an "
"iterator is unavailable",
(StringRef))
NOTE(iterator_potentially_unsafe, none, "C++ methods that return iterators "
"are potentially unsafe. Try re-writing "
"to use Swift iterator APIs.", ())

ERROR(reference_type_must_have_retain_attr,none,
"reference type '%0' must have 'retain:' swift attribute.", (StringRef))
Expand Down Expand Up @@ -210,10 +241,10 @@ ERROR(conforms_to_not_protocol,none,
"specified protocol conformance '%0' is not a protocol.", (StringRef))

NOTE(unsupported_builtin_type, none, "built-in type '%0' not supported", (StringRef))
NOTE(record_field_not_imported, none, "field %0 not imported", (const clang::NamedDecl*))
NOTE(invoked_func_not_imported, none, "function %0 not imported", (const clang::NamedDecl*))
NOTE(record_method_not_imported, none, "method %0 not imported", (const clang::NamedDecl*))
NOTE(objc_property_not_imported, none, "property %0 not imported", (const clang::NamedDecl*))
NOTE(record_field_not_imported, none, "field %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(invoked_func_not_imported, none, "function %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(record_method_not_imported, none, "method %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(objc_property_not_imported, none, "property %0 unavailable (cannot import)", (const clang::NamedDecl*))

NOTE(forward_declared_interface_label, none, "interface %0 forward declared here", (const clang::NamedDecl*))
NOTE(forward_declared_protocol_label, none, "protocol %0 forward declared here", (const clang::NamedDecl*))
Expand Down
147 changes: 128 additions & 19 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3743,8 +3743,10 @@ bool SubscriptMisuseFailure::diagnoseAsNote() {
return false;
}

static void diagnoseUnsafeCxxMethod(SourceLoc loc, Type baseType,
DeclName name) {
void MissingMemberFailure::diagnoseUnsafeCxxMethod(SourceLoc loc,
ASTNode anchor,
Type baseType,
DeclName name) const {
auto &ctx = baseType->getASTContext();

if (baseType->getAnyNominal() == nullptr ||
Expand All @@ -3762,25 +3764,131 @@ static void diagnoseUnsafeCxxMethod(SourceLoc loc, Type baseType,
if (!cxxMethod)
continue;

if (name.getBaseIdentifier().str() == "begin" ||
name.getBaseIdentifier().str() == "end") {
ctx.Diags.diagnose(loc, diag::dont_use_iterator_api,
auto conformsToRACollection = [&](auto baseType) {
auto raCollectionProto =
ctx.getProtocol(KnownProtocolKind::CxxRandomAccessCollection);
// We didn't load the overlay.
if (raCollectionProto == nullptr)
return false;

SmallVector<ProtocolConformance *, 2> scratch;
return baseType->getAnyNominal()->lookupConformance(raCollectionProto,
scratch);
};

auto returnTypeStr = cast<FuncDecl>(found)
->getResultInterfaceType()
->getAnyNominal()
->getName()
.str();

// Rewrite front() and back() as first and last.
if ((name.getBaseIdentifier().is("front") ||
name.getBaseIdentifier().is("back")) &&
cxxMethod->getReturnType()->isReferenceType() &&
conformsToRACollection(baseType)) {
auto dotExpr = getAsExpr<UnresolvedDotExpr>(anchor);
auto callExpr = getAsExpr<CallExpr>(findParentExpr(dotExpr));
bool isFront = name.getBaseIdentifier().is("front");

ctx.Diags.diagnose(loc, diag::projection_reference_not_imported,
name.getBaseIdentifier().str(), returnTypeStr);
ctx.Diags.diagnose(loc, diag::projection_may_return_interior_ptr,
name.getBaseIdentifier().str());
ctx.Diags
.diagnose(loc,
isFront ? diag::get_first_element : diag::get_last_element)
.fixItReplaceChars(
loc, loc.getAdvancedLoc(name.getBaseIdentifier().str().size()),
isFront ? "first" : "last")
.fixItRemove({callExpr->getArgs()->getStartLoc(),
callExpr->getArgs()->getEndLoc()});
}

// Rewrite begin() as a call to makeIterator() and end as nil.
if (name.getBaseIdentifier().is("begin")) {
if (conformsToRACollection(baseType)) {
ctx.Diags.diagnose(loc, diag::iterator_method_unavailable,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::get_swift_iterator)
.fixItReplaceChars(
loc, loc.getAdvancedLoc(name.getBaseIdentifier().str().size()),
"makeIterator");
} else {
ctx.Diags.diagnose(loc, diag::iterator_method_unavailable,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::iterator_potentially_unsafe);
}
} else if (name.getBaseIdentifier().is("end")) {
if (conformsToRACollection(baseType)) {
auto dotExpr = getAsExpr<UnresolvedDotExpr>(anchor);
auto callExpr = getAsExpr<CallExpr>(findParentExpr(dotExpr));

ctx.Diags.diagnose(loc, diag::iterator_method_unavailable,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::replace_with_nil)
.fixItReplaceChars(loc, callExpr->getArgs()->getEndLoc(), "nil");
} else {
ctx.Diags.diagnose(loc, diag::iterator_method_unavailable,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::iterator_potentially_unsafe);
}
} else if (cxxMethod->getReturnType()->isPointerType()) {
ctx.Diags.diagnose(loc, diag::projection_ptr_not_imported,
name.getBaseIdentifier().str(), returnTypeStr);
ctx.Diags.diagnose(loc, diag::projection_may_return_interior_ptr,
name.getBaseIdentifier().str());
} else if (cxxMethod->getReturnType()->isPointerType())
ctx.Diags.diagnose(loc, diag::projection_not_imported,
name.getBaseIdentifier().str(), "pointer");
else if (cxxMethod->getReturnType()->isReferenceType())
ctx.Diags.diagnose(loc, diag::projection_not_imported,
name.getBaseIdentifier().str(), "reference");
else if (cxxMethod->getReturnType()->isRecordType()) {
ctx.Diags.diagnose(loc, diag::mark_safe_to_import,
name.getBaseIdentifier().str());
} else if (cxxMethod->getReturnType()->isReferenceType()) {
// Rewrite a call to .at(42) as a subscript.
if (name.getBaseIdentifier().is("at") &&
cxxMethod->getReturnType()->isReferenceType() &&
conformsToRACollection(baseType)) {
auto dotExpr = getAsExpr<UnresolvedDotExpr>(anchor);
auto callExpr = getAsExpr<CallExpr>(findParentExpr(dotExpr));

ctx.Diags.diagnose(loc, diag::projection_reference_not_imported,
name.getBaseIdentifier().str(), returnTypeStr);
ctx.Diags.diagnose(loc, diag::projection_may_return_interior_ptr,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::at_to_subscript)
.fixItRemove(
{dotExpr->getDotLoc(), callExpr->getArgs()->getStartLoc()})
.fixItReplaceChars(
callExpr->getArgs()->getStartLoc(),
callExpr->getArgs()->getStartLoc().getAdvancedLoc(1), "[")
.fixItReplaceChars(
callExpr->getArgs()->getEndLoc(),
callExpr->getArgs()->getEndLoc().getAdvancedLoc(1), "]");
} else {
ctx.Diags.diagnose(loc, diag::projection_reference_not_imported,
name.getBaseIdentifier().str(), returnTypeStr);
ctx.Diags.diagnose(loc, diag::projection_may_return_interior_ptr,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::mark_safe_to_import,
name.getBaseIdentifier().str());
}
} else if (cxxMethod->getReturnType()->isRecordType()) {
if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(
cxxMethod->getReturnType()->getAsRecordDecl())) {
assert(evaluateOrDefault(ctx.evaluator,
CxxRecordSemantics({cxxRecord, ctx}), {}) ==
CxxRecordSemanticsKind::UnsafePointerMember);
ctx.Diags.diagnose(loc, diag::projection_not_imported,
name.getBaseIdentifier().str(),
cxxRecord->getNameAsString());
auto methodSemantics = evaluateOrDefault(
ctx.evaluator, CxxRecordSemantics({cxxRecord, ctx}), {});
if (methodSemantics == CxxRecordSemanticsKind::Iterator) {
ctx.Diags.diagnose(loc, diag::iterator_method_unavailable,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::iterator_potentially_unsafe);
} else {
assert(methodSemantics ==
CxxRecordSemanticsKind::UnsafePointerMember);
ctx.Diags.diagnose(loc, diag::projection_value_not_imported,
name.getBaseIdentifier().str(), returnTypeStr);
ctx.Diags.diagnose(loc, diag::projection_may_return_interior_ptr,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::mark_safe_to_import,
name.getBaseIdentifier().str());
ctx.Diags.diagnose(loc, diag::mark_self_contained, returnTypeStr);
}
}
}
}
Expand Down Expand Up @@ -3860,7 +3968,8 @@ bool MissingMemberFailure::diagnoseAsError() {
if (!ctx.LangOpts.DisableExperimentalClangImporterDiagnostics) {
ctx.getClangModuleLoader()->diagnoseMemberValue(getName().getFullName(),
baseType);
diagnoseUnsafeCxxMethod(getLoc(), baseType, getName().getFullName());
diagnoseUnsafeCxxMethod(getLoc(), anchor, baseType,
getName().getFullName());
}
};

Expand Down
13 changes: 12 additions & 1 deletion lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -1255,7 +1255,18 @@ class MissingMemberFailure : public InvalidMemberRefFailure {
/// overload to be present, but a class marked as `@dynamicCallable`
/// defines only `dynamicallyCall(withArguments:)` variant.
bool diagnoseForDynamicCallable() const;


/// Diagnose methods that return unsafe projections and suggest fixits.
/// For example, if Swift cannot find "vector::data" because it is unsafe, try
/// to diagnose this and tell the user why we did not import "vector::data".
///
/// Provides fixits for:
/// at -> subscript
/// begin, end -> makeIterator
/// front, back -> first, last
void diagnoseUnsafeCxxMethod(SourceLoc loc, ASTNode anchor, Type baseType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a small comment there describing what this method is trying to do?

DeclName name) const;

/// Tailored diagnostics for collection literal with unresolved member expression
/// that defaults the element type. e.g. _ = [.e]
bool diagnoseInLiteralCollectionContext() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ s.c = 5
// CHECK: experimental_clang_importer_diagnostics_bridging_header.swift:{{[0-9]+}}:3: error: value of type 'PartialImport' has no member 'c'
// CHECK-NEXT: s.c = 5
// CHECK-NEXT: ~ ^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:3: note: field 'c' not imported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:3: note: field 'c' unavailable (cannot import)
// CHECK-NEXT: int _Complex c;
// CHECK-NEXT: ^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:3: note: built-in type 'Complex' not supported
Expand All @@ -16,10 +16,10 @@ _ = CFunctionReturningAForwardDeclaredInterface()
// CHECK: experimental_clang_importer_diagnostics_bridging_header.swift:{{[0-9]+}}:5: error: cannot find 'CFunctionReturningAForwardDeclaredInterface' in scope
// CHECK-NEXT: _ = CFunctionReturningAForwardDeclaredInterface()
// CHECK-NEXT: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: function 'CFunctionReturningAForwardDeclaredInterface' not imported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: function 'CFunctionReturningAForwardDeclaredInterface' unavailable (cannot import)
// CHECK-NEXT: ForwardDeclaredInterface* CFunctionReturningAForwardDeclaredInterface();
// CHECK-NEXT: ^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: return type not imported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: return type unavailable (cannot import)
// CHECK-NEXT: ForwardDeclaredInterface* CFunctionReturningAForwardDeclaredInterface();
// CHECK-NEXT: ^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: interface 'ForwardDeclaredInterface' is incomplete
Expand All @@ -33,18 +33,18 @@ _ = FUNC_LIKE_MACRO()
// CHECK: experimental_clang_importer_diagnostics_bridging_header.swift:{{[0-9]+}}:{{[0-9]+}}: error: cannot find 'FUNC_LIKE_MACRO' in scope
// CHECK-NEXT: _ = FUNC_LIKE_MACRO()
// CHECK-NEXT: ^~~~~~~~~~~~~~~
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:9: note: macro 'FUNC_LIKE_MACRO' not imported: function like macros not supported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:9: note: macro 'FUNC_LIKE_MACRO' unavailable: function like macros not supported
// CHECK-NEXT: #define FUNC_LIKE_MACRO() 0
// CHECK-NEXT: {{^}} ^

unsupported_return_type()
// CHECK: experimental_clang_importer_diagnostics_bridging_header.swift:{{[0-9]+}}:{{[0-9]+}}: error: cannot find 'unsupported_return_type' in scope
// CHECK-NEXT: unsupported_return_type()
// CHECK-NEXT: ^~~~~~~~~~~~~~~~~~~~~~~
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: function 'unsupported_return_type' not imported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: function 'unsupported_return_type' unavailable (cannot import)
// CHECK-NEXT: _Complex int unsupported_return_type();
// CHECK-NEXT: {{^}}^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: return type not imported
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: return type unavailable (cannot import)
// CHECK-NEXT: _Complex int unsupported_return_type();
// CHECK-NEXT: {{^}}^
// CHECK-NEXT: experimental_clang_importer_diagnostics_bridging_header.h:{{[0-9]+}}:1: note: built-in type 'Complex' not supported
Expand Down
Loading