[clang-tidy] Fix false positive for cppcoreguidelines-pro-bounds-pointer-arithmetic

[flovent]

this PR fixes #126424 for ArraySubScriptExpr, hasBase Matcher will get right operand when it is not integer type, but is not for sure that left operand is integer type. For the example code below hasBase will get r for the Subsequent matching and causing false positive.

template <typename R>
int f(std::map<R*, int>& map, R* r) {
  return map[r];
}

so is needed to see if index is integer type to avoid this situation.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: None (flovent)

Changes

this PR fixs #126424 for ArraySubScriptExpr, hasBase Matcher will get right operand when it is not Integer type, but is not for sure that left operand is interger type. For the example code below hasBase will get r for the Subsequent matching and causing false positive.

template <typename R>
int f(std::map<R*, int>& map, R* r) {
  return map[r];
}

so is needed to see if index is interger type to avoid this situation.

---

Full diff: https://github.com/llvm/llvm-project/pull/127394.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp (+2-1)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (added) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp (+19)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
index a1494a095f5b6..0d68790349fb5 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
@@ -42,7 +42,8 @@ void ProBoundsPointerArithmeticCheck::registerMatchers(MatchFinder *Finder) {
       arraySubscriptExpr(
           hasBase(ignoringImpCasts(
               anyOf(AllPointerTypes,
-                    hasType(decayedType(hasDecayedType(pointerType())))))))
+                    hasType(decayedType(hasDecayedType(pointerType())))))),
+          hasIndex(hasType(isInteger())))
           .bind("expr"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b8fe22242417..937d710fe6100 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -110,6 +110,10 @@ Changes in existing checks
   <clang-tidy/checks/misc/redundant-expression>` check by providing additional
   examples and fixing some macro related false positives.
 
+- Improved :doc:`cppcoreguidelines-pro-bounds-pointer-arithmetic
+  <clang-tidy/checks/cppcoreguidelines/pro-bounds-pointer-arithmetic>` check by
+  fix false positives related to operator overloading and templates.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp
new file mode 100644
index 0000000000000..b6b7a9664f38d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-pointer-arithmetic %t
+
+namespace std {
+template <typename, typename>
+class pair {};
+
+template <typename Key, typename Value>
+class map {
+  public:
+   using value_type = pair<Key, Value>;
+   value_type& operator[](const Key& key);
+   value_type& operator[](Key&& key);
+ };
+}
+
+template <typename R>
+int f(std::map<R*, int>& map, R* r) {
+  return map[r]; // OK
+}
\ No newline at end of file

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: None (flovent)

Changes

this PR fixs #126424 for ArraySubScriptExpr, hasBase Matcher will get right operand when it is not Integer type, but is not for sure that left operand is interger type. For the example code below hasBase will get r for the Subsequent matching and causing false positive.

template <typename R>
int f(std::map<R*, int>& map, R* r) {
  return map[r];
}

so is needed to see if index is interger type to avoid this situation.

---

Full diff: https://github.com/llvm/llvm-project/pull/127394.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp (+2-1)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (added) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp (+19)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
index a1494a095f5b6..0d68790349fb5 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
@@ -42,7 +42,8 @@ void ProBoundsPointerArithmeticCheck::registerMatchers(MatchFinder *Finder) {
       arraySubscriptExpr(
           hasBase(ignoringImpCasts(
               anyOf(AllPointerTypes,
-                    hasType(decayedType(hasDecayedType(pointerType())))))))
+                    hasType(decayedType(hasDecayedType(pointerType())))))),
+          hasIndex(hasType(isInteger())))
           .bind("expr"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b8fe22242417..937d710fe6100 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -110,6 +110,10 @@ Changes in existing checks
   <clang-tidy/checks/misc/redundant-expression>` check by providing additional
   examples and fixing some macro related false positives.
 
+- Improved :doc:`cppcoreguidelines-pro-bounds-pointer-arithmetic
+  <clang-tidy/checks/cppcoreguidelines/pro-bounds-pointer-arithmetic>` check by
+  fix false positives related to operator overloading and templates.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp
new file mode 100644
index 0000000000000..b6b7a9664f38d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-bounds-pointer-arithmetic-issue126424.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-bounds-pointer-arithmetic %t
+
+namespace std {
+template <typename, typename>
+class pair {};
+
+template <typename Key, typename Value>
+class map {
+  public:
+   using value_type = pair<Key, Value>;
+   value_type& operator[](const Key& key);
+   value_type& operator[](Key&& key);
+ };
+}
+
+template <typename R>
+int f(std::map<R*, int>& map, R* r) {
+  return map[r]; // OK
+}
\ No newline at end of file

[flovent]

ping

[carlosgalvezp]

LGTM in general, but won't this fail with a std::map<int, int>, since the key is an integer?

Please rebase on top of latest main and resolve conflicts.

[flovent]

but won't this fail with a std::map<int, int>, since the key is an integer?

It won't, ArraySubScriptExpr is generated here because variable map's type is dependent, it's AST:

ArraySubscriptExpr <col:10, col:15> '<dependent type>' lvalue
      -DeclRefExpr <col:10> 'std::map<R *, int>':'map<R *, int>' lvalue ParmVar 0x227b3b48 'map' 'std::map<R *, int> &'
      -DeclRefExpr <col:14> 'R *' lvalue ParmVar 0x227b3bc8 'r' 'R *'

std::map's key is unrelated here, for std::map<int, int> a CallExpr will be generated since it's known that std::map<int, int> has a overload operator []

[flovent]

@vbvictor, thanks for the review! I just rebased and merged test case to this check's original test file.

[vbvictor]

"Checking for the ability to merge automatically..." message was for the past 6 hours, could you please rebase on fresh main again, maybe some conflicts cause it. I'll merge once conflicts resolved.