[clang-tidy] Fix check for Abseil internal namespace access

[rogeeff]

Summary

This change makes following modifications:

• If reference originated from macro expansion, we report location inside of the macro instead of location where macro is referenced
• If for any reason deduced location is not correct we silently ignore it.

[Eugene.Zelenko]

Should be change mentioned in Release Notes?

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

40

Please don't use auto when type is not spelled in statement or iterator. Does variable name comply to LLVM Coding Guidelines?

[Eugene.Zelenko]

Please mark fixed comments as done.

[rogeeff]

It is my first time submitting clang-tidy change. So I'm not sure of the procedure exactly. Do we wait for something from me?

[Eugene.Zelenko]

In D72484#1817187, @rogeeff wrote:

It is my first time submitting clang-tidy change. So I'm not sure of the procedure exactly. Do we wait for something from me?

Reviewers should make comments and finally approve patch when everything would be fine. Please keep in mind that people may be busy in real life.

[lebedev.ri]

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

Is there test coverage for this? When does/can this happen?

[rogeeff]

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

At the time when I wrote this internally the test cases in abseil-no-internal-dependencies.cpp were reproducing this failure. I'm not sure this is still the case. It is possible compiler was fixed since then.

[aaron.ballman]

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

I am not certain I'm following along (sorry if I'm just being dense). Are you saying that the existing test coverage in abseil-no-internal-dependencies.cpp was failing for you internally, and that's the reason for this fix? Or are you saying that the newly-added test cases in this patch were triggering this failure?

[rogeeff]

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

Newly added test cases were triggering the failure.

[EricWF]

LGTM.

The weird template test case might is derived from an old failure that I believe was addressed in Clang, but regression tests are fine by me.
The macro expansion fix is correct and correctly tested.

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

@lebedev.ri This also happened when the source location was inside a macro expansion. Which is solved by getting the spelling loc, and there is a test case that covers it.

[aaron.ballman]

LGTM

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

43–44

Newly added test cases were triggering the failure.

Thanks for the clarification!

[EricWF]

Committed in 020ed6713d889a95f8c98d7725c87b458d99f6b3.

clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp

Finder->addMatcher(
           .bind("InternalDep"),
       this);
 }
 
 void NoInternalDependenciesCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *InternalDependency =
       Result.Nodes.getNodeAs<NestedNameSpecifierLoc>("InternalDep");
 
-  diag(InternalDependency->getBeginLoc(),
+  auto loc_at_fault =
+      Result.SourceManager->getSpellingLoc(InternalDependency->getBeginLoc());
+
+  if (!loc_at_fault.isValid())
+    return;
+
+  diag(loc_at_fault,
        "do not reference any 'internal' namespaces; those implementation "
        "details are reserved to Abseil");
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h

 class SomeContainer {};
 namespace strings_internal {
 void InternalFunction() {}
 template <class P> P InternalTemplateFunction(P a) {}
 } // namespace strings_internal
 
 namespace container_internal {
 struct InternalStruct {};
+
+template <typename T> struct InternalTemplate {};
 } // namespace container_internal
 } // namespace absl
 
 // should not trigger warnings because inside Abseil files
 void DirectAcessInternal() {
   absl::strings_internal::InternalFunction();
   absl::strings_internal::InternalTemplateFunction<std::string>("a");
 }
 
 class FriendUsageInternal {
   friend struct absl::container_internal::InternalStruct;
 };
 
 namespace absl {
 void OpeningNamespaceInternally() { strings_internal::InternalFunction(); }
 } // namespace absl

clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp

 std::string Str = absl::StringsFunction("a");
 } // namespace absl
 
 #define USE_EXTERNAL(x) absl::strings_internal::Internal##x()
 
 void MacroUse() {
   USE_INTERNAL(Function); // no-warning
   USE_EXTERNAL(Function);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+  // CHECK-MESSAGES: :[[@LINE-5]]:25: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
 }
+
+class A : absl::container_internal::InternalStruct {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template <typename T>
+class B : absl::container_internal::InternalTemplate<T> {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template <typename T> class C : absl::container_internal::InternalTemplate<T> {
+public:
+  template <typename U> static C Make(U *p) { return C{}; }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:33: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil