[clang-tidy] Add check hicpp-ignored-remove-result

[bjosv]

This check implements the rule 17.5.1 of the HICPP standard which states:

• Do not ignore the result of std::remove, std::remove_if or std::unique

  The mutating algorithms std::remove, std::remove_if and both overloads of std::unique operate by swapping or moving elements of the range they are operating over. On completion, they return an iterator to the last valid element. In the majority of cases the correct behavior is to use this result as the first operand in a call to std::erase.

This check is based on bugprone-unused-return-value but with a fixed set of functions.

Suppressing issues by casting to void is enabled by default, but can be disabled by setting AllowCastToVoid option to false.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Björn Svensson (bjosv)

Changes

This check implements the rule 17.5.1 of the HICPP standard which states:

• Do not ignore the result of std::remove, std::remove_if or std::unique

  The mutating algorithms std::remove, std::remove_if and both overloads of std::unique operate by swapping or moving elements of the range they are operating over. On completion, they return an iterator to the last valid element. In the majority of cases the correct behavior is to use this result as the first operand in a call to std::erase.

This check is an alias of check bugprone-unused-return-value but with a fixed set of functions.

Suppressing issues by casting to void is enabled by default, but can be disabled by setting AllowCastToVoid option to false.

---

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

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp (+20)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (added) clang-tools-extra/docs/clang-tidy/checks/hicpp/ignored-remove-result.rst (+20)
• (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
• (added) clang-tools-extra/test/clang-tidy/checkers/hicpp/ignored-remove-result.cpp (+47)

diff --git a/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp b/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
index 3749796877120ed..09d15ccab3f29c2 100644
--- a/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/UndelegatedConstructorCheck.h"
+#include "../bugprone/UnusedReturnValueCheck.h"
 #include "../bugprone/UseAfterMoveCheck.h"
 #include "../cppcoreguidelines/AvoidGotoCheck.h"
 #include "../cppcoreguidelines/NoMallocCheck.h"
@@ -41,6 +42,15 @@
 #include "NoAssemblerCheck.h"
 #include "SignedBitwiseCheck.h"
 
+namespace {
+
+// Checked functions for hicpp-ignored-remove-result.
+const llvm::StringRef CheckedFunctions = "::std::remove;"
+                                         "::std::remove_if;"
+                                         "::std::unique;";
+
+} // namespace
+
 namespace clang::tidy {
 namespace hicpp {
 
@@ -64,6 +74,8 @@ class HICPPModule : public ClangTidyModule {
         "hicpp-explicit-conversions");
     CheckFactories.registerCheck<readability::FunctionSizeCheck>(
         "hicpp-function-size");
+    CheckFactories.registerCheck<bugprone::UnusedReturnValueCheck>(
+        "hicpp-ignored-remove-result");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "hicpp-named-parameter");
     CheckFactories.registerCheck<bugprone::UseAfterMoveCheck>(
@@ -107,6 +119,14 @@ class HICPPModule : public ClangTidyModule {
     CheckFactories.registerCheck<cppcoreguidelines::ProTypeVarargCheck>(
         "hicpp-vararg");
   }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+    Opts["hicpp-ignored-remove-result.CheckedFunctions"] = CheckedFunctions;
+    Opts["hicpp-ignored-remove-result.AllowCastToVoid"] = "true";
+    return Options;
+  }
 };
 
 // Register the HICPPModule using this statically initialized variable.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d5f49dc0625451..c940025df1c63cd 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -205,6 +205,10 @@ New check aliases
   <clang-tidy/checks/cppcoreguidelines/macro-to-enum>` to :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize/macro-to-enum>` was added.
 
+- New alias :doc:`hicpp-ignored-remove-result
+  <clang-tidy/checks/hicpp/ignored-remove-result>` to :doc:`bugprone-unused-return-value
+  <clang-tidy/checks/bugprone/unused-return-value>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/hicpp/ignored-remove-result.rst b/clang-tools-extra/docs/clang-tidy/checks/hicpp/ignored-remove-result.rst
new file mode 100644
index 000000000000000..4b6188b886db124
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/hicpp/ignored-remove-result.rst
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - hicpp-ignored-remove-result
+
+hicpp-ignored-remove-result
+===========================
+
+Ensure that the result of ``std::remove``, ``std::remove_if`` and ``std::unique``
+are not ignored according to
+`rule 17.5.1 <https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard/standard-library>`_.
+
+The mutating algorithms ``std::remove``, ``std::remove_if`` and both overloads
+of ``std::unique`` operate by swapping or moving elements of the range they are
+operating over. On completion, they return an iterator to the last valid
+element. In the majority of cases the correct behavior is to use this result as
+the first operand in a call to std::erase.
+
+This check is an alias of check :doc:`bugprone-unused-return-value <../bugprone/unused-return-value>`
+with a fixed set of functions.
+
+Suppressing issues by casting to ``void`` is enabled by default and can be
+disabled by setting `AllowCastToVoid` option to ``false``.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 6f987ba1672e3f2..417513b2f2aaa3a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -226,6 +226,7 @@ Clang-Tidy Checks
    :doc:`google-runtime-operator <google/runtime-operator>`,
    :doc:`google-upgrade-googletest-case <google/upgrade-googletest-case>`, "Yes"
    :doc:`hicpp-exception-baseclass <hicpp/exception-baseclass>`,
+   :doc:`hicpp-ignored-remove-result <hicpp/ignored-remove-result>`,
    :doc:`hicpp-multiway-paths-covered <hicpp/multiway-paths-covered>`,
    :doc:`hicpp-no-assembler <hicpp/no-assembler>`,
    :doc:`hicpp-signed-bitwise <hicpp/signed-bitwise>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/hicpp/ignored-remove-result.cpp b/clang-tools-extra/test/clang-tidy/checkers/hicpp/ignored-remove-result.cpp
new file mode 100644
index 000000000000000..86deb96216a687a
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/hicpp/ignored-remove-result.cpp
@@ -0,0 +1,47 @@
+// RUN: %check_clang_tidy %s hicpp-ignored-remove-result %t
+
+namespace std {
+
+template <typename ForwardIt, typename T>
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template <typename ForwardIt, typename UnaryPredicate>
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template <typename ForwardIt>
+ForwardIt unique(ForwardIt, ForwardIt);
+
+template <class InputIt, class T>
+InputIt find(InputIt, InputIt, const T&);
+
+} // namespace std
+
+void warning() {
+  std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors
+  // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning
+
+  std::remove_if(nullptr, nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors
+  // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning
+
+  std::unique(nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors
+  // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning
+}
+
+void noWarning() {
+
+  auto RemoveRetval = std::remove(nullptr, nullptr, 1);
+
+  auto RemoveIfRetval = std::remove_if(nullptr, nullptr, nullptr);
+
+  auto UniqueRetval = std::unique(nullptr, nullptr);
+
+  // cast to void should allow ignoring the return value
+  (void)std::remove(nullptr, nullptr, 1);
+
+  // no warning on std::find since the checker overrides
+  // bugprone-unused-return-value's checked functions.
+  std::find(nullptr, nullptr, 1);
+}

[PiotrZSL]

Looks fine, as we got already such checks that are and are not aliases.
But consider doing this as an actually separate check instead of using an alias functionality.
Simply change original check, add other constructor, split storeOptions method, so in this check list of functions would not be configurable by end user.

[bjosv]

Great, I'll have a go at it to move it into a separate check.
I used cert-err33-c as a blueprint and maybe that check can be updated later as well if we find a good model.

[PiotrZSL]

by separate check I mean "Create class that derive from oryginal check, move some code in oryginal check (config) to protected methods (mainly storeConfig) and just override config in parent class. SImply if you call --dump-config, then for your check list of functions shouldn't be configurable.

[bjosv]

Updated the check to be derived from the original check instead of an alias.

[PiotrZSL]

Small nits, but looks fine.

[PiotrZSL]

LGTM

[bjosv]

Thanks for the informative review comments @PiotrZSL, much appreciated.