Skip to content

[SE-0458] Unify escapability inference for AST and Interop #79434

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
Feb 17, 2025
Merged

[SE-0458] Unify escapability inference for AST and Interop #79434

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
6 changes: 2 additions & 4 deletions include/swift/ClangImporter/ClangImporterRequests.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,8 @@ class CxxRecordAsSwiftType

struct SafeUseOfCxxDeclDescriptor final {
const clang::Decl *decl;
ASTContext &ctx;

SafeUseOfCxxDeclDescriptor(const clang::Decl *decl, ASTContext &ctx)
: decl(decl), ctx(ctx) {}
SafeUseOfCxxDeclDescriptor(const clang::Decl *decl) : decl(decl) {}

friend llvm::hash_code hash_value(const SafeUseOfCxxDeclDescriptor &desc) {
return llvm::hash_combine(desc.decl);
Expand Down Expand Up @@ -528,7 +526,7 @@ enum class CxxEscapability { Escapable, NonEscapable, Unknown };

struct EscapabilityLookupDescriptor final {
const clang::Type *type;
ClangImporter::Implementation &impl;
ClangImporter::Implementation *impl;
bool annotationOnly = true;

friend llvm::hash_code hash_value(const EscapabilityLookupDescriptor &desc) {
Expand Down
6 changes: 3 additions & 3 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,9 +1153,9 @@ ExplicitSafety Decl::getExplicitSafety() const {
// If this declaration is from C, ask the Clang importer.
if (auto clangDecl = getClangDecl()) {
ASTContext &ctx = getASTContext();
return evaluateOrDefault(
ctx.evaluator, ClangDeclExplicitSafety({clangDecl, ctx}),
ExplicitSafety::Unspecified);
return evaluateOrDefault(ctx.evaluator,
ClangDeclExplicitSafety({clangDecl}),
ExplicitSafety::Unspecified);
}

// Inference: Check the enclosing context.
Expand Down
32 changes: 17 additions & 15 deletions lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5213,9 +5213,10 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
} else
nonPackArgs.push_back(arg);
for (auto nonPackArg : nonPackArgs) {
if (nonPackArg.getKind() != clang::TemplateArgument::Type) {
desc.impl.diagnose(loc, diag::type_template_parameter_expected,
argToCheck.second);
if (nonPackArg.getKind() != clang::TemplateArgument::Type &&
desc.impl) {
desc.impl->diagnose(loc, diag::type_template_parameter_expected,
Copy link
Member

Choose a reason for hiding this comment

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

If we're okay with disabling these diagnostics when we don't have the impl on hand, should we just remove them entirely?

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 we should refactor this code at some point. When we import these types we will always have the impl at hand. After they are imported, we no longer need to emit diagnostics about these types so we don't really need the impl. That being said a better design would be to return some information about the diagnostics that needs to be emitted (instead of having a side effect) and only consume those in the importer and ignore them in other parts of the compiler.

argToCheck.second);
return CxxEscapability::Unknown;
}

Expand All @@ -5236,8 +5237,9 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
}
}

for (auto name : conditionalParams)
desc.impl.diagnose(loc, diag::unknown_template_parameter, name);
if (desc.impl)
for (auto name : conditionalParams)
desc.impl->diagnose(loc, diag::unknown_template_parameter, name);

return hadUnknown ? CxxEscapability::Unknown : CxxEscapability::Escapable;
}
Expand Down Expand Up @@ -8216,9 +8218,7 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
}

/// Check whether the given Clang type involves an unsafe type.
static bool hasUnsafeType(
Evaluator &evaluator, ASTContext &swiftContext, clang::QualType clangType
) {
static bool hasUnsafeType(Evaluator &evaluator, clang::QualType clangType) {
// Handle pointers.
auto pointeeType = clangType->getPointeeType();
if (!pointeeType.isNull()) {
Expand All @@ -8239,10 +8239,9 @@ static bool hasUnsafeType(

// Handle records recursively.
if (auto recordDecl = clangType->getAsTagDecl()) {
auto safety = evaluateOrDefault(
evaluator,
ClangDeclExplicitSafety({recordDecl, swiftContext}),
ExplicitSafety::Unspecified);
auto safety =
evaluateOrDefault(evaluator, ClangDeclExplicitSafety({recordDecl}),
ExplicitSafety::Unspecified);
switch (safety) {
case ExplicitSafety::Unsafe:
return true;
Expand Down Expand Up @@ -8285,7 +8284,10 @@ ExplicitSafety ClangDeclExplicitSafety::evaluate(

// Escapable and non-escapable annotations imply that the declaration is
// safe.
if (hasNonEscapableAttr(recordDecl) || hasEscapableAttr(recordDecl))
if (evaluateOrDefault(
evaluator,
ClangTypeEscapability({recordDecl->getTypeForDecl(), nullptr}),
CxxEscapability::Unknown) != CxxEscapability::Unknown)
return ExplicitSafety::Safe;

// If we don't have a definition, leave it unspecified.
Expand All @@ -8296,14 +8298,14 @@ ExplicitSafety ClangDeclExplicitSafety::evaluate(
// If this is a C++ class, check its bases.
if (auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl)) {
for (auto base : cxxRecordDecl->bases()) {
if (hasUnsafeType(evaluator, desc.ctx, base.getType()))
if (hasUnsafeType(evaluator, base.getType()))
return ExplicitSafety::Unsafe;
}
}

// Check the fields.
for (auto field : recordDecl->fields()) {
if (hasUnsafeType(evaluator, desc.ctx, field->getType()))
if (hasUnsafeType(evaluator, field->getType()))
return ExplicitSafety::Unsafe;
}

Expand Down
9 changes: 4 additions & 5 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,7 +2246,7 @@ namespace {
bool isNonEscapable = false;
if (evaluateOrDefault(
Impl.SwiftContext.evaluator,
ClangTypeEscapability({decl->getTypeForDecl(), Impl}),
ClangTypeEscapability({decl->getTypeForDecl(), &Impl}),
CxxEscapability::Unknown) == CxxEscapability::NonEscapable) {
result->getAttrs().add(new (Impl.SwiftContext)
NonEscapableAttr(/*Implicit=*/true));
Expand Down Expand Up @@ -4118,7 +4118,7 @@ namespace {
evaluateOrDefault(
Impl.SwiftContext.evaluator,
ClangTypeEscapability(
{ctordecl->getParent()->getTypeForDecl(), Impl}),
{ctordecl->getParent()->getTypeForDecl(), &Impl}),
CxxEscapability::Unknown) == CxxEscapability::NonEscapable)
lifetimeDependencies.push_back(immortalLifetime);
}
Expand Down Expand Up @@ -8505,8 +8505,7 @@ static bool importAsUnsafe(ClangImporter::Implementation &impl,
return false;

if (isa<clang::CXXMethodDecl>(decl) &&
!evaluateOrDefault(context.evaluator, IsSafeUseOfCxxDecl({decl, context}),
{}))
!evaluateOrDefault(context.evaluator, IsSafeUseOfCxxDecl({decl}), {}))
return true;

if (isa<ClassDecl>(MappedDecl))
Expand All @@ -8522,7 +8521,7 @@ static bool importAsUnsafe(ClangImporter::Implementation &impl,
if (const auto *record = dyn_cast<clang::RecordDecl>(decl))
return evaluateOrDefault(
context.evaluator,
ClangTypeEscapability({record->getTypeForDecl(), impl, false}),
ClangTypeEscapability({record->getTypeForDecl(), &impl, false}),
CxxEscapability::Unknown) == CxxEscapability::Unknown;

return false;
Expand Down
2 changes: 1 addition & 1 deletion lib/ClangImporter/ImportName.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,7 +1494,7 @@ static StringRef renameUnsafeMethod(ASTContext &ctx,
const clang::NamedDecl *decl,
StringRef name) {
if (isa<clang::CXXMethodDecl>(decl) &&
!evaluateOrDefault(ctx.evaluator, IsSafeUseOfCxxDecl({decl, ctx}), {})) {
!evaluateOrDefault(ctx.evaluator, IsSafeUseOfCxxDecl({decl}), {})) {
return ctx.getIdentifier(("__" + name + "Unsafe").str()).str();
}

Expand Down