Skip to content

[Sema] Diagnose type error on Clang type mismatch. #35305

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
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
23 changes: 14 additions & 9 deletions include/swift/AST/PrintOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,8 @@ struct PrintOptions {
}

/// Retrieve the set of options suitable for diagnostics printing.
static PrintOptions printForDiagnostics(AccessLevel accessFilter) {
static PrintOptions printForDiagnostics(AccessLevel accessFilter,
bool printFullConvention) {
PrintOptions result = printVerbose();
result.PrintAccess = true;
result.Indent = 4;
Expand All @@ -523,12 +524,16 @@ struct PrintOptions {
QualifyNestedDeclarations::TypesOnly;
result.PrintDocumentationComments = false;
result.PrintCurrentMembersOnly = true;
if (printFullConvention)
result.PrintFunctionRepresentationAttrs =
PrintOptions::FunctionRepresentationMode::Full;
return result;
}

/// Retrieve the set of options suitable for interface generation.
static PrintOptions printInterface() {
PrintOptions result = printForDiagnostics(AccessLevel::Public);
static PrintOptions printInterface(bool printFullConvention) {
PrintOptions result =
printForDiagnostics(AccessLevel::Public, printFullConvention);
result.SkipUnavailable = true;
result.SkipImplicit = true;
result.SkipSwiftPrivateClangDecls = true;
Expand Down Expand Up @@ -565,8 +570,8 @@ struct PrintOptions {
/// Retrieve the set of options suitable for "Generated Interfaces", which
/// are a prettified representation of the public API of a module, to be
/// displayed to users in an editor.
static PrintOptions printModuleInterface();
static PrintOptions printTypeInterface(Type T);
static PrintOptions printModuleInterface(bool printFullConvention);
static PrintOptions printTypeInterface(Type T, bool printFullConvention);

void setBaseType(Type T);

Expand All @@ -582,16 +587,16 @@ struct PrintOptions {
}

/// Retrieve the print options that are suitable to print the testable interface.
static PrintOptions printTestableInterface() {
PrintOptions result = printInterface();
static PrintOptions printTestableInterface(bool printFullConvention) {
PrintOptions result = printInterface(printFullConvention);
result.AccessFilter = AccessLevel::Internal;
return result;
}

/// Retrieve the print options that are suitable to print interface for a
/// swift file.
static PrintOptions printSwiftFileInterface() {
PrintOptions result = printInterface();
static PrintOptions printSwiftFileInterface(bool printFullConvention) {
PrintOptions result = printInterface(printFullConvention);
result.AccessFilter = AccessLevel::Internal;
result.EmptyLineBetweenMembers = true;
return result;
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,9 @@ namespace swift {
/// Enable experimental support for one-way constraints for the
/// parameters of closures.
bool EnableOneWayClosureParameters = false;

/// See \ref FrontendOptions.PrintFullConvention
bool PrintFullConvention = false;
};

/// Options for controlling the behavior of the Clang importer.
Expand Down
11 changes: 11 additions & 0 deletions include/swift/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -1375,6 +1375,17 @@ enum class ConstraintSystemFlags {
/// CodeCompletionExpr, and should continue in the presence of errors wherever
/// possible.
ForCodeCompletion = 0x40,

/// Include Clang function types when checking equality for function types.
///
/// When LangOptions.UseClangFunctionTypes is set, we can synthesize
/// different @convention(c) function types with the same parameter and result
/// types (similarly for blocks). This difference is reflected in the `cType:`
/// field (@convention(c, cType: ...)). When the cType is different, the types
/// should be treated as semantically different, as they may have different
/// calling conventions, say due to Clang attributes such as
/// `__attribute__((ns_consumed))`.
UseClangFunctionTypes = 0x80,
};

/// Options that affect the constraint system as a whole.
Expand Down
27 changes: 20 additions & 7 deletions lib/AST/DiagnosticEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -442,12 +442,13 @@ static bool shouldShowAKA(Type type, StringRef typeName) {
/// with the same string representation, it should be qualified during
/// formatting.
static bool typeSpellingIsAmbiguous(Type type,
ArrayRef<DiagnosticArgument> Args) {
ArrayRef<DiagnosticArgument> Args,
PrintOptions &PO) {
for (auto arg : Args) {
if (arg.getKind() == DiagnosticArgumentKind::Type) {
auto argType = arg.getAsType();
if (argType && !argType->isEqual(type) &&
argType->getWithoutParens().getString() == type.getString()) {
argType->getWithoutParens().getString(PO) == type.getString(PO)) {
return true;
}
}
Expand Down Expand Up @@ -543,12 +544,22 @@ static void formatDiagnosticArgument(StringRef Modifier,
Type type;
bool needsQualification = false;

// TODO: We should use PrintOptions::printForDiagnostic here, or we should
// rename that method.
PrintOptions printOptions{};

if (Arg.getKind() == DiagnosticArgumentKind::Type) {
type = Arg.getAsType()->getWithoutParens();
needsQualification = typeSpellingIsAmbiguous(type, Args);
if (type->getASTContext().TypeCheckerOpts.PrintFullConvention)
printOptions.PrintFunctionRepresentationAttrs =
PrintOptions::FunctionRepresentationMode::Full;
needsQualification = typeSpellingIsAmbiguous(type, Args, printOptions);
} else {
assert(Arg.getKind() == DiagnosticArgumentKind::FullyQualifiedType);
type = Arg.getAsFullyQualifiedType().getType()->getWithoutParens();
if (type->getASTContext().TypeCheckerOpts.PrintFullConvention)
printOptions.PrintFunctionRepresentationAttrs =
PrintOptions::FunctionRepresentationMode::Full;
needsQualification = true;
}

Expand All @@ -574,12 +585,11 @@ static void formatDiagnosticArgument(StringRef Modifier,
auto descriptiveKind = opaqueTypeDecl->getDescriptiveKind();

Out << llvm::format(FormatOpts.OpaqueResultFormatString.c_str(),
type->getString().c_str(),
type->getString(printOptions).c_str(),
Decl::getDescriptiveKindName(descriptiveKind).data(),
NamingDeclText.c_str());

} else {
auto printOptions = PrintOptions();
printOptions.FullyQualifiedTypes = needsQualification;
std::string typeName = type->getString(printOptions);

Expand Down Expand Up @@ -1016,8 +1026,11 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
// Pretty-print the declaration we've picked.
llvm::raw_svector_ostream out(buffer);
TrackingPrinter printer(entries, out, bufferAccessLevel);
ppDecl->print(printer,
PrintOptions::printForDiagnostics(bufferAccessLevel));
ppDecl->print(
printer,
PrintOptions::printForDiagnostics(
bufferAccessLevel,
decl->getASTContext().TypeCheckerOpts.PrintFullConvention));
}

// Build a buffer with the pretty-printed declaration.
Expand Down
3 changes: 3 additions & 0 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,9 @@ static bool ParseTypeCheckerArgs(TypeCheckerOptions &Opts, ArgList &Args,
Opts.EnableOneWayClosureParameters |=
Args.hasArg(OPT_experimental_one_way_closure_params);

Opts.PrintFullConvention |=
Args.hasArg(OPT_experimental_print_full_convention);

Opts.DebugConstraintSolver |= Args.hasArg(OPT_debug_constraints);
Opts.DebugGenericSignatures |= Args.hasArg(OPT_debug_generic_signatures);

Expand Down
3 changes: 2 additions & 1 deletion lib/FrontendTool/FrontendTool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,8 @@ static void dumpAPIIfNeeded(const CompilerInstance &Instance) {
SmallString<512> TempBuf;
llvm::raw_svector_ostream TempOS(TempBuf);

PrintOptions PO = PrintOptions::printInterface();
PrintOptions PO = PrintOptions::printInterface(
Invocation.getFrontendOptions().PrintFullConvention);
PO.PrintOriginalSourceText = true;
PO.Indent = 2;
PO.PrintAccess = false;
Expand Down
3 changes: 2 additions & 1 deletion lib/IDE/CommentConversion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,8 @@ visitDocComment(const DocComment *DC, TypeOrExtensionDecl SynthesizedTarget) {
}

{
PrintOptions PO = PrintOptions::printInterface();
PrintOptions PO = PrintOptions::printInterface(
D->getASTContext().TypeCheckerOpts.PrintFullConvention);
PO.PrintAccess = false;
PO.AccessFilter = AccessLevel::Private;
PO.PrintDocumentationComments = false;
Expand Down
12 changes: 7 additions & 5 deletions lib/IDE/IDETypeChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,15 @@ class ModulePrinterPrintableChecker: public ShouldPrintChecker {
}
};

PrintOptions PrintOptions::printModuleInterface() {
PrintOptions result = printInterface();
PrintOptions PrintOptions::printModuleInterface(bool printFullConvention) {
PrintOptions result = printInterface(printFullConvention);
result.CurrentPrintabilityChecker.reset(new ModulePrinterPrintableChecker());
return result;
}

PrintOptions PrintOptions::printTypeInterface(Type T) {
PrintOptions result = printModuleInterface();
PrintOptions PrintOptions::printTypeInterface(Type T,
bool printFullConvention) {
PrintOptions result = printModuleInterface(printFullConvention);
result.PrintExtensionFromConformingProtocols = true;
result.TransformContext = TypeTransformContext(T);
result.printExtensionContentAsMembers = [T](const ExtensionDecl *ED) {
Expand All @@ -77,7 +78,8 @@ PrintOptions PrintOptions::printTypeInterface(Type T) {
}

PrintOptions PrintOptions::printDocInterface() {
PrintOptions result = PrintOptions::printModuleInterface();
PrintOptions result =
PrintOptions::printModuleInterface(/*printFullConvention*/ false);
result.PrintAccess = false;
result.SkipUnavailable = false;
result.ExcludeAttrList.push_back(DAK_Available);
Expand Down
4 changes: 3 additions & 1 deletion lib/IDE/ModuleInterfacePrinting.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,9 @@ printTypeInterface(ModuleDecl *M, Type Ty, ASTPrinter &Printer,
}
Ty = Ty->getRValueType();
if (auto ND = Ty->getNominalOrBoundGenericNominal()) {
PrintOptions Options = PrintOptions::printTypeInterface(Ty.getPointer());
PrintOptions Options = PrintOptions::printTypeInterface(
Ty.getPointer(),
Ty->getASTContext().TypeCheckerOpts.PrintFullConvention);
ND->print(Printer, Options);
printTypeNameToString(Ty, TypeName);
return false;
Expand Down
51 changes: 41 additions & 10 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1557,19 +1557,38 @@ isSubtypeOf(FunctionTypeRepresentation potentialSubRepr,
|| isThickRepresentation(potentialSuperRepr);
}

/// Returns true if `constraint rep1 rep2` is satisfied.
static bool matchFunctionRepresentations(FunctionTypeRepresentation rep1,
FunctionTypeRepresentation rep2,
ConstraintKind kind) {
/// Returns true if `constraint extInfo1 extInfo2` is satisfied.
static bool matchFunctionRepresentations(FunctionType::ExtInfo einfo1,
FunctionType::ExtInfo einfo2,
ConstraintKind kind,
ConstraintSystemOptions options) {
auto rep1 = einfo1.getRepresentation();
auto rep2 = einfo2.getRepresentation();
bool clangTypeMismatch =
(options.contains(ConstraintSystemFlags::UseClangFunctionTypes) &&
(einfo1.getClangTypeInfo() != einfo2.getClangTypeInfo()));
switch (kind) {
case ConstraintKind::Bind:
case ConstraintKind::BindParam:
case ConstraintKind::BindToPointerType:
case ConstraintKind::Equal:
return rep1 == rep2;
return (rep1 == rep2) && !clangTypeMismatch;

case ConstraintKind::Subtype: {
return isSubtypeOf(rep1, rep2);
// Breakdown of cases:
// 1. isSubtypeOf(rep1, rep2) == false (hence rep1 != rep2):
// In this case, this function will return false, indicating that we
// can't convert. E.g. you can't convert from @convention(swift) to
// @convention(c).
// 2. isSubtypeOf(rep1, rep2) == true and rep1 != rep2:
// In this case, this function will return true, indicating that we
// can convert, because the Clang type doesn't matter when converting
// between different representations. E.g. it is okay to convert from
// @convention(c) (regardless of cType) to @convention(swift).
// 3. isSubtypeOf(rep1, rep2) == true and rep1 == rep2:
// In this case, the function returns !clangTypeMismatch, as we forbid
// conversions between @convention(c) functions with different cTypes.
return isSubtypeOf(rep1, rep2) && ((rep1 != rep2) || !clangTypeMismatch);
}

// [NOTE: diagnose-swift-to-c-convention-change]: @convention(swift) ->
Expand All @@ -1588,6 +1607,19 @@ static bool matchFunctionRepresentations(FunctionTypeRepresentation rep1,
case ConstraintKind::Conversion:
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentConversion:
// For now, forbid conversion if representations match but cTypes differ.
//
// let f : @convention(c, cType: "id (*)(void) __attribute__((ns_returns_retained))")
// () -> AnyObject = ...
// let _ : @convention(c, cType: "id (*)(void)")
// () -> AnyObject = f // error
// let g : @convention(c, cType: "void (*)(void *)")
// (OpaquePointer?) -> () = ...
// let _ : @convention(c, cType: "void (*)(MyCtx *)")
// (OpaquePointer?) -> () = g // error
if ((rep1 == rep2) && clangTypeMismatch) {
return false;
}
return true;

case ConstraintKind::OpaqueUnderlyingType:
Expand Down Expand Up @@ -1908,9 +1940,8 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
return getTypeMatchFailure(locator);
}

if (!matchFunctionRepresentations(func1->getExtInfo().getRepresentation(),
func2->getExtInfo().getRepresentation(),
kind)) {
if (!matchFunctionRepresentations(func1->getExtInfo(), func2->getExtInfo(),
kind, Options)) {
return getTypeMatchFailure(locator);
}

Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ ConstraintSystem::ConstraintSystem(DeclContext *dc,
DC->getParentModule()->isMainModule()) {
Options |= ConstraintSystemFlags::DebugConstraints;
}
if (Context.LangOpts.UseClangFunctionTypes)
Options |= ConstraintSystemFlags::UseClangFunctionTypes;
}

ConstraintSystem::~ConstraintSystem() {
Expand Down
4 changes: 2 additions & 2 deletions lib/Sema/TypeCheckProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3085,8 +3085,8 @@ printRequirementStub(ValueDecl *Requirement, DeclContext *Adopter,
}
}

PrintOptions Options =
PrintOptions::printForDiagnostics(AccessLevel::Private);
PrintOptions Options = PrintOptions::printForDiagnostics(
AccessLevel::Private, Ctx.TypeCheckerOpts.PrintFullConvention);
Options.PrintDocumentationComments = false;
Options.PrintAccess = false;
Options.SkipAttributes = true;
Expand Down
7 changes: 4 additions & 3 deletions lib/SymbolGraphGen/SymbolGraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,10 @@ void SymbolGraph::recordConformanceSynthesizedMemberRelationships(Symbol S) {
return;
}

SynthesizedExtensionAnalyzer
ExtensionAnalyzer(const_cast<NominalTypeDecl*>(OwningNominal),
PrintOptions::printModuleInterface());
SynthesizedExtensionAnalyzer ExtensionAnalyzer(
const_cast<NominalTypeDecl *>(OwningNominal),
PrintOptions::printModuleInterface(
OwningNominal->getASTContext().TypeCheckerOpts.PrintFullConvention));
auto MergeGroupKind = SynthesizedExtensionAnalyzer::MergeGroupKind::All;
ExtensionAnalyzer.forEachExtensionMergeGroup(MergeGroupKind,
[&](ArrayRef<ExtensionInfo> ExtensionInfos){
Expand Down
50 changes: 50 additions & 0 deletions test/Sema/clang_fn_type_mismatch.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// RUN: %target-typecheck-verify-swift -sdk %clang-importer-sdk -experimental-print-full-convention -use-clang-function-types

import ctypes

// Setting a C function type with the correct cType works.
let f1 : (@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)? = getFunctionPointer_()

// However, trying to convert between @convention(c) functions
// with differing cTypes doesn't work.

let _ : @convention(c) (Int) -> Int = f1!
// expected-error@-1{{cannot convert value of type '@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int' to specified type '@convention(c) (Int) -> Int'}}

let _ : (@convention(c) (Int) -> Int)? = f1
// expected-error@-1{{cannot convert value of type '(@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?' to specified type '(@convention(c) (Int) -> Int)?'}}

let _ : (@convention(c, cType: "void *(*)(void *)") (Int) -> Int)? = f1
// expected-error@-1{{cannot convert value of type '(@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?' to specified type '(@convention(c, cType: "void *(*)(void *)") (Int) -> Int)?'}}


// Converting from @convention(c) -> @convention(swift) works

let _ : (Int) -> Int = ({ x in x } as @convention(c) (Int) -> Int)
let _ : (Int) -> Int = ({ x in x } as @convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)


// Converting from @convention(swift) -> @convention(c) doesn't work.

let fs : (Int) -> Int = { x in x }

let _ : @convention(c) (Int) -> Int = fs
// expected-error@-1{{a C function pointer can only be formed from a reference to a 'func' or a literal closure}}

let _ : @convention(c, cType: "size_t (*)(size_t)") (Int) -> Int = fs
// expected-error@-1{{a C function pointer can only be formed from a reference to a 'func' or a literal closure}}


// More complex examples.

let f2 : (@convention(c) ((@convention(c, cType: "size_t (*)(size_t)") (Swift.Int) -> Swift.Int)?) -> (@convention(c, cType: "size_t (*)(size_t)") (Swift.Int) -> Swift.Int)?)? = getHigherOrderFunctionPointer()!

let _ : (@convention(c) ((@convention(c) (Swift.Int) -> Swift.Int)?) -> (@convention(c, cType: "size_t (*)(size_t)") (Swift.Int) -> Swift.Int)?)? = f2!
// expected-error@-1{{cannot convert value of type '@convention(c) ((@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?) -> (@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?' to specified type '(@convention(c) ((@convention(c) (Int) -> Int)?) -> (@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?)?'}}

let _ : (@convention(c) ((@convention(c) (Swift.Int) -> Swift.Int)?) -> (@convention(c) (Swift.Int) -> Swift.Int)?)? = f2!
// expected-error@-1{{cannot convert value of type '@convention(c) ((@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?) -> (@convention(c, cType: "size_t (*)(size_t)") (Int) -> Int)?' to specified type '(@convention(c) ((@convention(c) (Int) -> Int)?) -> (@convention(c) (Int) -> Int)?)?'}}

let f3 = getFunctionPointer3

let _ : @convention(c) (UnsafeMutablePointer<ctypes.Dummy>?) -> UnsafeMutablePointer<ctypes.Dummy>? = f3()!
Loading