[clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[ilya]

Summary

Replace tidy::utils::lexer::getConstQualifyingToken with a corrected and also
generalized to other qualifiers variant - getQualifyingToken.

Fixes: http://llvm.org/PR44326

[ilya]

Fix documentation in LexerUtils.h

[aaron.ballman]

LGTM with a commenting request.

clang-tools-extra/clang-tidy/utils/LexerUtils.h

95–98

Should we add a comment that this is not for getting a member function qualifier? (I am assuming that case was never intended to work, but if it does work, should we also support ref qualifiers in that case?)

[ilya]

Update LexerUtils.h documentation.

[ilya]

@aaron.ballman, thanks for reviewing. Do you mind submitting this, as I don't have commit access?

clang-tools-extra/clang-tidy/utils/LexerUtils.h

95–98

That's a good point. For the purposes that this function is currently being used, I don't see a reason to support member function qualifiers - updated the documentation accordingly.

[ilya]

Doxygen format the documentation.

[aaron.ballman]

In D71666#1795068, @ilya wrote:

@aaron.ballman, thanks for reviewing. Do you mind submitting this, as I don't have commit access?

Happy to do so, thank you for the fix! I've commit in f58f39137c6e5a324ef684b1d72bddae244aa94d

clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp

findConstToRemove(const FunctionDecl *Def,
   // source file.
   CharSourceRange FileRange = Lexer::makeFileCharRange(
       CharSourceRange::getCharRange(Def->getBeginLoc(), NameBeginLoc),
       *Result.SourceManager, Result.Context->getLangOpts());
 
   if (FileRange.isInvalid())
     return None;
 
-  return utils::lexer::getConstQualifyingToken(FileRange, *Result.Context,
-                                               *Result.SourceManager);
+  return utils::lexer::getQualifyingToken(
+      tok::kw_const, FileRange, *Result.Context, *Result.SourceManager);
 }
 
 namespace {
 
 struct CheckResult {
   // Source range of the relevant `const` token in the definition being checked.
   CharSourceRange ConstRange;
 

clang-tools-extra/clang-tidy/utils/LexerUtils.h

 
 /// Re-lex the provide \p Range and return \c false if either a macro spans
 /// multiple tokens, a pre-processor directive or failure to retrieve the
 /// next token is found, otherwise \c true.
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts);
 
-/// Assuming that ``Range`` spans a const-qualified type, returns the ``const``
-/// token in ``Range`` that is responsible for const qualification. ``Range``
-/// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
+/// Assuming that ``Range`` spans a CVR-qualified type, returns the
+/// token in ``Range`` that is responsible for the qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying
 /// tokens are found.
-llvm::Optional<Token> getConstQualifyingToken(CharSourceRange Range,
-                                              const ASTContext &Context,
-                                              const SourceManager &SM);
+/// Note: doesn't support member function qualifiers.
+llvm::Optional<Token> getQualifyingToken(tok::TokenKind TK,
+                                         CharSourceRange Range,
 
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

if (Tok->is(tok::hash))
       return true;
 
     Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts).getLocWithOffset(1);
   }
 
   return false;
 }
 
-llvm::Optional<Token> getConstQualifyingToken(CharSourceRange Range,
-                                              const ASTContext &Context,
-                                              const SourceManager &SM) {
+llvm::Optional<Token> getQualifyingToken(tok::TokenKind TK,
+                                         CharSourceRange Range,
+  assert((TK == tok::kw_const || TK == tok::kw_volatile ||
+          TK == tok::kw_restrict) &&
+         "TK is not a qualifier keyword");
   std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Range.getBegin());
   StringRef File = SM.getBufferData(LocInfo.first);
   Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
                  File.begin(), File.data() + LocInfo.second, File.end());
-  llvm::Optional<Token> FirstConstTok;
-  Token LastTokInRange;
+  llvm::Optional<Token> LastMatchBeforeTemplate;
+  llvm::Optional<Token> LastMatchAfterTemplate;
+  bool SawTemplate = false;
   Token Tok;
   while (!RawLexer.LexFromRawLexer(Tok) &&
          Range.getEnd() != Tok.getLocation() &&
          !SM.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation())) {
     if (Tok.is(tok::raw_identifier)) {
       IdentifierInfo &Info = Context.Idents.get(
           StringRef(SM.getCharacterData(Tok.getLocation()), Tok.getLength()));
       Tok.setIdentifierInfo(&Info);
       Tok.setKind(Info.getTokenID());
     }
-    if (Tok.is(tok::kw_const) && !FirstConstTok)
-      FirstConstTok = Tok;
-    LastTokInRange = Tok;
-  }
-  // If the last token in the range is a `const`, then it const qualifies the
-  // type.  Otherwise, the first `const` token, if any, is the qualifier.
-  return LastTokInRange.is(tok::kw_const) ? LastTokInRange : FirstConstTok;
+    if (Tok.is(tok::less))
+      SawTemplate = true;
+    else if (Tok.isOneOf(tok::greater, tok::greatergreater))
+      LastMatchAfterTemplate = None;
+    else if (Tok.is(TK)) {
+      if (SawTemplate)
+        LastMatchAfterTemplate = Tok;
+      else
+        LastMatchBeforeTemplate = Tok;
+    }
+  }
+  return LastMatchAfterTemplate != None ? LastMatchAfterTemplate
+                                        : LastMatchBeforeTemplate;
 }
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const T' is 'const'-qual
 // CHECK-FIXES: T p32(T t) { return t; }
 
 // However, if the return type is itself a template instantiation, Clang does
 // not consider it const-qualified without knowing `T`.
 template <typename T>
 typename std::add_const<T>::type n15(T v) { return v; }
 
+template <bool B>
+struct MyStruct {};
+
 template <typename A>
 class Klazz {
 public:
   Klazz(A) {}
 };
 
 class Clazz {
  public:
const Clazz *const p11() {
   // CHECK-FIXES: const Clazz *p11() {
   return &bar;
 }
 
 const Klazz<const int> p12() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int>'
 // CHECK-FIXES: Klazz<const int> p12() {}
 
+const Klazz<const Klazz<const int>> p33() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz<const Klazz<const int>> p33() {}
+
 const Klazz<const int>* const p13() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int> *
 // CHECK-FIXES: const Klazz<const int>* p13() {}
 
+const Klazz<const int>* const volatile p14() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int> *
+// CHECK-FIXES: const Klazz<const int>* volatile p14() {}
+
+const MyStruct<0 < 1> p34() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p34() {}
+
+MyStruct<0 < 1> const p35() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p35() {}
+
+Klazz<MyStruct<0 < 1> const> const p36() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const MyStru
+// CHECK-FIXES: Klazz<MyStruct<0 < 1> const> p36() {}
+
+const Klazz<MyStruct<0 < 1> const> *const p37() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const MyStru
+// CHECK-FIXES: const Klazz<MyStruct<0 < 1> const> *p37() {}
+
+Klazz<const MyStruct<0 < 1>> const p38() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const MyStru
+// CHECK-FIXES: Klazz<const MyStruct<0 < 1>> p38() {}
+
+const Klazz<const MyStruct<0 < 1>> p39() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz<const MyStruct<0 < 1>> p39() {}
+
+const Klazz<const MyStruct<(0 > 1)>> p40() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const MyStru
+// CHECK-FIXES: Klazz<const MyStruct<(0 > 1)>> p40() {}
+
 // re-declaration of p15.
 const int p15();
 // CHECK-FIXES: int p15();
 
 const int p15() {
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning:
 // CHECK-FIXES: int p15() {
   return 0;