[Sema] Make deprecation fix-it replace all multi-parameter ObjC method slots.

[vsapsai]

Summary

Deprecation replacement can be any text but if it looks like a name of
ObjC method and has the same number of arguments as original method,
replace all slot names so after applying a fix-it you have valid code.

rdar://problem/36660853

[aaron.ballman]

clang/include/clang/Sema/DelayedDiagnostic.h

198–199 ↗

(On Diff #138779)

You can use makeArrayRef() here instead of the explicit template argument constructor.

clang/lib/Sema/SemaDeclAttr.cpp

7196 ↗

(On Diff #138779)

Do not use auto here, as the type is not explicitly spelled out in the initialization.

7207–7217 ↗

(On Diff #138779)

You can elide a lot of braces here.

[vsapsai]

clang/lib/Sema/SemaDeclAttr.cpp

7207–7217 ↗

(On Diff #138779)

Just to clarify, is it OK if if-branch is with braces but else-branch without? I.e.

if (...) {
  line 1
  line 2
} else
  the only line

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

7207–7217 ↗

(On Diff #138779)

We're consistently inconsistent here. :-) It's okay to do so, and it's okay to not do it, I only brought it up because of how many braces we could elide. It's your call, though.

[vsapsai]

Address review comments:

• Replace auto with explicit type.
• Use llvm::makeArrayRef.
• Remove curly braces around single-statement elses.

[vsapsai]

clang/lib/Sema/SemaDeclAttr.cpp

7207–7217 ↗

(On Diff #138779)

I know I like braces too much, more than code style mandates, so I removed them. Hopefully, that reflects code style better. And I tried to clang-format both options - no changes.

[aaron.ballman]

This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.

[vsapsai]

In D44589#1044350, @aaron.ballman wrote:

This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.

Thanks for the review, Aaron. I tried not to do anything stupid with MultiSourceLocation but another opinion will be definitely useful.

[erik.pilkington]

Hi Volodymyr, thanks for working on this! Overall this looks good, I just have a few nits.

clang/include/clang/Basic/SourceLocation.h

202 ↗

(On Diff #139233)

Why can't we just use an ArrayRef<SourceLocation> for this? It looks like that type already has a converting constructor from SourceLocation, so we should be able to use in in DiagnoseUseOfDecl without any noise.

clang/lib/Sema/SemaDeclAttr.cpp

6934–6938 ↗

(On Diff #139233)

Maybe tryParseReplacementObjCMethodName or something would be better? parseObjCMethodName() is pretty vague. Also the comment above should probably be a /// doxygen comment. It would be nice if you mentioned that Name originated from an availability attribute in the comment.

6959–6966 ↗

(On Diff #139233)

Might be better to add a defaulted AllowDollar parameter to isValidIdentifier() in CharInfo.h and use that rather than duplicating it here.

[vsapsai]

Address review comments:

• update comments for tryParseObjCMethodName, use isValidIdentifier.
• make MultiSourceLocation more lightweight.

I have rebased my patch, so diff between changes can be noisy.

[vsapsai]

clang/include/clang/Basic/SourceLocation.h

202 ↗

(On Diff #139233)

I've tried to go middle way this time and have MultiSourceLocation just as typedef. I modeled it after MultiExprArg which is using MultiExprArg = MutableArrayRef<Expr *>; But that approach can have different reasons and isn't necessarily applicable in this case.

My problem here is that on one hand I think MultiSourceLocation might be a useful abstraction and in that case probably Sema::BuildInstanceMessage should use this type for its parameter. On the other hand I am struggling to come up with good explanation what MultiSourceLocation is. And that's an indication it's not a good abstraction. What do you think?

clang/lib/Sema/SemaDeclAttr.cpp

6934–6938 ↗

(On Diff #139233)

I like "try" part. But not sure about "Replacement". This function doesn't care how its output will be used so I don't think it is worth mentioning.

I've updated comment for Name but not entirely satisfied with the wording. Will try to come up with something better and suggestions are welcome.

[erik.pilkington]

LGTM, this is a really nice feature!

clang/include/clang/Basic/SourceLocation.h

202 ↗

(On Diff #139233)

It does looks like this abstraction is used a lot in the sema for objective-c. I would probably rather leave it named ArrayRef<SourceLocation> because that is just as informative a name as MultiSourceLocation, but also makes it clear what the actual type is (makes it more obvious who owns the underlying array). I think this is fine either way though, if you'd rather add the typedef.

clang/lib/Sema/DelayedDiagnostic.cpp

49 ↗

(On Diff #139650)

You should probably move this assert up before you call .front()!

[vsapsai]

• More tweaks. Remove MultiSourceLocation as nobobody, including me, sees much value in it.

cfe/trunk/include/clang/Basic/CharInfo.h

LLVM_READONLY inline char toUppercase(char c) {
   if (isLowercase(c))
     return c + 'A' - 'a';
   return c;
 }
 
 
 /// Return true if this is a valid ASCII identifier.
 ///
-/// Note that this is a very simple check; it does not accept '$' or UCNs as
-/// valid identifier characters.
-LLVM_READONLY inline bool isValidIdentifier(StringRef S) {
-  if (S.empty() || !isIdentifierHead(S[0]))
+/// Note that this is a very simple check; it does not accept UCNs as valid
+/// identifier characters.
+LLVM_READONLY inline bool isValidIdentifier(StringRef S,
+                                            bool AllowDollar = false) {
+  if (S.empty() || !isIdentifierHead(S[0], AllowDollar))
     return false;
 
   for (StringRef::iterator I = S.begin(), E = S.end(); I != E; ++I)
-    if (!isIdentifierBody(*I))
+    if (!isIdentifierBody(*I, AllowDollar))
       return false;
 
   return true;
 }
 
 } // end namespace clang
 
 #endif

cfe/trunk/include/clang/Sema/DelayedDiagnostic.h

 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/Sema.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include <cassert>
 #include <cstddef>
 #include <utility>
 
 namespace clang {
public:
   DDKind Kind;
   bool Triggered;
 
   SourceLocation Loc;
 
   void Destroy();
 
   static DelayedDiagnostic makeAvailability(AvailabilityResult AR,
-                                            SourceLocation Loc,
+                                            ArrayRef<SourceLocation> Locs,
                                             const NamedDecl *ReferringDecl,
                                             const NamedDecl *OffendingDecl,
                                             const ObjCInterfaceDecl *UnknownObjCClass,
                                             const ObjCPropertyDecl  *ObjCProperty,
                                             StringRef Msg,
                                             bool ObjCPropertyAccess);
 
-
   static DelayedDiagnostic makeAccess(SourceLocation Loc,
                                       const AccessedEntity &Entity) {
     DelayedDiagnostic DD;
     DD.Kind = Access;
     DD.Triggered = false;
     DD.Loc = Loc;
     new (&DD.getAccessData()) AccessedEntity(Entity);
     return DD;
const NamedDecl *getAvailabilityOffendingDecl() const {
     return AvailabilityData.OffendingDecl;
   }
 
   StringRef getAvailabilityMessage() const {
     assert(Kind == Availability && "Not an availability diagnostic.");
     return StringRef(AvailabilityData.Message, AvailabilityData.MessageLen);
   }
 
+  ArrayRef<SourceLocation> getAvailabilitySelectorLocs() const {
+    assert(Kind == Availability && "Not an availability diagnostic.");
+    return llvm::makeArrayRef(AvailabilityData.SelectorLocs,
+                              AvailabilityData.NumSelectorLocs);
+  }
+
   AvailabilityResult getAvailabilityResult() const {
     assert(Kind == Availability && "Not an availability diagnostic.");
     return AvailabilityData.AR;
   }
 
   /// The diagnostic ID to emit.  Used like so:
   ///   Diag(diag.Loc, diag.getForbiddenTypeDiagnostic())
   ///     << diag.getForbiddenTypeOperand()

 private:
   struct AD {
     const NamedDecl *ReferringDecl;
     const NamedDecl *OffendingDecl;
     const ObjCInterfaceDecl *UnknownObjCClass;
     const ObjCPropertyDecl  *ObjCProperty;
     const char *Message;
     size_t MessageLen;
+    SourceLocation *SelectorLocs;
+    size_t NumSelectorLocs;
     AvailabilityResult AR;
     bool ObjCPropertyAccess;
   };
 
   struct FTD {
     unsigned Diagnostic;
     unsigned Argument;
     void *OperandType;

cfe/trunk/include/clang/Sema/Sema.h

ParsingClassState PushParsingClass() {
     return DelayedDiagnostics.pushUndelayed();
   }
   void PopParsingClass(ParsingClassState state) {
     DelayedDiagnostics.popUndelayed(state);
   }
 
   void redelayDiagnostics(sema::DelayedDiagnosticPool &pool);
 
-  void DiagnoseAvailabilityOfDecl(NamedDecl *D, SourceLocation Loc,
+  void DiagnoseAvailabilityOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
                                   const ObjCInterfaceDecl *UnknownObjCClass,
                                   bool ObjCPropertyAccess,
                                   bool AvoidPartialAvailabilityChecks = false);
 
   bool makeUnavailableInSystemHeader(SourceLocation loc,
                                      UnavailableAttr::ImplicitReason reason);
 
   /// \brief Issue any -Wunguarded-availability warnings in \c FD
   void DiagnoseUnguardedAvailabilityViolations(Decl *FD);
 
   //===--------------------------------------------------------------------===//
   // Expression Parsing Callbacks: SemaExpr.cpp.
 
   bool CanUseDecl(NamedDecl *D, bool TreatUnavailableAsInvalid);
-  bool DiagnoseUseOfDecl(NamedDecl *D, SourceLocation Loc,
+  bool DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
                          const ObjCInterfaceDecl *UnknownObjCClass = nullptr,
                          bool ObjCPropertyAccess = false,
                          bool AvoidPartialAvailabilityChecks = false);
   void NoteDeletedFunction(FunctionDecl *FD);
   void NoteDeletedInheritingConstructor(CXXConstructorDecl *CD);
   std::string getDeletedOrUnavailableSuffix(const FunctionDecl *FD);
   bool DiagnosePropertyAccessorMismatch(ObjCPropertyDecl *PD,
                                         ObjCMethodDecl *Getter,

cfe/trunk/lib/Sema/DelayedDiagnostic.cpp

 #include "clang/Sema/DelayedDiagnostic.h"
 #include <cstring>
 
 using namespace clang;
 using namespace sema;
 
 DelayedDiagnostic
 DelayedDiagnostic::makeAvailability(AvailabilityResult AR,
-                                    SourceLocation Loc,
+                                    ArrayRef<SourceLocation> Locs,
                                     const NamedDecl *ReferringDecl,
                                     const NamedDecl *OffendingDecl,
                                     const ObjCInterfaceDecl *UnknownObjCClass,
                                     const ObjCPropertyDecl  *ObjCProperty,
                                     StringRef Msg,
                                     bool ObjCPropertyAccess) {
+  assert(!Locs.empty());
   DelayedDiagnostic DD;
   DD.Kind = Availability;
   DD.Triggered = false;
-  DD.Loc = Loc;
+  DD.Loc = Locs.front();
   DD.AvailabilityData.ReferringDecl = ReferringDecl;
   DD.AvailabilityData.OffendingDecl = OffendingDecl;
   DD.AvailabilityData.UnknownObjCClass = UnknownObjCClass;
   DD.AvailabilityData.ObjCProperty = ObjCProperty;
   char *MessageData = nullptr;
   if (!Msg.empty()) {
     MessageData = new char [Msg.size()];
     memcpy(MessageData, Msg.data(), Msg.size());
   }
-
   DD.AvailabilityData.Message = MessageData;
   DD.AvailabilityData.MessageLen = Msg.size();
+
+  DD.AvailabilityData.SelectorLocs = new SourceLocation[Locs.size()];
+  memcpy(DD.AvailabilityData.SelectorLocs, Locs.data(),
+         sizeof(SourceLocation) * Locs.size());
+  DD.AvailabilityData.NumSelectorLocs = Locs.size();
+
   DD.AvailabilityData.AR = AR;
   DD.AvailabilityData.ObjCPropertyAccess = ObjCPropertyAccess;
   return DD;
 }
 
 void DelayedDiagnostic::Destroy() {
   switch (Kind) {
   case Access: 
     getAccessData().~AccessedEntity(); 
     break;
 
   case Availability:
     delete[] AvailabilityData.Message;
+    delete[] AvailabilityData.SelectorLocs;
     break;
 
   case ForbiddenType:
     break;
   }
 }

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

struct AttributeInsertion {
   }
   static AttributeInsertion createInsertionBefore(const NamedDecl *D) {
     return {"", D->getLocStart(), "\n"};
   }
 };
 
 } // end anonymous namespace
 
+/// Tries to parse a string as ObjC method name.
+///
+/// \param Name The string to parse. Expected to originate from availability
+/// attribute argument.
+/// \param SlotNames The vector that will be populated with slot names. In case
+/// of unsuccessful parsing can contain invalid data.
+/// \returns A number of method parameters if parsing was successful, None
+/// otherwise.
+static Optional<unsigned>
+tryParseObjCMethodName(StringRef Name, SmallVectorImpl<StringRef> &SlotNames,
+                       const LangOptions &LangOpts) {
+  // Accept replacements starting with - or + as valid ObjC method names.
+  if (!Name.empty() && (Name.front() == '-' || Name.front() == '+'))
+    Name = Name.drop_front(1);
+  if (Name.empty())
+    return None;
+  Name.split(SlotNames, ':');
+  unsigned NumParams;
+  if (Name.back() == ':') {
+    // Remove an empty string at the end that doesn't represent any slot.
+    SlotNames.pop_back();
+    NumParams = SlotNames.size();
+  } else {
+    if (SlotNames.size() != 1)
+      // Not a valid method name, just a colon-separated string.
+      return None;
+    NumParams = 0;
+  }
+  // Verify all slot names are valid.
+  bool AllowDollar = LangOpts.DollarIdents;
+  for (StringRef S : SlotNames) {
+    if (S.empty())
+      continue;
+    if (!isValidIdentifier(S, AllowDollar))
+      return None;
+  }
+  return NumParams;
+}
+
 /// Returns a source location in which it's appropriate to insert a new
 /// attribute for the given declaration \D.
 static Optional<AttributeInsertion>
 createAttributeInsertion(const NamedDecl *D, const SourceManager &SM,
                          const LangOptions &LangOpts) {
   if (isa<ObjCPropertyDecl>(D))
     return AttributeInsertion::createInsertionAfter(D);
   if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {

 /// \param OffendingDecl A related decl to \c ReferringDecl that has an
 /// availability attribute corrisponding to \c K attached to it. Note that this
 /// may not be the same as ReferringDecl, i.e. if an EnumDecl is annotated and
 /// we refer to a member EnumConstantDecl, ReferringDecl is the EnumConstantDecl
 /// and OffendingDecl is the EnumDecl.
 static void DoEmitAvailabilityWarning(Sema &S, AvailabilityResult K,
                                       Decl *Ctx, const NamedDecl *ReferringDecl,
                                       const NamedDecl *OffendingDecl,
-                                      StringRef Message, SourceLocation Loc,
+                                      StringRef Message,
+                                      ArrayRef<SourceLocation> Locs,
                                       const ObjCInterfaceDecl *UnknownObjCClass,
                                       const ObjCPropertyDecl *ObjCProperty,
                                       bool ObjCPropertyAccess) {
   // Diagnostics for deprecated or unavailable.
   unsigned diag, diag_message, diag_fwdclass_message;
   unsigned diag_available_here = diag::note_availability_specified_here;
   SourceLocation NoteLocation = OffendingDecl->getLocation();
 
   // Matches 'diag::note_property_attribute' options.
   unsigned property_note_select;
 
   // Matches diag::note_availability_specified_here.
   unsigned available_here_select_kind;
 
   VersionTuple DeclVersion;
   if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, OffendingDecl))
     DeclVersion = AA->getIntroduced();
 
   if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx))
     return;
 
+  SourceLocation Loc = Locs.front();
+
   // The declaration can have multiple availability attributes, we are looking
   // at one of them.
   const AvailabilityAttr *A = getAttrForPlatform(S.Context, OffendingDecl);
   if (A && A->isInherited()) {
     for (const Decl *Redecl = OffendingDecl->getMostRecentDecl(); Redecl;
          Redecl = Redecl->getPreviousDecl()) {
       const AvailabilityAttr *AForRedecl =
           getAttrForPlatform(S.Context, Redecl);
if (auto AL = OffendingDecl->getAttr<UnavailableAttr>()) {
       }
     }
     break;
 
   case AR_Available:
     llvm_unreachable("Warning for availability of available declaration?");
   }
 
-  CharSourceRange UseRange;
-  StringRef Replacement;
+  SmallVector<FixItHint, 12> FixIts;
   if (K == AR_Deprecated) {
+    StringRef Replacement;
     if (auto AL = OffendingDecl->getAttr<DeprecatedAttr>())
       Replacement = AL->getReplacement();
     if (auto AL = getAttrForPlatform(S.Context, OffendingDecl))
       Replacement = AL->getReplacement();
 
+    CharSourceRange UseRange;
     if (!Replacement.empty())
       UseRange =
           CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
+    if (UseRange.isValid()) {
+      if (const auto *MethodDecl = dyn_cast<ObjCMethodDecl>(ReferringDecl)) {
+        Selector Sel = MethodDecl->getSelector();
+        SmallVector<StringRef, 12> SelectorSlotNames;
+        Optional<unsigned> NumParams = tryParseObjCMethodName(
+            Replacement, SelectorSlotNames, S.getLangOpts());
+        if (NumParams && NumParams.getValue() == Sel.getNumArgs()) {
+          assert(SelectorSlotNames.size() == Locs.size());
+          for (unsigned I = 0; I < Locs.size(); ++I) {
+            if (!Sel.getNameForSlot(I).empty()) {
+              CharSourceRange NameRange = CharSourceRange::getCharRange(
+                  Locs[I], S.getLocForEndOfToken(Locs[I]));
+              FixIts.push_back(FixItHint::CreateReplacement(
+                  NameRange, SelectorSlotNames[I]));
+            } else
+              FixIts.push_back(
+                  FixItHint::CreateInsertion(Locs[I], SelectorSlotNames[I]));
+          }
+        } else
+          FixIts.push_back(FixItHint::CreateReplacement(UseRange, Replacement));
+      } else
+        FixIts.push_back(FixItHint::CreateReplacement(UseRange, Replacement));
+    }
   }
 
   if (!Message.empty()) {
-    S.Diag(Loc, diag_message) << ReferringDecl << Message
-      << (UseRange.isValid() ?
-          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
+    S.Diag(Loc, diag_message) << ReferringDecl << Message << FixIts;
     if (ObjCProperty)
       S.Diag(ObjCProperty->getLocation(), diag::note_property_attribute)
           << ObjCProperty->getDeclName() << property_note_select;
   } else if (!UnknownObjCClass) {
-    S.Diag(Loc, diag) << ReferringDecl
-      << (UseRange.isValid() ?
-          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
+    S.Diag(Loc, diag) << ReferringDecl << FixIts;
     if (ObjCProperty)
       S.Diag(ObjCProperty->getLocation(), diag::note_property_attribute)
           << ObjCProperty->getDeclName() << property_note_select;
   } else {
-    S.Diag(Loc, diag_fwdclass_message) << ReferringDecl
-      << (UseRange.isValid() ?
-          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
+    S.Diag(Loc, diag_fwdclass_message) << ReferringDecl << FixIts;
     S.Diag(UnknownObjCClass->getLocation(), diag::note_forward_class);
   }
 
   S.Diag(NoteLocation, diag_available_here)
     << OffendingDecl << available_here_select_kind;
 }
 
 static void handleDelayedAvailabilityCheck(Sema &S, DelayedDiagnostic &DD,
                                            Decl *Ctx) {
   assert(DD.Kind == DelayedDiagnostic::Availability &&
          "Expected an availability diagnostic here");
 
   DD.Triggered = true;
   DoEmitAvailabilityWarning(
       S, DD.getAvailabilityResult(), Ctx, DD.getAvailabilityReferringDecl(),
-      DD.getAvailabilityOffendingDecl(), DD.getAvailabilityMessage(), DD.Loc,
-      DD.getUnknownObjCClass(), DD.getObjCProperty(), false);
+      DD.getAvailabilityOffendingDecl(), DD.getAvailabilityMessage(),
+      DD.getAvailabilitySelectorLocs(), DD.getUnknownObjCClass(),
+      DD.getObjCProperty(), false);
 }
 
 void Sema::PopParsingDeclaration(ParsingDeclState state, Decl *decl) {
   assert(DelayedDiagnostics.getCurrentPool());
   DelayedDiagnosticPool &poppedPool = *DelayedDiagnostics.getCurrentPool();
   DelayedDiagnostics.popWithoutEmitting(state);
 
   // When delaying diagnostics to run in the context of a parsed
void Sema::redelayDiagnostics(DelayedDiagnosticPool &pool) {
   DelayedDiagnosticPool *curPool = DelayedDiagnostics.getCurrentPool();
   assert(curPool && "re-emitting in undelayed context not supported");
   curPool->steal(pool);
 }
 
 static void EmitAvailabilityWarning(Sema &S, AvailabilityResult AR,
                                     const NamedDecl *ReferringDecl,
                                     const NamedDecl *OffendingDecl,
-                                    StringRef Message, SourceLocation Loc,
+                                    StringRef Message,
+                                    ArrayRef<SourceLocation> Locs,
                                     const ObjCInterfaceDecl *UnknownObjCClass,
                                     const ObjCPropertyDecl *ObjCProperty,
                                     bool ObjCPropertyAccess) {
   // Delay if we're currently parsing a declaration.
   if (S.DelayedDiagnostics.shouldDelayDiagnostics()) {
     S.DelayedDiagnostics.add(
         DelayedDiagnostic::makeAvailability(
-            AR, Loc, ReferringDecl, OffendingDecl, UnknownObjCClass,
+            AR, Locs, ReferringDecl, OffendingDecl, UnknownObjCClass,
             ObjCProperty, Message, ObjCPropertyAccess));
     return;
   }
 
   Decl *Ctx = cast<Decl>(S.getCurLexicalContext());
   DoEmitAvailabilityWarning(S, AR, Ctx, ReferringDecl, OffendingDecl,
-                            Message, Loc, UnknownObjCClass, ObjCProperty,
+                            Message, Locs, UnknownObjCClass, ObjCProperty,
                             ObjCPropertyAccess);
 }
 
 namespace {
 
 /// Returns true if the given statement can be a body-like child of \p Parent.
 bool isBodyLikeChildStmt(const Stmt *S, const Stmt *Parent) {
   switch (Parent->getStmtClass()) {
void Sema::DiagnoseUnguardedAvailabilityViolations(Decl *D) {
   else if (auto *BD = dyn_cast<BlockDecl>(D))
     Body = BD->getBody();
 
   assert(Body && "Need a body here!");
 
   DiagnoseUnguardedAvailability(*this, D).IssueDiagnostics(Body);
 }
 
-void Sema::DiagnoseAvailabilityOfDecl(NamedDecl *D, SourceLocation Loc,
+void Sema::DiagnoseAvailabilityOfDecl(NamedDecl *D,
+                                      ArrayRef<SourceLocation> Locs,
                                       const ObjCInterfaceDecl *UnknownObjCClass,
                                       bool ObjCPropertyAccess,
                                       bool AvoidPartialAvailabilityChecks) {
   std::string Message;
   AvailabilityResult Result;
   const NamedDecl* OffendingDecl;
   // See if this declaration is unavailable, deprecated, or partial.
   std::tie(Result, OffendingDecl) = ShouldDiagnoseAvailabilityOfDecl(D, &Message);
void Sema::DiagnoseAvailabilityOfDecl(NamedDecl *D,
   if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     if (const ObjCPropertyDecl *PD = MD->findPropertyDecl()) {
       AvailabilityResult PDeclResult = PD->getAvailability(nullptr);
       if (PDeclResult == Result)
         ObjCPDecl = PD;
     }
   }
 
-  EmitAvailabilityWarning(*this, Result, D, OffendingDecl, Message, Loc,
+  EmitAvailabilityWarning(*this, Result, D, OffendingDecl, Message, Locs,
                           UnknownObjCClass, ObjCPDecl, ObjCPropertyAccess);
 }

cfe/trunk/lib/Sema/SemaExpr.cpp

 /// declarations that can occur when using a declaration. For example,
 /// it might warn if a deprecated or unavailable declaration is being
 /// used, or produce an error (and return true) if a C++0x deleted
 /// function is being used.
 ///
 /// \returns true if there was an error (this declaration cannot be
 /// referenced), false otherwise.
 ///
-bool Sema::DiagnoseUseOfDecl(NamedDecl *D, SourceLocation Loc,
+bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
                              const ObjCInterfaceDecl *UnknownObjCClass,
                              bool ObjCPropertyAccess,
                              bool AvoidPartialAvailabilityChecks) {
+  SourceLocation Loc = Locs.front();
   if (getLangOpts().CPlusPlus && isa<FunctionDecl>(D)) {
     // If there were any diagnostics suppressed by template argument deduction,
     // emit them now.
     auto Pos = SuppressedDiagnostics.find(D->getCanonicalDecl());
     if (Pos != SuppressedDiagnostics.end()) {
       for (const PartialDiagnosticAt &Suppressed : Pos->second)
         Diag(Suppressed.first, Suppressed.second);
 
bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
   if (LangOpts.OpenMP && DRD && !CurContext->containsDecl(D) &&
       isa<VarDecl>(D)) {
     Diag(Loc, diag::err_omp_wrong_var_in_declare_reduction)
         << getCurFunction()->HasOMPDeclareReductionCombiner;
     Diag(D->getLocation(), diag::note_entity_declared_at) << D;
     return true;
   }
 
-  DiagnoseAvailabilityOfDecl(D, Loc, UnknownObjCClass, ObjCPropertyAccess,
+  DiagnoseAvailabilityOfDecl(D, Locs, UnknownObjCClass, ObjCPropertyAccess,
                              AvoidPartialAvailabilityChecks);
 
   DiagnoseUnusedOfDecl(*this, D, Loc);
 
   diagnoseUseOfInternalDeclInInlineFunction(*this, D, Loc);
 
   return false;
 }

cfe/trunk/lib/Sema/SemaExprObjC.cpp

ExprResult Sema::BuildClassMessage(TypeSourceInfo *ReceiverTypeInfo,
                                    bool isImplicit) {
   SourceLocation Loc = SuperLoc.isValid()? SuperLoc
     : ReceiverTypeInfo->getTypeLoc().getSourceRange().getBegin();
   if (LBracLoc.isInvalid()) {
     Diag(Loc, diag::err_missing_open_square_message_send)
       << FixItHint::CreateInsertion(Loc, "[");
     LBracLoc = Loc;
   }
-  SourceLocation SelLoc;
+  ArrayRef<SourceLocation> SelectorSlotLocs;
   if (!SelectorLocs.empty() && SelectorLocs.front().isValid())
-    SelLoc = SelectorLocs.front();
+    SelectorSlotLocs = SelectorLocs;
   else
-    SelLoc = Loc;
+    SelectorSlotLocs = Loc;
+  SourceLocation SelLoc = SelectorSlotLocs.front();
 
   if (ReceiverType->isDependentType()) {
     // If the receiver type is dependent, we can't type-check anything
     // at this point. Build a dependent expression.
     unsigned NumArgs = ArgsIn.size();
     Expr **Args = ArgsIn.data();
     assert(SuperLoc.isInvalid() && "Message to super with dependent type");
     return ObjCMessageExpr::Create(
         Context, ReceiverType, VK_RValue, LBracLoc, ReceiverTypeInfo, Sel,
         SelectorLocs, /*Method=*/nullptr, makeArrayRef(Args, NumArgs), RBracLoc,
         isImplicit);
   }
   
   // Find the class to which we are sending this message.
   ObjCInterfaceDecl *Class = nullptr;
   const ObjCObjectType *ClassType = ReceiverType->getAs<ObjCObjectType>();
   if (!ClassType || !(Class = ClassType->getInterface())) {
     Diag(Loc, diag::err_invalid_receiver_class_message)
       << ReceiverType;
     return ExprError();
   }
   assert(Class && "We don't know which class we're messaging?");
   // objc++ diagnoses during typename annotation.
   if (!getLangOpts().CPlusPlus)
-    (void)DiagnoseUseOfDecl(Class, SelLoc);
+    (void)DiagnoseUseOfDecl(Class, SelectorSlotLocs);
   // Find the method we are messaging.
   if (!Method) {
     SourceRange TypeRange 
       = SuperLoc.isValid()? SourceRange(SuperLoc)
                           : ReceiverTypeInfo->getTypeLoc().getSourceRange();
     if (RequireCompleteType(Loc, Context.getObjCInterfaceType(Class),
                             (getLangOpts().ObjCAutoRefCount
                                ? diag::err_arc_receiver_forward_class
                                : diag::warn_receiver_forward_class),
                             TypeRange)) {
       // A forward class used in messaging is treated as a 'Class'
       Method = LookupFactoryMethodInGlobalPool(Sel, 
                                                SourceRange(LBracLoc, RBracLoc));
       if (Method && !getLangOpts().ObjCAutoRefCount)
         Diag(Method->getLocation(), diag::note_method_sent_forward_class)
           << Method->getDeclName();
     }
     if (!Method)
       Method = Class->lookupClassMethod(Sel);
 
     // If we have an implementation in scope, check "private" methods.
     if (!Method)
       Method = Class->lookupPrivateClassMethod(Sel);
 
-    if (Method && DiagnoseUseOfDecl(Method, SelLoc))
+    if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
       return ExprError();
   }
 
   // Check the argument types and determine the result type.
   QualType ReturnType;
   ExprValueKind VK = VK_RValue;
 
   unsigned NumArgs = ArgsIn.size();
ExprResult Sema::BuildInstanceMessage(Expr *Receiver,
   assert((Receiver || SuperLoc.isValid()) && "If the Receiver is null, the "
                                              "SuperLoc must be valid so we can "
                                              "use it instead.");
 
   // The location of the receiver.
   SourceLocation Loc = SuperLoc.isValid()? SuperLoc : Receiver->getLocStart();
   SourceRange RecRange =
       SuperLoc.isValid()? SuperLoc : Receiver->getSourceRange();
-  SourceLocation SelLoc;
+  ArrayRef<SourceLocation> SelectorSlotLocs;
   if (!SelectorLocs.empty() && SelectorLocs.front().isValid())
-    SelLoc = SelectorLocs.front();
+    SelectorSlotLocs = SelectorLocs;
   else
-    SelLoc = Loc;
+    SelectorSlotLocs = Loc;
+  SourceLocation SelLoc = SelectorSlotLocs.front();
 
   if (LBracLoc.isInvalid()) {
     Diag(Loc, diag::err_missing_open_square_message_send)
       << FixItHint::CreateInsertion(Loc, "[");
     LBracLoc = Loc;
   }
 
   // If we have a receiver expression, perform appropriate promotions
if (receiverIsIdLike || ReceiverType->isBlockPointerType() ||
 
         if (ObjCMethodDecl *BestMethod =
             SelectBestMethod(Sel, ArgsIn, Method->isInstanceMethod(), Methods))
           Method = BestMethod;
 
         if (!AreMultipleMethodsInGlobalPool(Sel, Method,
                                             SourceRange(LBracLoc, RBracLoc),
                                             receiverIsIdLike, Methods))
-           DiagnoseUseOfDecl(Method, SelLoc);
+          DiagnoseUseOfDecl(Method, SelectorSlotLocs);
       }
     } else if (ReceiverType->isObjCClassOrClassKindOfType() ||
                ReceiverType->isObjCQualifiedClassType()) {
       // Handle messages to Class.
       // We allow sending a message to a qualified Class ("Class<foo>"), which
       // is ok as long as one of the protocols implements the selector (if not,
       // warn).
       if (!ReceiverType->isObjCClassOrClassKindOfType()) {
if (receiverIsIdLike || ReceiverType->isBlockPointerType() ||
         if (ObjCMethodDecl *CurMeth = getCurMethodDecl()) {
           if (ObjCInterfaceDecl *ClassDecl = CurMeth->getClassInterface()) {
             // First check the public methods in the class interface.
             Method = ClassDecl->lookupClassMethod(Sel);
 
             if (!Method)
               Method = ClassDecl->lookupPrivateClassMethod(Sel);
           }
-          if (Method && DiagnoseUseOfDecl(Method, SelLoc))
+          if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
             return ExprError();
         }
         if (!Method) {
           // If not messaging 'self', look for any factory method named 'Sel'.
           if (!Receiver || !isSelfExpr(Receiver)) {
             // If no class (factory) method was found, check if an _instance_
             // method of the same name exists in the root class only.
             SmallVector<ObjCMethodDecl*, 4> Methods;
if (receiverIsIdLike || ReceiverType->isBlockPointerType() ||
       // long as one of the protocols implements the selector (if not, warn).
       // And as long as message is not deprecated/unavailable (warn if it is).
       if (const ObjCObjectPointerType *QIdTy 
                                    = ReceiverType->getAsObjCQualifiedIdType()) {
         // Search protocols for instance methods.
         Method = LookupMethodInQualifiedType(Sel, QIdTy, true);
         if (!Method)
           Method = LookupMethodInQualifiedType(Sel, QIdTy, false);
-        if (Method && DiagnoseUseOfDecl(Method, SelLoc))
+        if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
           return ExprError();
       } else if (const ObjCObjectPointerType *OCIType
                    = ReceiverType->getAsObjCInterfacePointerType()) {
         // We allow sending a message to a pointer to an interface (an object).
         ClassDecl = OCIType->getInterfaceDecl();
 
         // Try to complete the type. Under ARC, this is a hard error from which
         // we don't try to recover.
if (receiverIsIdLike || ReceiverType->isBlockPointerType() ||
               }
               if (Method && !forwardClass)
                 Diag(SelLoc, diag::warn_maynot_respond)
                   << OCIType->getInterfaceDecl()->getIdentifier()
                   << Sel << RecRange;
             }
           }
         }
-        if (Method && DiagnoseUseOfDecl(Method, SelLoc, forwardClass))
+        if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs, forwardClass))
           return ExprError();
       } else {
         // Reject other random receiver types (e.g. structs).
         Diag(Loc, diag::err_bad_receiver_type)
           << ReceiverType << Receiver->getSourceRange();
         return ExprError();
       }
     }

cfe/trunk/test/SemaObjC/attr-deprecated-replacement-fixit.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck --implicit-check-not fix-it: %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -DIGNORE_UNSUCCESSFUL_RENAMES -fixit -x objective-c %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -DIGNORE_UNSUCCESSFUL_RENAMES -Werror -x objective-c %t
+
+#if !__has_feature(attribute_deprecated_with_replacement)
+#error "Missing __has_feature"
+#endif
+
+#if !__has_feature(attribute_availability_with_replacement)
+#error "Missing __has_feature"
+#endif
+
+#define DEPRECATED(replacement) __attribute__((deprecated("message", replacement)))
+
+@protocol SuccessfulMultiParameterRenames
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)multi:(int)param1 parameter:(int)param2 replacement:(int)param3 DEPRECATED("multi_new:parameter_new:replace_new_ment:");
+- (void)multi_new:(int)param1 parameter_new:(int)param2 replace_new_ment:(int)param3;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)varArgs:(int)params, ... DEPRECATED("renameVarArgs:");
+- (void)renameVarArgs:(int)params, ...;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)leadingMinus:(int)param DEPRECATED("-leadingMinusRenamed:");
+- (void)leadingMinusRenamed:(int)param;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)leadingPlus:(int)param DEPRECATED("+leadingPlusRenamed:");
+- (void)leadingPlusRenamed:(int)param;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)sourceEmptyName:(int)param1 :(int)param2 DEPRECATED("renameEmptyName:toNonEmpty:");
+- (void)renameEmptyName:(int)param1 toNonEmpty:(int)param2;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)target:(int)param1 willBecomeEmpty:(int)param2 emptyName:(int)param3 DEPRECATED("target::emptyName:");
+- (void)target:(int)param1 :(int)param2 emptyName:(int)param3;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)extra:(int)param1 whiteSpace:(int)param2 DEPRECATED("renameExtra:whiteSpace:");
+- (void)renameExtra:(int)param1 whiteSpace:(int)param2;
+
+// Test renaming that was producing valid code earlier is still producing valid
+// code. The difference is that now we detect different number of parameters.
+//
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)singleArgumentRegression:(int)param DEPRECATED("renameSingleArgument");
+- (void)renameSingleArgument:(int)param;
+@end
+
+void successfulRenames(id<SuccessfulMultiParameterRenames> object) {
+  [object multi:0 parameter:1 replacement:2]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:16}:"multi_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:28}:"parameter_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:31-[[@LINE-3]]:42}:"replace_new_ment"
+
+  [object varArgs:1, 2, 3, 0]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:18}:"renameVarArgs"
+
+  [object leadingMinus:0]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:23}:"leadingMinusRenamed"
+
+  [object leadingPlus:0]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:22}:"leadingPlusRenamed"
+
+  [object sourceEmptyName:0 :1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:26}:"renameEmptyName"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:29-[[@LINE-2]]:29}:"toNonEmpty"
+
+  [object target:0 willBecomeEmpty:1 emptyName:2]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:17}:"target"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:20-[[@LINE-2]]:35}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:38-[[@LINE-3]]:47}:"emptyName"
+
+  [object extra: 0    whiteSpace:   1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:16}:"renameExtra"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:23-[[@LINE-2]]:33}:"whiteSpace"
+
+  [object singleArgumentRegression:0]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:35}:"renameSingleArgument"
+}
+
+#ifndef IGNORE_UNSUCCESSFUL_RENAMES
+@protocol UnsuccessfulMultiParameterRenames
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)differentNumberOfParameters:(int)param DEPRECATED("rename:hasMoreParameters:");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)differentNumber:(int)param1 ofParameters:(int)param2 DEPRECATED("renameHasLessParameters:");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)methodLike:(int)param1 replacement:(int)param2 DEPRECATED("noColon:atTheEnd");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)freeFormText DEPRECATED("Use something else");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)freeFormTextReplacementStartsAsMethod DEPRECATED("-Use something different");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)replacementHasSingleSkipCharacter DEPRECATED("-");
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)replacementHasInvalid:(int)param1 slotName:(int)param2 DEPRECATED("renameWith:1nonIdentifier:");
+@end
+
+void unsuccessfulRenames(id<UnsuccessfulMultiParameterRenames> object) {
+  [object differentNumberOfParameters:0]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:38}:"rename:hasMoreParameters:"
+
+  [object differentNumber:0 ofParameters:1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:26}:"renameHasLessParameters:"
+
+  [object methodLike:0 replacement:1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:21}:"noColon:atTheEnd"
+
+  [object freeFormText]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:23}:"Use something else"
+
+  [object freeFormTextReplacementStartsAsMethod]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:48}:"-Use something different"
+
+  [object replacementHasSingleSkipCharacter]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:44}:"-"
+
+  [object replacementHasInvalid:0 slotName:1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:32}:"renameWith:1nonIdentifier:"
+}
+#endif // IGNORE_UNSUCCESSFUL_RENAMES
+
+// Make sure classes are treated the same way as protocols.
+__attribute__((objc_root_class))
+@interface Interface
+// expected-note@+1 {{has been explicitly marked deprecated here}}
++ (void)classMethod:(int)param1 replacement:(int)param2 DEPRECATED("renameClassMethod:replace_new_ment:");
++ (void)renameClassMethod:(int)param1 replace_new_ment:(int)param2;
+
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)multi:(int)param1 parameter:(int)param2 replacement:(int)param3 DEPRECATED("multi_new:parameter_new:replace_new_ment:");
+- (void)multi_new:(int)param1 parameter_new:(int)param2 replace_new_ment:(int)param3;
+@end
+
+@implementation Interface
++ (void)classMethod:(int)param1 replacement:(int)param2 {}
++ (void)renameClassMethod:(int)param1 replace_new_ment:(int)param2 {}
+
+- (void)multi:(int)param1 parameter:(int)param2 replacement:(int)param3 {}
+- (void)multi_new:(int)param1 parameter_new:(int)param2 replace_new_ment:(int)param3 {}
+
+- (void)usage {
+  [Interface classMethod:0 replacement:1]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:25}:"renameClassMethod"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:28-[[@LINE-2]]:39}:"replace_new_ment"
+
+  [self multi:0 parameter:1 replacement:2]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:14}:"multi_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:26}:"parameter_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:40}:"replace_new_ment"
+}
+@end
+
+// Make sure availability attribute is handled the same way as deprecation attribute.
+@protocol AvailabilityAttributeRenames
+// expected-note@+1 {{has been explicitly marked deprecated here}}
+- (void)multi:(int)param1 parameter:(int)param2 replacement:(int)param3 __attribute__((availability(macosx,deprecated=9.0,replacement="multi_new:parameter_new:replace_new_ment:")));
+- (void)multi_new:(int)param1 parameter_new:(int)param2 replace_new_ment:(int)param3;
+@end
+
+void availabilityAttributeRenames(id<AvailabilityAttributeRenames> object) {
+  [object multi:0 parameter:1 replacement:2]; // expected-warning {{is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:16}:"multi_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:28}:"parameter_new"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:31-[[@LINE-3]]:42}:"replace_new_ment"
+}

cfe/trunk/unittests/Basic/CharInfoTest.cpp

TEST(CharInfoTest, isValidIdentifier) {
   EXPECT_FALSE(isValidIdentifier("$"));
   EXPECT_FALSE(isValidIdentifier("1"));
 
   EXPECT_TRUE(isValidIdentifier("_"));
   EXPECT_TRUE(isValidIdentifier("a"));
   EXPECT_TRUE(isValidIdentifier("z"));
   EXPECT_TRUE(isValidIdentifier("A"));
   EXPECT_TRUE(isValidIdentifier("Z"));
+  EXPECT_TRUE(isValidIdentifier("$", /*AllowDollar=*/true));
 
   // 2 characters, '_' suffix
   EXPECT_FALSE(isValidIdentifier("._"));
   EXPECT_FALSE(isValidIdentifier("\n_"));
   EXPECT_FALSE(isValidIdentifier(" _"));
   EXPECT_FALSE(isValidIdentifier("\x80_"));
   EXPECT_FALSE(isValidIdentifier("\xc2_"));
   EXPECT_FALSE(isValidIdentifier("\xff_"));
   EXPECT_FALSE(isValidIdentifier("$_"));
   EXPECT_FALSE(isValidIdentifier("1_"));
 
   EXPECT_TRUE(isValidIdentifier("__"));
   EXPECT_TRUE(isValidIdentifier("a_"));
   EXPECT_TRUE(isValidIdentifier("z_"));
   EXPECT_TRUE(isValidIdentifier("A_"));
   EXPECT_TRUE(isValidIdentifier("Z_"));
+  EXPECT_TRUE(isValidIdentifier("$_", /*AllowDollar=*/true));
 
   // 2 characters, '_' prefix
   EXPECT_FALSE(isValidIdentifier("_."));
   EXPECT_FALSE(isValidIdentifier("_\n"));
   EXPECT_FALSE(isValidIdentifier("_ "));
   EXPECT_FALSE(isValidIdentifier("_\x80"));
   EXPECT_FALSE(isValidIdentifier("_\xc2"));
   EXPECT_FALSE(isValidIdentifier("_\xff"));
   EXPECT_FALSE(isValidIdentifier("_$"));
   EXPECT_TRUE(isValidIdentifier("_1"));
 
   EXPECT_TRUE(isValidIdentifier("__"));
   EXPECT_TRUE(isValidIdentifier("_a"));
   EXPECT_TRUE(isValidIdentifier("_z"));
   EXPECT_TRUE(isValidIdentifier("_A"));
   EXPECT_TRUE(isValidIdentifier("_Z"));
+  EXPECT_TRUE(isValidIdentifier("_$", /*AllowDollar=*/true));
 
   // 3 characters, '__' prefix
   EXPECT_FALSE(isValidIdentifier("__."));
   EXPECT_FALSE(isValidIdentifier("__\n"));
   EXPECT_FALSE(isValidIdentifier("__ "));
   EXPECT_FALSE(isValidIdentifier("__\x80"));
   EXPECT_FALSE(isValidIdentifier("__\xc2"));
   EXPECT_FALSE(isValidIdentifier("__\xff"));
   EXPECT_FALSE(isValidIdentifier("__$"));
   EXPECT_TRUE(isValidIdentifier("__1"));
 
   EXPECT_TRUE(isValidIdentifier("___"));
   EXPECT_TRUE(isValidIdentifier("__a"));
   EXPECT_TRUE(isValidIdentifier("__z"));
   EXPECT_TRUE(isValidIdentifier("__A"));
   EXPECT_TRUE(isValidIdentifier("__Z"));
+  EXPECT_TRUE(isValidIdentifier("__$", /*AllowDollar=*/true));
 
   // 3 characters, '_' prefix and suffix
   EXPECT_FALSE(isValidIdentifier("_._"));
   EXPECT_FALSE(isValidIdentifier("_\n_"));
   EXPECT_FALSE(isValidIdentifier("_ _"));
   EXPECT_FALSE(isValidIdentifier("_\x80_"));
   EXPECT_FALSE(isValidIdentifier("_\xc2_"));
   EXPECT_FALSE(isValidIdentifier("_\xff_"));
   EXPECT_FALSE(isValidIdentifier("_$_"));
   EXPECT_TRUE(isValidIdentifier("_1_"));
 
   EXPECT_TRUE(isValidIdentifier("___"));
   EXPECT_TRUE(isValidIdentifier("_a_"));
   EXPECT_TRUE(isValidIdentifier("_z_"));
   EXPECT_TRUE(isValidIdentifier("_A_"));
   EXPECT_TRUE(isValidIdentifier("_Z_"));
+  EXPECT_TRUE(isValidIdentifier("_$_", /*AllowDollar=*/true));
 }