Diagnose possible enum common-prefix mistakes

Clang Importer strips prefixes from enum and option set case names. The logic to do this computes a common prefix from the type name and all non-deprecated case names (to oversimplify), which means that adding, removing, or changing one case can change the prefix that is removed from *all* cases. This typically causes the prefix to become shorter, meaning that additional words are prepended to each existing case name.

Existing diagnostics make it look like the case has disappeared, when in fact it still exists under a different name. A little more information may help developers to figure out what happened.

Add a tailored diagnostic for this scenario which kicks in when (a) a missing member is diagnosed, (b) the base is an imported enum or option set’s metatype, and (c) an enum case or static property exists which has the name we attempted to look up as a suffix.

Fixes rdar://116251319.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -104,6 +104,12 @@ ERROR(could_not_find_enum_case,none,
       "enum type %0 has no case %1; did you mean %2?",
       (Type, DeclNameRef, DeclName))
 
+ERROR(could_not_find_imported_enum_case,none,
+      "type %0 has no case %1, but it does have a case named %2; if %1 worked "
+      "before, a recent change to the underlying C enum may have affected how "
+      "prefixes are stripped from its case names",
+      (Type, DeclNameRef, ValueDecl *))
+
 NOTE(did_you_mean_raw_type,none,
      "did you mean to specify a raw type on the enum declaration?", ())
 
@@ -1712,6 +1718,9 @@ NOTE(fixit_add_private_for_objc_implementation,none,
 NOTE(fixit_add_final_for_objc_implementation,none,
      "add 'final' to define a Swift %0 that cannot be overridden",
      (DescriptiveDeclKind))
+NOTE(fixit_add_nonobjc_for_objc_implementation,none,
+     "add '@nonobjc' to define a Swift-only %0",
+     (DescriptiveDeclKind))
 
 ERROR(objc_implementation_wrong_category,none,
       "%kind0 should be implemented in extension for "

include/swift/Basic/StringExtras.h

@@ -223,6 +223,8 @@ namespace swift {
 
       reverse_iterator rbegin() const { return reverse_iterator(end()); }
       reverse_iterator rend() const { return reverse_iterator(begin()); }
+
+      bool hasWordStartingAt(unsigned targetPosition) const;
     };
 
     /// Retrieve the camelCase words in the given string.
@@ -236,6 +238,10 @@ namespace swift {
     /// case of the first letter.
     bool startsWithIgnoreFirstCase(StringRef word1, StringRef word2);
 
+    /// Check whether the first word ends with the second word, ignoring the
+    /// case of the first word (handles initialisms).
+    bool hasWordSuffix(StringRef haystack, StringRef needle);
+
     /// Lowercase the first word within the given camelCase string.
     ///
     /// \param string The string to lowercase.

lib/Basic/StringExtras.cpp

@@ -215,6 +215,17 @@ void WordIterator::computePrevPosition() const {
   PrevPositionValid = true;
 }
 
+bool camel_case::Words::hasWordStartingAt(unsigned targetPosition) const {
+  // Iterate over the words until we see one at or past targetPosition.
+  // FIXME: Is there a faster way to do this by looking at the characters around
+  //        the position?
+  for (auto i = begin(); i != end() && i.getPosition() <= targetPosition; i++) {
+    if (i.getPosition() == targetPosition)
+      return true;
+  }
+  return false;
+}
+
 StringRef camel_case::getFirstWord(StringRef string) {
   if (string.empty())
     return "";
@@ -249,6 +260,30 @@ bool camel_case::startsWithIgnoreFirstCase(StringRef word1, StringRef word2) {
   return word1.substr(1, word2.size() - 1) == word2.substr(1);
 }
 
+bool camel_case::hasWordSuffix(StringRef haystack, StringRef needle) {
+  // Is it even possible for one to be a suffix of the other?
+  if (needle.empty() || haystack.size() <= needle.size())
+    return false;
+
+  // Does haystack have a word boundary at the right position?
+  auto targetPosition = haystack.size() - needle.size();
+  if (!Words(haystack).hasWordStartingAt(targetPosition))
+    return false;
+
+  StringRef suffix = haystack.substr(targetPosition);
+
+  // Fast path: Without potentially copying the strings, do they match?
+  if (sameWordIgnoreFirstCase(suffix, needle))
+    return true;
+
+  // Flatten out leading initialisms. Do they match?
+  SmallString<32> suffixScratch, needleScratch;
+  auto suffixFlat = toLowercaseInitialisms(suffix, suffixScratch);
+  auto needleFlat = toLowercaseInitialisms(needle, needleScratch);
+
+  return suffixFlat == needleFlat;
+}
+
 StringRef camel_case::toLowercaseWord(StringRef string,
                                       SmallVectorImpl<char> &scratch) {
   if (string.empty())

lib/Sema/CSDiagnostics.cpp

@@ -4125,6 +4125,84 @@ DeclName MissingMemberFailure::findCorrectEnumCaseName(
   return (candidate ? candidate->getName() : DeclName());
 }
 
+/// If  \p instanceTy is an imported Clang enum, find the best imported case (if
+/// any) which has \p name as a (capitalization-adjusted) suffix.
+ValueDecl *MissingMemberFailure::
+findImportedCaseWithMatchingSuffix(Type instanceTy, DeclNameRef name) {
+  IterableDeclContext *idc = nullptr;
+
+  if (auto ED = instanceTy->getEnumOrBoundGenericEnum()) {
+    idc = ED;
+  }
+  else if (auto SD = instanceTy->getStructOrBoundGenericStruct()) {
+    // Did ClangImporter add OptionSet to this struct?
+    for (auto protoAttr :
+            SD->getAttrs().getAttributes<SynthesizedProtocolAttr>()) {
+      auto knownProto = protoAttr->getProtocol()->getKnownProtocolKind();
+      if (knownProto == KnownProtocolKind::OptionSet) {
+        idc = SD;
+        break;
+      }
+    }
+  }
+
+  if (!idc || !idc->getDecl()->hasClangNode())
+    // Not an imported clang enum.
+    return nullptr;
+
+  auto betterMatch = [](ValueDecl *a, ValueDecl *b) -> ValueDecl * {
+#define WORSE(BAD_CONDITION) do {     \
+      auto aBad = a BAD_CONDITION;    \
+      auto bBad = b BAD_CONDITION;    \
+      if (aBad > bBad)                \
+        return b;                     \
+      if (aBad < bBad)                \
+        return a;                     \
+    } while (false)
+
+    // Is one null? Return the other.
+    WORSE(== nullptr);
+
+    assert((a && b) && "neither should be null here");
+    ASTContext &ctx = a->getASTContext();
+
+    // Is one more available than the other?
+    WORSE(->getAttrs().isUnavailable(ctx));
+    WORSE(->getAttrs().getDeprecated(ctx));
+
+    // Does one have a shorter name (so the non-matching prefix is shorter)?
+    WORSE(->getName().getBaseName().userFacingName().size());
+#undef WORSE
+
+    // Arbitrary tiebreaker: compare the names.
+    if (a->getName().compare(b->getName()) > 0)
+      return b;
+    return a;
+  };
+
+  StringRef needle = name.getBaseName().userFacingName();
+
+  ValueDecl *bestMatch = nullptr;
+  for (auto member : idc->getMembers()) {
+    auto VD = dyn_cast<ValueDecl>(member);
+    if (!VD)
+      continue;
+    if (!(isa<EnumElementDecl>(VD) || isa<VarDecl>(VD)))
+      continue;
+    if (VD->isInstanceMember())
+      continue;
+    if (!VD->getInterfaceType()->isEqual(instanceTy))
+      continue;
+    if (!camel_case::hasWordSuffix(VD->getName().getBaseName().userFacingName(),
+                                   needle))
+      continue;
+
+    // This member matches. Is it the best match?
+    bestMatch = betterMatch(bestMatch, VD);
+  }
+  return bestMatch;
+}
+
 bool MissingMemberFailure::diagnoseAsError() {
   auto anchor = getRawAnchor();
   auto memberBase = getAnchor();
@@ -4243,7 +4321,16 @@ bool MissingMemberFailure::diagnoseAsError() {
       } else {
         emitBasicError(baseType);
       }
-    } else {
+    } else if (ValueDecl *bestMatch =
+                  findImportedCaseWithMatchingSuffix(instanceTy, getName())) {
+       // Sometimes Clang Importer's case prefix stripping unexpectedly
+       // changes the name of unrelated cases when someone adds a new case or
+       // changes the deprecation of an existing case. If this is such an
+       // imported type and we can find a case which has `getName()` as a
+       // suffix, mention this possibility.
+       emitDiagnostic(diag::could_not_find_imported_enum_case, instanceTy,
+                      getName(), bestMatch);
+     } else {
       emitBasicError(baseType);
     }
   } else if (auto moduleTy = baseType->getAs<ModuleType>()) {

lib/Sema/CSDiagnostics.h

@@ -1276,6 +1276,9 @@ class MissingMemberFailure : public InvalidMemberRefFailure {
   static DeclName findCorrectEnumCaseName(Type Ty,
                                           TypoCorrectionResults &corrections,
                                           DeclNameRef memberName);
+
+  static ValueDecl *findImportedCaseWithMatchingSuffix(Type instanceTy,
+                                                       DeclNameRef name);
 };
 
 class UnintendedExtraGenericParamMemberFailure final

test/ClangImporter/enum.swift

@@ -146,6 +146,27 @@ _ = NSPrefixWordBreakReordered2Custom.problemCase == .goodCase
 _ = NSPrefixWordBreakReordered2Custom.problemCase == .PrefixWordBreakReordered2DeprecatedBadCase // expected-warning {{deprecated}}
 _ = NSPrefixWordBreakReordered2Custom.problemCase == .deprecatedGoodCase // expected-warning {{deprecated}}
 
+#if !IRGEN
+_ = NSPrefixWordBreakForgotToDeprecate.newName1 // expected-error {{type 'NSPrefixWordBreakForgotToDeprecate' has no case 'newName1', but it does have a case named 'PrefixWordBreakNewName1'; if 'newName1' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+_ = NSPrefixWordBreakForgotToDeprecate.newName2 // expected-error {{type 'NSPrefixWordBreakForgotToDeprecate' has no case 'newName2', but it does have a case named 'PrefixWordBreakNewName2'; if 'newName2' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+_ = NSPrefixWordBreakForgotToDeprecate.noMatches // expected-error {{type 'NSPrefixWordBreakForgotToDeprecate' has no member 'noMatches'}}
+
+_ = NSPrefixWordBreakInvalidWord.foo // expected-error {{type 'NSPrefixWordBreakInvalidWord' has no case 'foo', but it does have a case named 'BreakFoo'; if 'foo' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+_ = NSPrefixWordBreakInvalidWord.bar // expected-error {{type 'NSPrefixWordBreakInvalidWord' has no case 'bar', but it does have a case named 'BreakBar'; if 'bar' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+_ = NSPrefixWordBreakInvalidWord.noMatches // expected-error {{type 'NSPrefixWordBreakInvalidWord' has no member 'noMatches'}}
+
+_ = NSPrefixWordBreakSuffixTieBreakers.forTest1 // expected-error {{Better}}
+_ = NSPrefixWordBreakSuffixTieBreakers.forTest2 // expected-error {{Better}}
+_ = NSPrefixWordBreakSuffixTieBreakers.forTest3 // expected-error {{Better}}
+_ = NSPrefixWordBreakSuffixTieBreakers.forTest4 // expected-error {{Better}}
+_ = NSPrefixWordBreakSuffixTieBreakers.forTest5 // expected-error {{Better}}
+
+_ = [
+  .newOption1,  // expected-error {{type 'NSPrefixWordBreakOptions.ArrayLiteralElement' (aka 'NSPrefixWordBreakOptions') has no case 'newOption1', but it does have a case named 'prefixWordBreakNewOption1'; if 'newOption1' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+  .newOption2,  // expected-error {{type 'NSPrefixWordBreakOptions.ArrayLiteralElement' (aka 'NSPrefixWordBreakOptions') has no case 'newOption2', but it does have a case named 'prefixWordBreakNewOption2'; if 'newOption2' worked before, a recent change to the underlying C enum may have affected how prefixes are stripped from its case names}}
+] as NSPrefixWordBreakOptions
+#endif
+
 _ = NSSwiftNameAllTheThings.Foo == .Bar
 _ = NSSwiftNameBad.`class`
 

test/Inputs/clang-importer-sdk/usr/include/Foundation.h

@@ -387,6 +387,47 @@ typedef NS_ENUM(NSInteger, NSPrefixWordBreakReordered2Custom) {
   NSPrefixWordBreakReordered2DeprecatedGoodCase __attribute__((deprecated)),
 };
 
+typedef NS_ENUM(NSInteger, NSPrefixWordBreakForgotToDeprecate) {
+  NSPrefixWordBreakNewName1,
+  NSPrefixWordBreakNewName2,
+  NSOldName1PrefixWordBreak = NSPrefixWordBreakNewName1, // should have been deprecated
+};
+
+typedef NS_ENUM(NSInteger, NSPrefixWordBreakInvalidWord) {
+  NSPrefixWordBreakFoo,
+  NSPrefixWordBreakBar,
+  NSPrefixWordBreak42,  // expected prefix would have left an invalid identifier
+};
+
+// Check tiebreaking rules. In each case, the diagnostic should choose the 'Better' case over the 'Worse' case.
+typedef NS_ENUM(NSInteger, NSPrefixWordBreakSuffixTieBreakers) {
+  // Available preferred over unavailable.
+  NSPrefixWordBreakBetterForTest1,
+  NSPrefixWordBreakWorseForTest1 __attribute__((unavailable)),
+
+  // Un-deprecated preferred over deprecated.
+  NSPrefixWordBreakBetterForTest2,
+  NSPrefixWordBreakWorseForTest2 __attribute__((deprecated)),
+
+  // Shorter preferred over longer (it's a closer match).
+  NSPrefixWordBreakBetterForTest3,
+  NSPrefixWordBreakWorseXXXForTest3,
+
+  // Alphabetically first preferred over second (arbitrary tiebreaker).
+  NSPrefixWordBreakBetterForTest4,
+  NSPrefixWordBreakWorseXForTest4,
+
+  // Make sure we're not choosing based on ordering.
+  NSPrefixWordBreakWorseXForTest5,
+  NSPrefixWordBreakBetterForTest5,
+};
+
+typedef NS_OPTIONS(NSInteger, NSPrefixWordBreakOptions) {
+  NSPrefixWordBreakNewOption1 = 0x1,
+  NSPrefixWordBreakNewOption2 = 0x2,
+  NSOldOption1PrefixWordBreak = NSPrefixWordBreakNewOption1, // should have been deprecated
+};
+
 typedef NS_ENUM(NSInteger, NSSwiftNameAllTheThings) {
   NSSwiftNameAllTheThingsA __attribute__((swift_name("Foo"))),
   NSSwiftNameAllTheThingsB __attribute__((swift_name("Bar"))),