Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 2, 2025
Merged

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

merged 6 commits into from
Jul 2, 2025

Conversation

@flovent
Copy link
Contributor

@flovent flovent commented Feb 16, 2025 •
edited by carlosgalvezp
Loading

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
Copy link
Member

llvmbot commented Feb 16, 2025

@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 &lt;typename R&gt;
int f(std::map&lt;R*, int&gt;&amp; 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
Copy link
Member

llvmbot commented Feb 16, 2025

@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 &lt;typename R&gt;
int f(std::map&lt;R*, int&gt;&amp; 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

EugeneZelenko
EugeneZelenko reviewed Feb 16, 2025
View reviewed changes
@flovent
Copy link
Contributor Author

flovent commented Feb 24, 2025

ping

@carlosgalvezp
Copy link
Contributor

carlosgalvezp commented Apr 30, 2025

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.

carlosgalvezp
carlosgalvezp reviewed Apr 30, 2025
View reviewed changes
@flovent
Copy link
Contributor Author

flovent commented May 1, 2025

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 flovent force-pushed the clang-tidy-issue-126424 branch from e898caf to 8284ecf Compare May 24, 2025 10:11
@flovent flovent requested a review from carlosgalvezp May 24, 2025 10:14
@flovent flovent force-pushed the clang-tidy-issue-126424 branch 2 times, most recently from c24d7bb to 38d3f82 Compare May 25, 2025 09:26
vbvictor
vbvictor approved these changes Jun 29, 2025
View reviewed changes
Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit.
Please rebase on fresh main and I think we can merge it, unless @carlosgalvezp has something against.

@flovent flovent force-pushed the clang-tidy-issue-126424 branch from 38d3f82 to 3aad049 Compare June 30, 2025 12:37
@flovent
Copy link
Contributor Author

flovent commented Jun 30, 2025

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

@vbvictor
Copy link
Contributor

vbvictor commented Jul 2, 2025 •
edited
Loading

"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.

@flovent flovent force-pushed the clang-tidy-issue-126424 branch from 2b8773f to a91f5b6 Compare July 2, 2025 14:07
@vbvictor vbvictor merged commit 5a8d096 into llvm:main Jul 2, 2025
8 checks passed
@flovent flovent deleted the clang-tidy-issue-126424 branch July 2, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosgalvezp carlosgalvezp carlosgalvezp left review comments

@EugeneZelenko EugeneZelenko EugeneZelenko left review comments

@vbvictor vbvictor vbvictor approved these changes

@PiotrZSL PiotrZSL Awaiting requested review from PiotrZSL

@5chmidti 5chmidti Awaiting requested review from 5chmidti

@HerrCai0907 HerrCai0907 Awaiting requested review from HerrCai0907

Assignees

No one assigned

Labels

clang-tidy clang-tools-extra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[clang-tidy] cppcoreguidelines-pro-bounds-pointer-arithmetic generates incorrect warnings re. to std::map::operator[]

5 participants

@flovent @llvmbot @carlosgalvezp @vbvictor @EugeneZelenko