Merge pull request #79206 from beccadax/this-name-is-not-constructive

Diagnose and forbid invalid Swift names on inits

Author: beccadax

CHANGELOG.md

@@ -5,6 +5,11 @@
 
 ## Swift 6.1
 
+* Previous versions of Swift would incorrectly allow Objective-C `-init...`
+  methods with custom Swift names to be imported as initializers, but with base 
+  names other than `init`. The compiler now diagnoses these attributes and
+  infers a name for the initializer as though they are not present.
+
 * Projected value initializers are now correctly injected into calls when
   an argument exactly matches a parameter with an external property wrapper.
 

include/swift/AST/DiagnosticsClangImporter.def

@@ -54,6 +54,10 @@ ERROR(dump_pcm_error,Fatal,
 WARNING(invalid_swift_name_method,none,
   "too %select{few|many}0 parameters in swift_name attribute (expected %1; "
   "got %2)", (bool, unsigned, unsigned))
+WARNING(invalid_swift_name_for_decl,none,
+  "custom Swift name '%0' ignored because it is not valid for %kindonly1; "
+  "imported as %1 instead",
+  (StringRef, ValueDecl *))
 
 NOTE(note_while_importing, none, "while importing '%0'", (StringRef))
 ERROR(swift_name_protocol_static, none, "swift_name cannot be used to define "

include/swift/Frontend/DiagnosticVerifier.h

@@ -45,11 +45,13 @@ struct LineColumnRange {
 };
 
 class CapturedFixItInfo final {
+  SourceManager *diagSM;
   DiagnosticInfo::FixIt FixIt;
   mutable LineColumnRange LineColRange;
 
 public:
-  CapturedFixItInfo(DiagnosticInfo::FixIt FixIt) : FixIt(FixIt) {}
+  CapturedFixItInfo(SourceManager &diagSM, DiagnosticInfo::FixIt FixIt)
+    : diagSM(&diagSM), FixIt(FixIt) {}
 
   CharSourceRange &getSourceRange() { return FixIt.getRange(); }
   const CharSourceRange &getSourceRange() const { return FixIt.getRange(); }
@@ -58,13 +60,12 @@ class CapturedFixItInfo final {
 
   /// Obtain the line-column range corresponding to the fix-it's
   /// replacement range.
-  const LineColumnRange &getLineColumnRange(const SourceManager &SM,
-                                            unsigned BufferID) const;
+  const LineColumnRange &getLineColumnRange(SourceManager &SM) const;
 };
 
 struct CapturedDiagnosticInfo {
   llvm::SmallString<128> Message;
-  llvm::SmallString<32> FileName;
+  std::optional<unsigned> SourceBufferID;
   DiagnosticKind Classification;
   SourceLoc Loc;
   unsigned Line;
@@ -73,14 +74,14 @@ struct CapturedDiagnosticInfo {
   SmallVector<std::string, 1> EducationalNotes;
 
   CapturedDiagnosticInfo(llvm::SmallString<128> Message,
-                         llvm::SmallString<32> FileName,
+                         std::optional<unsigned> SourceBufferID,
                          DiagnosticKind Classification, SourceLoc Loc,
                          unsigned Line, unsigned Column,
                          SmallVector<CapturedFixItInfo, 2> FixIts,
                          SmallVector<std::string, 1> EducationalNotes)
-      : Message(Message), FileName(FileName), Classification(Classification),
-        Loc(Loc), Line(Line), Column(Column), FixIts(FixIts),
-        EducationalNotes(EducationalNotes) {
+      : Message(Message), SourceBufferID(SourceBufferID),
+        Classification(Classification), Loc(Loc), Line(Line), Column(Column),
+        FixIts(FixIts), EducationalNotes(EducationalNotes) {
     std::sort(EducationalNotes.begin(), EducationalNotes.end());
   }
 };
@@ -91,25 +92,23 @@ class DiagnosticVerifier : public DiagnosticConsumer {
   SourceManager &SM;
   std::vector<CapturedDiagnosticInfo> CapturedDiagnostics;
   ArrayRef<unsigned> BufferIDs;
-  SmallVector<unsigned, 4> AdditionalBufferIDs;
+  ArrayRef<std::string> AdditionalFilePaths;
   bool AutoApplyFixes;
   bool IgnoreUnknown;
   bool UseColor;
   ArrayRef<std::string> AdditionalExpectedPrefixes;
 
 public:
   explicit DiagnosticVerifier(SourceManager &SM, ArrayRef<unsigned> BufferIDs,
+                              ArrayRef<std::string> AdditionalFilePaths,
                               bool AutoApplyFixes, bool IgnoreUnknown,
                               bool UseColor,
                               ArrayRef<std::string> AdditionalExpectedPrefixes)
-      : SM(SM), BufferIDs(BufferIDs), AutoApplyFixes(AutoApplyFixes),
-        IgnoreUnknown(IgnoreUnknown), UseColor(UseColor),
+      : SM(SM), BufferIDs(BufferIDs), AdditionalFilePaths(AdditionalFilePaths),
+        AutoApplyFixes(AutoApplyFixes), IgnoreUnknown(IgnoreUnknown),
+        UseColor(UseColor),
         AdditionalExpectedPrefixes(AdditionalExpectedPrefixes) {}
 
-  void appendAdditionalBufferID(unsigned bufferID) {
-    AdditionalBufferIDs.push_back(bufferID);
-  }
-
   virtual void handleDiagnostic(SourceManager &SM,
                                 const DiagnosticInfo &Info) override;
 

lib/ClangImporter/ImportDecl.cpp

@@ -7323,6 +7323,14 @@ ConstructorDecl *SwiftDeclConverter::importConstructor(
   // If this constructor overrides another constructor, mark it as such.
   recordObjCOverride(result);
 
+  // If we ignored a custom Swift name because it wasn't suitable for an init,
+  // diagnose that now.
+  if (importedName.hasInvalidCustomName() && isActiveSwiftVersion()) {
+    if (auto customName = NameImporter::findCustomName(objcMethod, version)) {
+      result->diagnose(diag::invalid_swift_name_for_decl, *customName, result);
+    }
+  }
+
   return result;
 }
 

lib/ClangImporter/ImportName.cpp

@@ -1501,6 +1501,15 @@ static StringRef renameUnsafeMethod(ASTContext &ctx,
   return name;
 }
 
+std::optional<StringRef>
+NameImporter::findCustomName(const clang::Decl *decl,
+                             ImportNameVersion version) {
+  if (auto nameAttr = findSwiftNameAttr(decl, version)) {
+    return nameAttr->name;
+  }
+  return std::nullopt;
+}
+
 ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
                                           ImportNameVersion version,
                                           clang::DeclarationName givenName) {
@@ -1676,6 +1685,11 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
           // Note that this is an initializer.
           isInitializer = true;
         }
+      } else if (shouldImportAsInitializer(method, version, initPrefixLength)) {
+        // This is an initializer, but its custom name is ill-formed. Ignore
+        // the swift_name attribute.
+        skipCustomName = true;
+        result.info.hasInvalidCustomName = true;
       }
     }
 

lib/ClangImporter/ImportName.h

@@ -230,12 +230,16 @@ class ImportedName {
 
     unsigned hasAsyncAlternateInfo: 1;
 
+    /// Whether this declaration had a custom name that was ignored because it
+    /// was invalid.
+    unsigned hasInvalidCustomName : 1;
+
     Info()
         : errorInfo(), selfIndex(), initKind(CtorInitializerKind::Designated),
           accessorKind(ImportedAccessorKind::None), hasCustomName(false),
           droppedVariadic(false), importAsMember(false), hasSelfIndex(false),
           hasErrorInfo(false), hasAsyncInfo(false),
-          hasAsyncAlternateInfo(false) {}
+          hasAsyncAlternateInfo(false), hasInvalidCustomName(false) {}
   } info;
 
 public:
@@ -313,6 +317,10 @@ class ImportedName {
   bool hasCustomName() const { return info.hasCustomName; }
   void setHasCustomName() { info.hasCustomName = true; }
 
+  /// Whether this declaration had a custom name that was ignored because it
+  /// was invalid.
+  bool hasInvalidCustomName() const { return info.hasInvalidCustomName; }
+
   /// Whether this was one of a special class of Objective-C
   /// initializers for which we drop the variadic argument rather
   /// than refuse to import the initializer.
@@ -489,6 +497,10 @@ class NameImporter {
     importSymbolicCXXDecls = isEnabled;
   }
 
+  /// Retrieve a purported custom name even if it is invalid.
+  static std::optional<StringRef>
+  findCustomName(const clang::Decl *decl, ImportNameVersion version);
+
 private:
   bool enableObjCInterop() const { return swiftCtx.LangOpts.EnableObjCInterop; }