[clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck

[berenm]

Summary

This diff requires http://reviews.llvm.org/D13079 to be applied first. I wasn't sure about how to make patch series in Phabricator, and I wanted to keep the two separate for clarity.

It looks like that most cases can be supported with this patch. I'm not totally sure about the actual coverage though. I think that the matchers are very generic, but I'm still not totally fluent with the AST.

[berenm]

Remove remaining commented code.

[berenm]

Fix incorrect line removal in unit test.

[berenm]

Update the diff with more context.

[berenm]

• Do not check for identifier names from system headers
• Check for SourceLocation validity before modifying them

[alexfh]

Awesome! This makes the check far more usable.

See a few minor comments in-line.

clang-tidy/readability/IdentifierNamingCheck.cpp

140

ClangTidy takes care of filtering errors. It doesn't show errors in system headers by default, but there's an option to show them, if needed (e.g. for library developers). Is there any reason to add filtering here as well?

537

Please split return and addUsage here and in other places. It looks rather confusing to me and adds no benefits.

test/clang-tidy/readability-identifier-naming.cpp

69

Please don't #include system headers to tests. There are multiple reasons to avoid this:

• that won't work in some test configurations;
• we don't control the contents of system headers, so we can't make tests reliable;
• testing time can increase significantly due to the possibly large size of system headers.

If you need to test includes, put mock headers to test/clang-tidy/Inputs/<check-name>/ and specify this directory in the clang-tidy invocation:

// RUN: %python %S/check_clang_tidy.py %s readability-identifier-naming %t \
// RUN:   -config=... \
// RUN:   -- -std=...  -isystem=%S/Inputs/readability-identifier-naming

[berenm]

Reworked the patches a little bit, addressing the comments and adding
better handling of checks in header files with corresponding unit test cases.

[berenm]

clang-tidy/readability/IdentifierNamingCheck.cpp

141

I wasn't sure how system headers were handled, and I thought it was a waste of time to check identifiers in them. But as you say it could be useful, I rolled back the change.

From what I've seen, clang-tidy will display the warnings from incorrect declaration names in system header files when using the -system-headers and -header-filter=.* flags, but will not offer fixes for the declaration or usages in user's code. I guess editing system headers isn't going to work fine?

For user headers, warnings and fixes are emitted when -header-filter=.* is used.

Without the flag, no warnings or fixes are emitted, which is expected.

[berenm]

I think the latest version also addresses D13090 in a better way. The check will now let clang-tidy decide whether warnings and fixes are displayed, whether it is in or from system/user header files or in main file, and will only worry about not emitting warnings and fixes if a macro is involved in the declaration or any usage.

[alexfh]

clang-tidy/readability/IdentifierNamingCheck.cpp

537

There are cases where this will fail (~ ClassName() or ??-ClassName or ~\<EOL>ClassName), but I saw these only a couple of times in real code. In other similar cases I'd recommend using the lexer and skip to the next token, but here it seems to be an overkill.

545

The four cases are too similar. It should be possible to write the code much shorter. This might work:

if (isa<TagTypeLoc>(Loc) || isa<InjectedClassNameTypeLoc>(Loc) || ...)
  addUsage(NamingCheckFailures, Loc->getType()->getDecl(),
      Loc->getSourceRange(), Result.SourceManager);

575

Please remove = SourceRange.

578

Can you just cast to TemplateDecl or RedeclarableTemplateDecl, whichever suits better?

[berenm]

clang-tidy/readability/IdentifierNamingCheck.cpp

545

It doesn't look to work well with TypeLoc. This looks to be a convoluted class hierarchy (http://clang.llvm.org/doxygen/classclang_1_1TypeLoc.html) and getDecl() is only available on some derived TypeLoc classes.

I can change to something smaller, is that any better?

NamedDecl *Decl = nullptr;
if (const auto &Ref = Loc->getAs<TagTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<InjectedClassNameTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<UnresolvedUsingTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<TemplateTypeParmTypeLoc>()) {
  Decl = Ref.getDecl();
}

if (Decl) {
  addUsage(NamingCheckFailures, Decl, Loc->getSourceRange(),
           Result.SourceManager);
  return;
}

[berenm]

Address the latest comments, as well as useless code removal.

• Only the start of the token range is stored, so there is no need to care about the end of the range. Remove the code that was trying to match the end of the range (template, nested name).
• Add some spaces in test cases (destructors, templates) just to be sure that everything works as expected.
• Add a missing CHECK-FIXES in the test case on the abstract_class destructor.

[berenm]

Fix arcanist messup...

[alexfh]

The patch looks good with one comment. I'll fix it and submit the patch for you.

Thank you for working on this!

clang-tidy/readability/IdentifierNamingCheck.cpp

548

Note, that I suggested to use Loc->getType()->getDecl(), which uses TypeLoc::getType() and Type::getDecl().

[alexfh]

clang-tidy/readability/IdentifierNamingCheck.cpp

548

Ah, I see now: the problem is not TypeLoc here, it's that there's no Type::getDecl. So your version is probably the best we can do here.

[alexfh]

Also note, that in October I'll be mostly unavailable, but other clang-tidy contributors can do the review and help getting patches in: sbenza, bkramer, aaron.ballman.

[berenm]

Alright, thank you for your time!

clang-tidy/readability/IdentifierNamingCheck.cpp

   // enclosing context (namespace, class, etc).
 
   Finder->addMatcher(namedDecl().bind("decl"), this);
-  Finder->addMatcher(declRefExpr().bind("declref"), this);
+  Finder->addMatcher(declRefExpr().bind("declRef"), this);
+  Finder->addMatcher(cxxConstructorDecl().bind("classRef"), this);
+  Finder->addMatcher(cxxDestructorDecl().bind("classRef"), this);
+  Finder->addMatcher(typeLoc().bind("typeLoc"), this);
+  Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
 }
 
 static bool matchesStyle(StringRef Name,

       return;
   }
 
+  // llvm::errs() << Range.getBegin().printToString(*SM) << "\n  -"
+  //              << Range.getEnd().printToString(*SM) << "\n";
+
   Failure.Usages.push_back(Range);
   Failure.ShouldFix = Failure.ShouldFix && SM->isInMainFile(Range.getBegin()) &&
                       SM->isInMainFile(Range.getEnd()) &&

 }
 
 void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
-  if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref")) {
+  if (const auto *Decl =
+          Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
+    if (Decl->isImplicit())
+      return;
+
+    return addUsage(NamingCheckFailures, Decl->getParent(),
+                    Decl->getNameInfo().getSourceRange(), Result.SourceManager);
+  }
+
+  if (const auto *Decl =
+          Result.Nodes.getNodeAs<CXXDestructorDecl>("classRef")) {
+    if (Decl->isImplicit())
+      return;
+
+    SourceRange Range = Decl->getNameInfo().getSourceRange();
+    Range.setBegin(Range.getBegin().getLocWithOffset(1));
+    return addUsage(NamingCheckFailures, Decl->getParent(), Range,
+                    Result.SourceManager);
+  }
+
+  if (const auto *Loc = Result.Nodes.getNodeAs<TypeLoc>("typeLoc")) {
+    if (const auto &Ref = Loc->getAs<TagTypeLoc>()) {
+      return addUsage(NamingCheckFailures, Ref.getDecl(), Loc->getSourceRange(),
+                      Result.SourceManager);
+    }
+
+    if (const auto &Ref = Loc->getAs<InjectedClassNameTypeLoc>()) {
+      return addUsage(NamingCheckFailures, Ref.getDecl(), Loc->getSourceRange(),
+                      Result.SourceManager);
+    }
+
+    if (const auto &Ref = Loc->getAs<UnresolvedUsingTypeLoc>()) {
+      return addUsage(NamingCheckFailures, Ref.getDecl(), Loc->getSourceRange(),
+                      Result.SourceManager);
+    }
+
+    if (const auto &Ref = Loc->getAs<TemplateTypeParmTypeLoc>()) {
+      return addUsage(NamingCheckFailures, Ref.getDecl(), Loc->getSourceRange(),
+                      Result.SourceManager);
+    }
+
+    if (const auto &Ref = Loc->getAs<TemplateSpecializationTypeLoc>()) {
+      const auto *Decl =
+          Ref.getTypePtr()->getTemplateName().getAsTemplateDecl();
+      SourceRange Range = SourceRange(Ref.getTemplateNameLoc(),
+                                      Ref.getLAngleLoc().getLocWithOffset(-1));
+
+      if (const auto *ClassDecl = dyn_cast<ClassTemplateDecl>(Decl)) {
+        return addUsage(NamingCheckFailures, ClassDecl->getTemplatedDecl(),
+                        Range, Result.SourceManager);
+      }
+
+      if (const auto *ClassDecl = dyn_cast<FunctionTemplateDecl>(Decl)) {
+        return addUsage(NamingCheckFailures, ClassDecl->getTemplatedDecl(),
+                        Range, Result.SourceManager);
+      }
+
+      if (const auto *ClassDecl = dyn_cast<TypeAliasTemplateDecl>(Decl)) {
+        return addUsage(NamingCheckFailures, ClassDecl->getTemplatedDecl(),
+                        Range, Result.SourceManager);
+      }
+
+      if (const auto *ClassDecl = dyn_cast<VarTemplateDecl>(Decl)) {
+        return addUsage(NamingCheckFailures, ClassDecl->getTemplatedDecl(),
+                        Range, Result.SourceManager);
+      }
+    }
+
+    if (const auto &Ref =
+            Loc->getAs<DependentTemplateSpecializationTypeLoc>()) {
+      return addUsage(NamingCheckFailures, Ref.getTypePtr()->getAsTagDecl(),
+                      Loc->getSourceRange(), Result.SourceManager);
+    }
+  }
+
+  if (const auto *Loc =
+          Result.Nodes.getNodeAs<NestedNameSpecifierLoc>("nestedNameLoc")) {
+    if (NestedNameSpecifier *Spec = Loc->getNestedNameSpecifier()) {
+      if (NamespaceDecl *Decl = Spec->getAsNamespace()) {
+        SourceRange Range = Loc->getLocalSourceRange();
+        Range.setEnd(Range.getEnd().getLocWithOffset(-1));
+        return addUsage(NamingCheckFailures, Decl, Range, Result.SourceManager);
+      }
+    }
+  }
+
+  if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
     return addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
                     Result.SourceManager);

     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
+    if (isa<ClassTemplateSpecializationDecl>(Decl))
+      return;
+
     StyleKind SK = findStyleKind(Decl, NamingStyles);
     if (SK == SK_Invalid)
       return;

test/clang-tidy/readability-identifier-naming.cpp

 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_class'
 // CHECK-FIXES: {{^}}class CMyClass {{{$}}
     my_class();
+// CHECK-FIXES: {{^}}    CMyClass();{{$}}
+    ~my_class();
+// CHECK-FIXES: {{^}}    ~CMyClass();{{$}}
 
   const int MEMBER_one_1 = ConstExpr_variable;
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: invalid case style for constant member 'MEMBER_one_1'

 
 const int my_class::classConstant = 4;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class constant 'classConstant'
-// CHECK-FIXES: {{^}}const int my_class::kClassConstant = 4;{{$}}
-// FIXME: The fixup should reflect class name fixups as well:
-// FIXME: {{^}}const int CMyClass::kClassConstant = 4;{{$}}
+// CHECK-FIXES: {{^}}const int CMyClass::kClassConstant = 4;{{$}}
 
 int my_class::ClassMember_2 = 5;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class member 'ClassMember_2'
-// CHECK-FIXES: {{^}}int my_class::ClassMember2 = 5;{{$}}
-// FIXME: The fixup should reflect class name fixups as well:
-// FIXME: {{^}}int CMyClass::ClassMember2 = 5;{{$}}
+// CHECK-FIXES: {{^}}int CMyClass::ClassMember2 = 5;{{$}}
+
+class my_derived_class : public virtual my_class {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_derived_class'
+// CHECK-FIXES: {{^}}class CMyDerivedClass : public virtual CMyClass {};{{$}}
+
+class CMyWellNamedClass {};
+// No warning expected as this class is well named.
+
+template<typename T>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for type template parameter 'T'
+// CHECK-FIXES: {{^}}template<typename t_t>{{$}}
+class my_templated_class : CMyWellNamedClass {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_templated_class'
+// CHECK-FIXES: {{^}}class CMyTemplatedClass : CMyWellNamedClass {};{{$}}
+
+template<typename T>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for type template parameter 'T'
+// CHECK-FIXES: {{^}}template<typename t_t>{{$}}
+class my_other_templated_class : my_templated_class<my_class>, private my_derived_class {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_other_templated_class'
+// CHECK-FIXES: {{^}}class CMyOtherTemplatedClass : CMyTemplatedClass<CMyClass>, private CMyDerivedClass {};{{$}}
+
+template<typename t_t>
+using MYSUPER_TPL = my_other_templated_class<::FOO_NS::my_class>;
+// CHECK-FIXES: {{^}}using MYSUPER_TPL = CMyOtherTemplatedClass<::foo_ns::CMyClass>;{{$}}
 
 const int global_Constant = 6;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for global constant 'global_Constant'

 void Global_Fun(TYPE_parameters... PARAMETER_PACK) {
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for global function 'Global_Fun'
 // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: invalid case style for parameter pack 'PARAMETER_PACK'
-// CHECK-FIXES: {{^}}void GlobalFun(TYPE_parameters... parameterPack) {{{$}}
+// CHECK-FIXES: {{^}}void GlobalFun(typeParameters_t... parameterPack) {{{$}}
     global_function(1, 2);
 // CHECK-FIXES: {{^}}    GlobalFunction(1, 2);{{$}}
     FOO_bar = Global_variable;

     void non_Virtual_METHOD() {}
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for private method 'non_Virtual_METHOD'
 // CHECK-FIXES: {{^}}    void __non_Virtual_METHOD() {}{{$}}
+
+public:
     static void CLASS_METHOD() {}
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for class method 'CLASS_METHOD'
 // CHECK-FIXES: {{^}}    static void classMethod() {}{{$}}

 
 struct THIS___Structure {
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for struct 'THIS___Structure'
-// CHECK-FIXES: {{^}}struct this_structure {{{$}}
+// CHECK-FIXES: {{^}}    this_structure();{{$}}
     THIS___Structure();
-// FIXME: There should be a fixup for the constructor as well
-// FIXME: {{^}}    this_structure();{{$}}
 
   union __MyUnion_is_wonderful__ {};
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: invalid case style for union '__MyUnion_is_wonderful__'

 
 typedef THIS___Structure struct_type;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for typedef 'struct_type'
-// CHECK-FIXES: {{^}}typedef THIS___Structure struct_type_t;{{$}}
-// FIXME: The fixup should reflect structure name fixups as well:
-// FIXME: {{^}}typedef this_structure struct_type_t;{{$}}
+// CHECK-FIXES: {{^}}typedef this_structure struct_type_t;{{$}}
 
 static void static_Function() {
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for function 'static_Function'
 // CHECK-FIXES: {{^}}static void staticFunction() {{{$}}
+
+  ::FOO_NS::InlineNamespace::abstract_class::CLASS_METHOD();
+// CHECK-FIXES: {{^}}  ::foo_ns::inline_namespace::AAbstractClass::classMethod();{{$}}
+  ::FOO_NS::InlineNamespace::static_Function();
+// CHECK-FIXES: {{^}}  ::foo_ns::inline_namespace::staticFunction();{{$}}
+
+  using ::FOO_NS::InlineNamespace::CE_function;
+// CHECK-FIXES: {{^}}  using ::foo_ns::inline_namespace::CE_function;{{$}}
+// FIXME: The fixup should reflect CE_function fixups as well:
+// FIXME: {{^}}  using ::foo_ns::inline_namespace::ce_function;{{$}}
 }
 
 }