[clang-tidy] Fix confusable identifiers interaction with DeclContext

[serge-sans-paille]

Summary

Properly checks enclosing DeclContext, and add the related test case.

Fix #56221

[aaron.ballman]

LGTM, but I have some additional testing requests which might spawn follow-up patches.

clang-tools-extra/test/clang-tidy/checkers/misc/confusable-identifiers.cpp

42

It looks like there's quite a bit of missing test coverage for all the various situations in C++. Here are some additional test cases to add:

template <typename i1>
struct S {
  template <typename il> // Should warn on this one, right?
  void func2() {}
};

template <typename i1>
void func(int il) { // Should warn on this one, right?
}

template <typename O0>
void other() {
  int OO = 0; // Should warn on this one, right?
}
int OO = 0; // But not this one

namespace foo {
  int i1;
}

namespace bar {
  int il;  // Should not warn about this
}

namespace foo {
  int il; // But should warn about this
}

struct Base {
  virtual void O0();

private:
  void II();
};

struct Derived : Base {
  void OO(); // We should warn about this
  void I1(); // But not this
};

(I'm sure there are more that I've missed, but you get the idea.) If any of these point out other issues, feel free to address them in a follow-up patch if you'd prefer.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/misc/confusable-identifiers.cpp

42

Another case to consider:

template <typename O0, typename OO>
void f();

[serge-sans-paille]

@aaron.ballman : I agree with most of your suggestion except the one below that I annotated accordingly

struct Base {
  virtual void O0();

private:
  void II();
};

struct Derived : Base {
  void OO(); // We should warn about this
  void I1();   // I think we should warn about this too, because implementation of, say, `O0` could refer to `I1` wich would be confusable with `Il`
};

void I1(); // we should probably warn here too, becaus ein the implementation of, says, `O0`, if we refer to `Il`, it's confusable with this `I1` too

[aaron.ballman]

In D128715#3619650, @serge-sans-paille wrote:

@aaron.ballman : I agree with most of your suggestion except the one below that I annotated accordingly

struct Base {
  virtual void O0();

private:
  void II();
};

struct Derived : Base {
  void OO(); // We should warn about this
  void I1();   // I think we should warn about this too, because implementation of, say, `O0` could refer to `I1` wich would be confusable with `Il`
};

void I1(); // we should probably warn here too, becaus ein the implementation of, says, `O0`, if we refer to `Il`, it's confusable with this `I1` too

I don't think we should diagnose Derived::I1() because Base::II() is private, so I1 can never be validly confused with II (either from within the class context or externally).

The free function case is actually interesting but not because of confusable identifiers. If you call I1() within the context of Derived, are you trying to call the member function (you will) or the free function (you won't)? But I don't think it should be warned as a confusable identifier in terms of Unicode confusables because calling the member function outside of the class context requires an object while calling the free function does not, while calling it within the class is already confused because of name hiding.

Does that change your opinion at all?

[serge-sans-paille]

Update test case to take into accounts reviewers suggestions.

As a consequence use a more complex approach to detect whether two NamedDecl may interfere.

[aaron.ballman]

LGTM aside from some minor style nits.

clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp

130–132

135–137

142–145

[thakis]

Looks like this breaks tests on windows: http://45.33.8.238/win/61559/step_8.txt

[serge-sans-paille]

In D128715#3629253, @thakis wrote:

Looks like this breaks tests on windows: http://45.33.8.238/win/61559/step_8.txt

I'm on it.

clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp

if (Where == std::end(ConfusableEntries) || CodePoint != Where->codepoint) {
         break;
       }
       Skeleton.append((char *)BufferStart, (char *)IBuffer);
     }
   }
   return Skeleton;
 }
 
+static bool mayShadowImpl(const NamedDecl *ND0, const NamedDecl *ND1) {
+  const DeclContext *DC0 = ND0->getDeclContext()->getPrimaryContext();
+  const DeclContext *DC1 = ND1->getDeclContext()->getPrimaryContext();
+
+  if (isa<TemplateTypeParmDecl>(ND0) || isa<TemplateTypeParmDecl>(ND0))
+    return true;
+
+  while (DC0->isTransparentContext())
+    DC0 = DC0->getParent();
+  while (DC1->isTransparentContext())
+    DC1 = DC1->getParent();
+
+  if (DC0->Equals(DC1))
+    return true;
+
+  return false;
+}
+
+static bool isMemberOf(const NamedDecl *ND, const CXXRecordDecl *RD) {
+  const DeclContext *NDParent = ND->getDeclContext();
+  if (!NDParent || !isa<CXXRecordDecl>(NDParent))
+    return false;
+  if (NDParent == RD)
+    return true;
+  return !RD->forallBases(
+      [NDParent](const CXXRecordDecl *Base) { return NDParent != Base; });
+}
+
+static bool mayShadow(const NamedDecl *ND0, const NamedDecl *ND1) {
+
+  const DeclContext *DC0 = ND0->getDeclContext()->getPrimaryContext();
+  const DeclContext *DC1 = ND1->getDeclContext()->getPrimaryContext();
+
+  if (const CXXRecordDecl *RD0 = dyn_cast<CXXRecordDecl>(DC0)) {
+    if (ND1->getAccess() != AS_private && isMemberOf(ND1, RD0))
+      return true;
+  }
+  if (const CXXRecordDecl *RD1 = dyn_cast<CXXRecordDecl>(DC1)) {
+    if (ND0->getAccess() != AS_private && isMemberOf(ND0, RD1))
+      return true;
+  }
+
+  if (DC0->Encloses(DC1))
+    return mayShadowImpl(ND0, ND1);
+  if (DC1->Encloses(DC0))
+    return mayShadowImpl(ND1, ND0);
+  return false;
+}
+
 void ConfusableIdentifierCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("nameddecl")) {
-    if (IdentifierInfo *II = ND->getIdentifier()) {
-      StringRef NDName = II->getName();
+    if (IdentifierInfo *NDII = ND->getIdentifier()) {
+      StringRef NDName = NDII->getName();
       llvm::SmallVector<const NamedDecl *> &Mapped = Mapper[skeleton(NDName)];
-      const DeclContext *NDDecl = ND->getDeclContext();
       for (const NamedDecl *OND : Mapped) {
-        if (!NDDecl->isDeclInLexicalTraversal(OND) &&
-            !OND->getDeclContext()->isDeclInLexicalTraversal(ND))
-          continue;
-        if (OND->getIdentifier()->getName() != NDName) {
-          diag(OND->getLocation(), "%0 is confusable with %1")
-              << OND->getName() << NDName;
-          diag(ND->getLocation(), "other declaration found here",
+        const IdentifierInfo *ONDII = OND->getIdentifier();
+        if (mayShadow(ND, OND)) {
+          StringRef ONDName = ONDII->getName();
+          if (ONDName != NDName) {
+            diag(ND->getLocation(), "%0 is confusable with %1") << ND << OND;
+            diag(OND->getLocation(), "other declaration found here",
                  DiagnosticIDs::Note);
           }
         }
+      }
       Mapped.push_back(ND);
     }
   }
 }
 
 void ConfusableIdentifierCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(ast_matchers::namedDecl().bind("nameddecl"), this);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/misc/confusable-identifiers.cpp

 // RUN: %check_clang_tidy %s misc-confusable-identifiers %t
 
 int fo;
-// CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: fo is confusable with 𝐟o [misc-confusable-identifiers]
 int 𝐟o;
-// CHECK-MESSAGES: :[[#@LINE-1]]:5: note: other declaration found here
+// CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: '𝐟o' is confusable with 'fo' [misc-confusable-identifiers]
+// CHECK-MESSAGES: :[[#@LINE-3]]:5: note: other declaration found here
 
 void no() {
   int 𝐟oo;
 }
 
 void worry() {
   int foo;
 }
-
 int 𝐟i;
-// CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: 𝐟i is confusable with fi [misc-confusable-identifiers]
 int fi;
-// CHECK-MESSAGES: :[[#@LINE-1]]:5: note: other declaration found here
+// CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: 'fi' is confusable with '𝐟i' [misc-confusable-identifiers]
+// CHECK-MESSAGES: :[[#@LINE-3]]:5: note: other declaration found here
+
+bool f0(const char *q1, const char *ql) {
+  // CHECK-MESSAGES: :[[#@LINE-1]]:37: warning: 'ql' is confusable with 'q1' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-2]]:21: note: other declaration found here
+  return q1 < ql;
+}
 
 // should not print anything
 namespace ns {
 struct Foo {};
 } // namespace ns
 auto f = ns::Foo();
+
+struct Test {
+  void f1(const char *pl);
+};
+
+bool f2(const char *p1, const char *ql) {
+  return p1 < ql;
+}
+
+bool f3(const char *q0, const char *q1) {
+  return q0 < q1;
+}
+
+template <typename i1>
+struct S {
+  template <typename il>
+  void f4() {}
+  // CHECK-MESSAGES: :[[#@LINE-2]]:22: warning: 'il' is confusable with 'i1' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-5]]:20: note: other declaration found here
+};
+
+template <typename i1>
+void f5(int il) {
+  // CHECK-MESSAGES: :[[#@LINE-1]]:13: warning: 'il' is confusable with 'i1' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-3]]:20: note: other declaration found here
+}
+
+template <typename O0>
+void f6() {
+  int OO = 0;
+  // CHECK-MESSAGES: :[[#@LINE-1]]:7: warning: 'OO' is confusable with 'O0' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-4]]:20: note: other declaration found here
+}
+int OO = 0; // no warning, not same scope as f6
+
+namespace f7 {
+int i1;
+}
+
+namespace f8 {
+int il; // no warning, different namespace
+}
+
+namespace f7 {
+int il;
+// CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: 'il' is confusable with 'i1' [misc-confusable-identifiers]
+// CHECK-MESSAGES: :[[#@LINE-10]]:5: note: other declaration found here
+} // namespace f7
+
+template <typename t1, typename tl>
+// CHECK-MESSAGES: :[[#@LINE-1]]:33: warning: 'tl' is confusable with 't1' [misc-confusable-identifiers]
+// CHECK-MESSAGES: :[[#@LINE-2]]:20: note: other declaration found here
+void f9();
+
+struct Base0 {
+  virtual void mO0();
+
+private:
+  void mII();
+};
+
+struct Derived0 : Base0 {
+  void mOO();
+  // CHECK-MESSAGES: :[[#@LINE-1]]:8: warning: 'mOO' is confusable with 'mO0' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-9]]:16: note: other declaration found here
+
+  void mI1(); // no warning: mII is private
+};
+
+struct Base1 {
+  long mO0;
+
+private:
+  long mII;
+};
+
+struct Derived1 : Base1 {
+  long mOO;
+  // CHECK-MESSAGES: :[[#@LINE-1]]:8: warning: 'mOO' is confusable with 'mO0' [misc-confusable-identifiers]
+  // CHECK-MESSAGES: :[[#@LINE-9]]:8: note: other declaration found here
+
+  long mI1(); // no warning: mII is private
+};