Merge pull request #35305 from varungandhi-apple/vg-diag-clang-type-mismatch

varungandhi-apple · web-flow · commit d77d7fa30852 · 2021-01-26T20:50:33.000-08:00
[Sema] Diagnose type error on Clang type mismatch.
diff --git a/include/swift/AST/PrintOptions.h b/include/swift/AST/PrintOptions.h
@@ -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;
@@ -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;
@@ -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);
 
@@ -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;
diff --git a/include/swift/Basic/LangOptions.h b/include/swift/Basic/LangOptions.h
@@ -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.
diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h
@@ -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.
diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp
@@ -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;
       }
     }
@@ -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;
     }
 
@@ -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);
 
@@ -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.
diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
@@ -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);
 
diff --git a/lib/FrontendTool/FrontendTool.cpp b/lib/FrontendTool/FrontendTool.cpp
@@ -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;
diff --git a/lib/IDE/CommentConversion.cpp b/lib/IDE/CommentConversion.cpp
@@ -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;
diff --git a/lib/IDE/IDETypeChecking.cpp b/lib/IDE/IDETypeChecking.cpp
@@ -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) {
@@ -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);
diff --git a/lib/IDE/ModuleInterfacePrinting.cpp b/lib/IDE/ModuleInterfacePrinting.cpp
@@ -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;
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -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) ->
@@ -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:
@@ -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);
   }
 
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -86,6 +86,8 @@ ConstraintSystem::ConstraintSystem(DeclContext *dc,
       DC->getParentModule()->isMainModule()) {
     Options |= ConstraintSystemFlags::DebugConstraints;
   }
+  if (Context.LangOpts.UseClangFunctionTypes)
+    Options |= ConstraintSystemFlags::UseClangFunctionTypes;
 }
 
 ConstraintSystem::~ConstraintSystem() {
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -3098,8 +3098,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;
diff --git a/lib/SymbolGraphGen/SymbolGraph.cpp b/lib/SymbolGraphGen/SymbolGraph.cpp
@@ -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){
diff --git a/test/Sema/clang_fn_type_mismatch.swift b/test/Sema/clang_fn_type_mismatch.swift
@@ -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()!
diff --git a/tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp b/tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp
diff --git a/tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp b/tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp
diff --git a/tools/swift-ide-test/swift-ide-test.cpp b/tools/swift-ide-test/swift-ide-test.cpp

Original file line number	Diff line number	Diff line change
`@@ -359,7 +359,8 @@ visitDocComment(const DocComment *DC, TypeOrExtensionDecl SynthesizedTarget) {`
`359`	`359`	`}`
`360`	`360`
`361`	`361`	`{`
`362`		`- PrintOptions PO = PrintOptions::printInterface();`
	`362`	`+ PrintOptions PO = PrintOptions::printInterface(`
	`363`	`+ D->getASTContext().TypeCheckerOpts.PrintFullConvention);`
`363`	`364`	`PO.PrintAccess = false;`
`364`	`365`	`PO.AccessFilter = AccessLevel::Private;`
`365`	`366`	`PO.PrintDocumentationComments = false;`