Diagnose ref-qualifiers occuring after virt-specifier-seq and generate fixit hints

[• ehsan]

Summary

Follow-up to the fix of PR22075.

[rsmith]

Looks good, thanks!

lib/Parse/ParseDecl.cpp

5319–5321

Remove braces here, please.

lib/Parse/ParseDeclCXX.cpp

2100–2139

Maybe only produce a single diagnostic no matter how many specifiers were provided? You can then also use FixItHint::CreateInsertionFromRange to produce a fixit that copies the complete range of specifiers from after the virt-specifier to before (this will preserve things like macro names and whitespace between the tokens).

[• ehsan]

lib/Parse/ParseDeclCXX.cpp

2100–2139

Can you please tell me how that would work? My fixit hints are careful so that if you have something like: void foo() override final &; you would get void foo() final & override; whereas if you have void foo() final override final &; you would get void foo() final & override; (IOW you won't get two final keywords.)

I don't know how to do the same with a range based insertion fixit hint...

include/clang/Basic/DiagnosticParseKinds.td

def warn_attribute_on_function_definition : Warning<
   InGroup<GccCompat>;
 def warn_gcc_attribute_location : Warning<
   "GCC does not allow an attribute in this position on a function declaration">, 
   InGroup<GccCompat>;
 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup<IgnoredAttributes>;
 def err_expected_method_body : Error<"expected method body">;
+def err_declspec_after_virtspec : Error<
+  "'%0' qualifier may not appear after the virtual specifier '%1'">;
 def err_invalid_token_after_toplevel_declarator : Error<
   "expected ';' after top level declarator">;
 def err_invalid_token_after_declarator_suggest_equal : Error<
   "invalid %0 at end of declaration; did you mean '='?">;
 def err_expected_statement : Error<"expected statement">;
 def err_expected_lparen_after : Error<"expected '(' after '%0'">;
 def err_expected_rparen_after : Error<"expected ')' after '%0'">;
 def err_expected_punc : Error<"expected ')' or ',' after '%0'">;

include/clang/Parse/Parser.h

void ParseTypeQualifierListOpt(DeclSpec &DS,
                                  bool IdentifierRequired = false);
   void ParseDirectDeclarator(Declarator &D);
   void ParseParenDeclarator(Declarator &D);
   void ParseFunctionDeclarator(Declarator &D,
                                ParsedAttributes &attrs,
                                BalancedDelimiterTracker &Tracker,
                                bool IsAmbiguous,
                                bool RequiresArg = false);
+  bool ParseRefQualifier(bool &RefQualifierIsLValueRef,
+                         SourceLocation &RefQualifierLoc);
   bool isFunctionDeclaratorIdentifierList();
   void ParseFunctionDeclaratorIdentifierList(
          Declarator &D,
          SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo);
   void ParseParameterDeclarationClause(
          Declarator &D,
          ParsedAttributes &attrs,
          SmallVectorImpl<DeclaratorChunk::ParamInfo> &ParamInfo,
void ParseCXXMemberSpecification(SourceLocation StartLoc,
                                    unsigned TagType,
                                    Decl *TagDecl);
   ExprResult ParseCXXMemberInitializer(Decl *D, bool IsFunction,
                                        SourceLocation &EqualLoc);
   void ParseCXXMemberDeclaratorBeforeInitializer(Declarator &DeclaratorInfo,
                                                  VirtSpecifiers &VS,
                                                  ExprResult &BitfieldSize,
                                                  LateParsedAttrList &LateAttrs);
+  void MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(Declarator &D,
+                                                               VirtSpecifiers &VS);
   void ParseCXXClassMemberDeclaration(AccessSpecifier AS, AttributeList *Attr,
                   const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo(),
                   ParsingDeclRAIIObject *DiagsFromTParams = nullptr);
   void ParseConstructorInitializer(Decl *ConstructorDecl);
   MemInitResult ParseMemInitializer(Decl *ConstructorDecl);
   void HandleMemberFunctionDeclDelays(Declarator& DeclaratorInfo,
                                       Decl *ThisDecl);
 

include/clang/Sema/DeclSpec.h

 public:
   enum Specifier {
     VS_None = 0,
     VS_Override = 1,
     VS_Final = 2,
     VS_Sealed = 4
   };
 
-  VirtSpecifiers() : Specifiers(0) { }
+  VirtSpecifiers() : Specifiers(0), LastSpecifier(VS_None) { }
 
   bool SetSpecifier(Specifier VS, SourceLocation Loc,
                     const char *&PrevSpec);
 
   bool isUnset() const { return Specifiers == 0; }
 
   bool isOverrideSpecified() const { return Specifiers & VS_Override; }
   SourceLocation getOverrideLoc() const { return VS_overrideLoc; }
 
   bool isFinalSpecified() const { return Specifiers & (VS_Final | VS_Sealed); }
   bool isFinalSpelledSealed() const { return Specifiers & VS_Sealed; }
   SourceLocation getFinalLoc() const { return VS_finalLoc; }
 
   void clear() { Specifiers = 0; }
 
   static const char *getSpecifierName(Specifier VS);
 
+  SourceLocation getFirstLocation() const { return FirstLocation; }
   SourceLocation getLastLocation() const { return LastLocation; }
+  Specifier getLastSpecifier() const { return LastSpecifier; }
   
 private:
   unsigned Specifiers;
+  Specifier LastSpecifier;
 
   SourceLocation VS_overrideLoc, VS_finalLoc;
+  SourceLocation FirstLocation;
   SourceLocation LastLocation;
 };
 
 /// \brief Represents a complete lambda introducer.
 struct LambdaIntroducer {
   /// \brief An individual capture in a lambda introducer.
   struct LambdaCapture {
     LambdaCaptureKind Kind;

lib/Parse/ParseDecl.cpp

if (isFunctionDeclaratorIdentifierList()) {
     Tracker.consumeClose();
     RParenLoc = Tracker.getCloseLocation();
     LocalEndLoc = RParenLoc;
     EndLoc = RParenLoc;
 
     if (getLangOpts().CPlusPlus) {
       // FIXME: Accept these components in any order, and produce fixits to
       // correct the order if the user gets it wrong. Ideally we should deal
-      // with the virt-specifier-seq and pure-specifier in the same way.
+      // with the pure-specifier in the same way.
 
       // Parse cv-qualifier-seq[opt].
       ParseTypeQualifierListOpt(DS, AR_NoAttributesParsed,
                                 /*AtomicAllowed*/ false);
       if (!DS.getSourceRange().getEnd().isInvalid()) {
         EndLoc = DS.getSourceRange().getEnd();
         ConstQualifierLoc = DS.getConstSpecLoc();
         VolatileQualifierLoc = DS.getVolatileSpecLoc();
         RestrictQualifierLoc = DS.getRestrictSpecLoc();
       }
 
       // Parse ref-qualifier[opt].
-      if (Tok.is(tok::amp) || Tok.is(tok::ampamp)) {
-        Diag(Tok, getLangOpts().CPlusPlus11 ?
-             diag::warn_cxx98_compat_ref_qualifier :
-             diag::ext_ref_qualifier);
-
-        RefQualifierIsLValueRef = Tok.is(tok::amp);
-        RefQualifierLoc = ConsumeToken();
+      if (ParseRefQualifier(RefQualifierIsLValueRef, RefQualifierLoc)) {
         EndLoc = RefQualifierLoc;
       }
 
       // C++11 [expr.prim.general]p3:
       //   If a declaration declares a member function or member function
       //   template of a class X, the expression this is a prvalue of type
       //   "pointer to cv-qualifier-seq X" between the optional cv-qualifer-seq
       //   and the end of the function-definition, member-declarator, or
       //   declarator.
       // FIXME: currently, "static" case isn't handled correctly.
D.AddTypeInfo(DeclaratorChunk::getFunction(HasProto,
                                              NoexceptExpr.isUsable() ?
                                                NoexceptExpr.get() : nullptr,
                                              ExceptionSpecTokens,
                                              StartLoc, LocalEndLoc, D,
                                              TrailingReturnType),
                 FnAttrs, EndLoc);
 }
 
+/// ParseRefQualifier - Parses a member function ref-qualifier. Returns
+/// true if a ref-qualifier is found.
+bool Parser::ParseRefQualifier(bool &RefQualifierIsLValueRef,
+                               SourceLocation &RefQualifierLoc) {
+  if (Tok.is(tok::amp) || Tok.is(tok::ampamp)) {
+    Diag(Tok, getLangOpts().CPlusPlus11 ?
+         diag::warn_cxx98_compat_ref_qualifier :
+         diag::ext_ref_qualifier);
+
+    RefQualifierIsLValueRef = Tok.is(tok::amp);
+    RefQualifierLoc = ConsumeToken();
+    return true;
+  }
+  return false;
+}
+
 /// isFunctionDeclaratorIdentifierList - This parameter list may have an
 /// identifier list form for a K&R-style function:  void foo(a,b,c)
 ///
 /// Note that identifier-lists are only allowed for normal declarators, not for
 /// abstract-declarators.
 bool Parser::isFunctionDeclaratorIdentifierList() {
   return !getLangOpts().CPlusPlus
          && Tok.is(tok::identifier)

lib/Parse/ParseDeclCXX.cpp

else
     DeclaratorInfo.SetIdentifier(nullptr, Tok.getLocation());
 
   if (!DeclaratorInfo.isFunctionDeclarator() && TryConsumeToken(tok::colon)) {
     assert(DeclaratorInfo.isPastIdentifier() &&
            "don't know where identifier would go yet?");
     BitfieldSize = ParseConstantExpression();
     if (BitfieldSize.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
-  } else
+  } else {
     ParseOptionalCXX11VirtSpecifierSeq(
         VS, getCurrentClass().IsInterface,
         DeclaratorInfo.getDeclSpec().getFriendSpecLoc());
+    if (!VS.isUnset())
+      MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(DeclaratorInfo, VS);
+  }
 
   // If a simple-asm-expr is present, parse it.
   if (Tok.is(tok::kw_asm)) {
     SourceLocation Loc;
     ExprResult AsmLabel(ParseSimpleAsm(&Loc));
     if (AsmLabel.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
 
if (!VS.isUnset()) {
       // If we saw any GNU-style attributes that are known to GCC followed by a
       // virt-specifier, issue a GCC-compat warning.
       const AttributeList *Attr = DeclaratorInfo.getAttributes();
       while (Attr) {
         if (Attr->isKnownToGCC() && !Attr->isCXX11Attribute())
           Diag(Attr->getLoc(), diag::warn_gcc_attribute_location);
         Attr = Attr->getNext();
       }
-    }
-  }
+      MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(DeclaratorInfo, VS);
+    }
+
+/// \brief Look for declaration specifiers possibly occurring after C++11
+/// virt-specifier-seq and diagnose them.
+void Parser::MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(
+    Declarator &D,
+    VirtSpecifiers &VS) {
+  DeclSpec DS(AttrFactory);
+
+  // GNU-style and C++11 attributes are not allowed here, but they will be
+  // handled by the caller.  Diagnose everything else.
+  ParseTypeQualifierListOpt(DS, AR_NoAttributesParsed, false);
+  D.ExtendWithDeclSpec(DS);
+
+  auto &Function = D.getFunctionTypeInfo();
+  if (DS.getTypeQualifiers() != DeclSpec::TQ_unspecified) {
+    auto DeclSpecCheck = [&] (DeclSpec::TQ TypeQual,
+                              const char *FixItName,
+                              SourceLocation SpecLoc,
+                              unsigned* QualifierLoc) {
+      FixItHint Insertion;
+      if (DS.getTypeQualifiers() & TypeQual) {
+        if (!(Function.TypeQuals & TypeQual)) {
+          std::string Name(FixItName);
+          Name += " ";
+          Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name.c_str());
+          Function.TypeQuals |= TypeQual;
+          *QualifierLoc = SpecLoc.getRawEncoding();
+        }
+        Diag(SpecLoc, diag::err_declspec_after_virtspec)
+          << FixItName
+          << VirtSpecifiers::getSpecifierName(VS.getLastSpecifier())
+          << FixItHint::CreateRemoval(SpecLoc)
+          << Insertion;
+      }
+    };
+    DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc(),
+                  &Function.ConstQualifierLoc);
+    DeclSpecCheck(DeclSpec::TQ_volatile, "volatile", DS.getVolatileSpecLoc(),
+                  &Function.VolatileQualifierLoc);
+    DeclSpecCheck(DeclSpec::TQ_restrict, "restrict", DS.getRestrictSpecLoc(),
+                  &Function.RestrictQualifierLoc);
+  }
+
+  // Parse ref-qualifiers.
+  bool RefQualifierIsLValueRef = true;
+  SourceLocation RefQualifierLoc;
+  if (ParseRefQualifier(RefQualifierIsLValueRef, RefQualifierLoc)) {
+    const char *Name = (RefQualifierIsLValueRef ? "& " : "&& ");
+    FixItHint Insertion = FixItHint::CreateInsertion(VS.getFirstLocation(), Name);
+    Function.RefQualifierIsLValueRef = RefQualifierIsLValueRef;
+    Function.RefQualifierLoc = RefQualifierLoc.getRawEncoding();
+
+    Diag(RefQualifierLoc, diag::err_declspec_after_virtspec)
+      << (RefQualifierIsLValueRef ? "&" : "&&")
+      << VirtSpecifiers::getSpecifierName(VS.getLastSpecifier())
+      << FixItHint::CreateRemoval(RefQualifierLoc)
+      << Insertion;
+    D.SetRangeEnd(RefQualifierLoc);
+  }
 }
 
 /// ParseCXXClassMemberDeclaration - Parse a C++ class member declaration.
 ///
 ///       member-declaration:
 ///         decl-specifier-seq[opt] member-declarator-list[opt] ';'
 ///         function-definition ';'[opt]
 ///         ::[opt] nested-name-specifier template[opt] unqualified-id ';'[TODO]

lib/Sema/DeclSpec.cpp

for (unsigned I = 0; I != 3; ++I) {
     
     if (SymbolLocations[I].isValid())
       EndLocation = SymbolLocations[I];
   }
 }
 
 bool VirtSpecifiers::SetSpecifier(Specifier VS, SourceLocation Loc,
                                   const char *&PrevSpec) {
+  if (!FirstLocation.isValid())
+    FirstLocation = Loc;
   LastLocation = Loc;
+  LastSpecifier = VS;
   
   if (Specifiers & VS) {
     PrevSpec = getSpecifierName(VS);
     return true;
   }
 
   Specifiers |= VS;
 

test/FixIt/fixit-cxx0x.cpp

namespace MisplacedParameterPack {
   void redundantEllipsisInTemplateTypeParameter(Args<int>...);
 
   template <int N...> // expected-error {{'...' must immediately precede declared identifier}}
   void misplacedEllipsisInNonTypeTemplateParameter();
 
   template <int... N...> // expected-error {{'...' must immediately precede declared identifier}}
   void redundantEllipsisInNonTypeTemplateParameter();
 }
+
+namespace MisplacedDeclAndRefSpecAfterVirtSpec {
+  struct B {
+    virtual void f();
+    virtual void f() volatile const;
+  };
+  struct D : B {
+    virtual void f() override;
+    virtual void f() override final const volatile; // expected-error {{'const' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'volatile' qualifier may not appear after the virtual specifier 'final'}}
+  };
+  struct B2 {
+    virtual void f() &;
+    virtual void f() volatile const &&;
+  };
+  struct D2 : B2 {
+    virtual void f() override &; // expected-error {{'&' qualifier may not appear after the virtual specifier 'override'}}
+    virtual void f() override final const volatile &&; //  expected-error {{'const' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'volatile' qualifier may not appear after the virtual specifier 'final'}} expected-error {{'&&' qualifier may not appear after the virtual specifier 'final'}}
+  };
+}