Create rule S7172 Named methods should be used to avoid confusion between testing an optional or an expected and testing the wrapped value CPP-5929

rules/S7172/cfamily/rule.adoc

@@ -1,11 +1,7 @@
-FIXME: add a description
-
-// If you want to factorize the description uncomment the following line and create the file.
-//include::../description.adoc[]
 
 == Why is this an issue?
 
-`std::optional`, `boost::optional`, and `std::expected` are types that allow to represent a contained value of a certain type, and additional data that denote the absence of a value (`nullopt` for `optional`, and error values for `expected`). To check if the wrapper contains a value, it is possible to call the member function `has_value()`. It is equivalent to converting the wrapper to `bool`, which can be more concise, especially when used in a place that allows _contextual conversion to bool_ (for instance, the condition of an `if`). It means that the following syntax is valid:
+`std::optional`, `boost::optional`, and `std::expected` are types that allow to represent a contained value of a certain type, and additional data that denote the absence of a value (`nullopt` for `optional`, and error values for `expected`). To check if the wrapper contains a value, it is possible to call its member function `has_value()`. Alternatively, it can also be converted to `bool`, which is more concise, especially when used in a place that allows _contextual conversion to bool_ (for instance, the condition of an `if`). It means that the following syntax is valid:
 
 [source,cpp]
 ----
@@ -15,16 +11,27 @@ if(flag) { ... }
 if(flag.has_value()) { ... }
 ----
 
-When the wrapped type is also convertible to `bool`, using this concise syntax can be confusing. What is tested: The wrapper, or the wrapped value?
-This rule raises an issue when `std::optional`, `boost::optional`, or `std::expected` wrap a basic type, and the presence of the value is tested with the concise syntax. However, if in a single expression, the presence of the value is tested and the value is accessed, the risk of confusion is reduced, and no violation is raised.
+When the contained type is also convertible to `bool`, using this concise syntax can be confusing. What is tested: The wrapper, or the contained value?
+
+This rule raises an issue when `std::optional`, `boost::optional`, or `std::expected` wrap a basic type, and the conversion to `bool` is used to test presence of the value.
 
 === What is the potential impact?
 
 There are two possibilities, depending on the initial intent of the autor of the code:
 
-- If the intent was actually to test the presence of the value, the code is correct, but it is not clear. When reading it, people might think that the wrapped value is tested, and not the presence of the value.
+- If the intent was actually to test the presence of the value, the code is correct, but it is not clear. When reading it, people might think that the contained value is tested, and not the presence of the value.
+
+- If the intent was to test the contained value, the code is incorrect. This situation can especially happen when evolving code that worked directly with values to work with optional or expected values, and forgetting to update the test. This will lead to code that does not behave in the intended way, but still works in a plausible way, and the lack of clear clue on what is tested makes finding the issue difficult.
+
+=== Exceptions
+
+If, in a single expression, the presence of the value is tested and the value is accessed, the risk of confusion is reduced, and no violation is raised.
 
-- If the intent was to test the wrapped value, the code is incorrect. This situation can especially happen when evolving code that worked directly with values to work with optional or expected values, and forgetting to update the test. This will lead to code that does not behave in the intended way, but still works in a plausible way, and the lack of clear clue on twhat is tested makes finding the issue difficult.
+[source,cpp]
+----
+std::optional<bool> flag;
+if(flag && *flag) { ... } // Compliant
+----
 
 == How to fix it
 
@@ -107,7 +114,7 @@ The downside of this version is that `getDefaultRecurseMode()` is called even if
 [source,cpp]
 ----
 void perform(optional<bool> recursive) {
-  bool recurse = recursive.or_else([]() -> optional<bool> { return getDefaultRecurseMode();}).value();
+  bool recurse = recursive.or_else([]() { return optional {getDefaultRecurseMode()};}).value();
   if (recurse) {
      // perform recursion...
   }