From d9a5f37b9398d70003f359aa348ce982217dbab2 Mon Sep 17 00:00:00 2001 From: Congcong Cai Date: Sat, 8 Mar 2025 21:50:19 +0800 Subject: [PATCH 1/2] [clang-tidy] support to detect conversion in `make_optional` for `bugprone-optional-value-conversion` Fixes: #119554 --- .../bugprone/OptionalValueConversionCheck.cpp | 14 ++++++++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 4 ++++ ...ptional-value-conversion-construct-from-std.cpp | 13 +++++++++++++ 3 files changed, 31 insertions(+) diff --git a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp index 33e823ac07490..cb5a1c7bea801 100644 --- a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp @@ -12,6 +12,7 @@ #include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" #include using namespace clang::ast_matchers; @@ -31,6 +32,7 @@ constexpr std::array MakeSmartPtrList{ "::std::make_unique", "::std::make_shared", }; +constexpr StringRef MakeOptional = "::std::make_optional"; } // namespace @@ -86,6 +88,18 @@ void OptionalValueConversionCheck::registerMatchers(MatchFinder *Finder) { callee(functionDecl( matchers::matchesAnyListedName(MakeSmartPtrList), hasTemplateArgument(0, refersToType(BindOptionalType)))), + hasArgument(0, OptionalDerefMatcher)), + callExpr( + // match first std::make_optional by limit argument count (1) + // and template count (1). + // 1. template< class T > constexpr + // std::optional> make_optional(T&& value); + // 2. template< class T, class... Args > constexpr + // std::optional make_optional(Args&&... args); + argumentCountIs(1), + callee(functionDecl(templateArgumentCountIs(1), + hasName(MakeOptional), + returns(BindOptionalType))), hasArgument(0, OptionalDerefMatcher))), unless(anyOf(hasAncestor(typeLoc()), hasAncestor(expr(matchers::hasUnevaluatedContext()))))) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8f8e6e3c0165d..56dcccd5c854c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,10 @@ Changes in existing checks no longer be needed and will be removed. Also fixing false positive from const reference accessors to objects containing optional member. +- Improved :doc:`bugprone-optional-value-conversion + ` check to detect + conversion in argument of ``std::make_optional``. + - Improved :doc:`bugprone-unsafe-functions ` check to allow specifying additional C++ member functions to match. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion-construct-from-std.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion-construct-from-std.cpp index 768ab1ce014ce..305fd6890710d 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion-construct-from-std.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion-construct-from-std.cpp @@ -27,9 +27,19 @@ class unique_ptr {}; template class shared_ptr {}; +template +class initializer_list {}; + template unique_ptr make_unique(Args &&...args); template shared_ptr make_shared(Args &&...args); +template +constexpr std::optional<__decay(T)> make_optional(T &&value); +template +constexpr std::optional make_optional(Args &&...args); +template +constexpr std::optional make_optional(std::initializer_list il, Args &&...args); + } // namespace std struct A { @@ -45,9 +55,12 @@ void invalid() { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] std::make_shared>(opt.value()); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] + std::make_optional(opt.value()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] } void valid() { std::make_unique(opt.value()); std::make_shared(opt.value()); + std::make_optional(opt.value()); } From eee9e3f16d341b86810cd2502c40ff4ab126d567 Mon Sep 17 00:00:00 2001 From: Congcong Cai Date: Tue, 11 Mar 2025 17:12:43 +0800 Subject: [PATCH 2/2] fix --- .../bugprone/OptionalValueConversionCheck.cpp | 29 +++++++++++-------- clang-tools-extra/docs/ReleaseNotes.rst | 8 ++--- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp index cb5a1c7bea801..cda9288c0531a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp @@ -85,21 +85,26 @@ void OptionalValueConversionCheck::registerMatchers(MatchFinder *Finder) { // known template methods in std callExpr( argumentCountIs(1), - callee(functionDecl( - matchers::matchesAnyListedName(MakeSmartPtrList), - hasTemplateArgument(0, refersToType(BindOptionalType)))), + anyOf( + // match std::make_unique std::make_shared + callee(functionDecl( + matchers::matchesAnyListedName(MakeSmartPtrList), + hasTemplateArgument( + 0, refersToType(BindOptionalType)))), + // match first std::make_optional by limit argument count + // (1) and template count (1). + // 1. template< class T > constexpr + // std::optional> make_optional(T&& value); + // 2. template< class T, class... Args > constexpr + // std::optional make_optional(Args&&... args); + callee(functionDecl(templateArgumentCountIs(1), + hasName(MakeOptional), + returns(BindOptionalType)))), hasArgument(0, OptionalDerefMatcher)), callExpr( - // match first std::make_optional by limit argument count (1) - // and template count (1). - // 1. template< class T > constexpr - // std::optional> make_optional(T&& value); - // 2. template< class T, class... Args > constexpr - // std::optional make_optional(Args&&... args); + argumentCountIs(1), - callee(functionDecl(templateArgumentCountIs(1), - hasName(MakeOptional), - returns(BindOptionalType))), + hasArgument(0, OptionalDerefMatcher))), unless(anyOf(hasAncestor(typeLoc()), hasAncestor(expr(matchers::hasUnevaluatedContext()))))) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 56dcccd5c854c..110f949741c3f 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -112,6 +112,10 @@ New check aliases Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`bugprone-optional-value-conversion + ` check to detect + conversion in argument of ``std::make_optional``. + - Improved :doc:`bugprone-string-constructor ` check to find suspicious calls of ``std::string`` constructor with char pointer, start position and @@ -124,10 +128,6 @@ Changes in existing checks no longer be needed and will be removed. Also fixing false positive from const reference accessors to objects containing optional member. -- Improved :doc:`bugprone-optional-value-conversion - ` check to detect - conversion in argument of ``std::make_optional``. - - Improved :doc:`bugprone-unsafe-functions ` check to allow specifying additional C++ member functions to match.