[PATCH] D24507: Add attribute for return values that shouldn't be cast to bool

[Anton Urusov via cfe-commits]

urusant added a comment.

Thank you for the feedback.

> The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).


There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've added another one for attribute accepting no args, so now the last two test cases in this file are those you were asking about. Can you think of any other cases of invalid attribute usage?

> Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?


No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like `int r = X509_verify_cert(...)` and the function `X509_verify_cert` has a return type with attribute, `r` won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?


================
Comment at: include/clang/Basic/Attr.td:1138
@@ +1137,3 @@
+def WarnImpcastToBool : InheritableAttr {
+  let Spellings = [GCC<"warn_impcast_to_bool">];
+  let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
----------------
aaron.ballman wrote:
> This should not use a GCC spelling because it's not an attribute that GCC supports. You should probably use GNU instead, since I suspect this attribute will be useful in C as well as C++.
Yeah, makes sense.

================
Comment at: include/clang/Basic/Attr.td:1140
@@ +1139,3 @@
+  let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
+                             "ExpectedFunctionOrMethod">;
+  let Documentation = [WarnImpcastToBoolDocs];
----------------
aaron.ballman wrote:
> No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they will be handled automatically.
I didn't know that, thanks.

================
Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
 }
+def WarnImpcastToBoolDocs : Documentation {
+  let Category = DocCatFunction;
----------------
zaks.anna wrote:
> You probably need to "propose" the attribute to the clang community. I'd send an email to the cfe-dev as it might not have enough attention if it's just the patch.  
OK, will do.

================
Comment at: include/clang/Basic/AttrDocs.td:2058
@@ +2057,3 @@
+  let Content = [{
+    The ``warn_impcast_to_bool`` attribute is used to indicate that the return value of a function with integral return type cannot be used as a boolean value. For example, if a function returns -1 if it couldn't efficiently read the data, 0 if the data is invalid and 1 for success, it might be dangerous to implicitly cast the return value to bool, e.g. to indicate success. Therefore, it is a good idea to trigger a warning about such cases. However, in case a programmer uses an explicit cast to bool, that probably means that he knows what he is doing, therefore a warning should be triggered only for implicit casts.
+
----------------
aaron.ballman wrote:
> You should manually wrap this to roughly the 80 col limit.
> 
> Instead of "he", can you use "they" please?
OK, I did that. However, 80 col limit in this case feels a bit inconsistent with the rest of the file to me because most of other similar descriptions don't follow it.

================
Comment at: include/clang/Basic/DiagnosticGroups.td:57
@@ -56,2 +56,3 @@
 def DoublePromotion : DiagGroup<"double-promotion">;
+def UnsafeBoolConversion : DiagGroup<"unsafe-bool-conversion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
----------------
aaron.ballman wrote:
> I'm not certain this requires its own diagnostic group. This can probably be handled under `BoolConversion`
OK.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2259
@@ +2258,3 @@
+def warn_attribute_return_int_only : Warning<
+  "%0 attribute only applies to return values that are integers">,
+  InGroup<IgnoredAttributes>;
----------------
aaron.ballman wrote:
> How about: ...only applies to integer return types?
Yeah, that sounds better.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2883
@@ +2882,3 @@
+  "implicit conversion turns non-bool into bool: %0 to %1">,
+  InGroup<UnsafeBoolConversion>, DefaultIgnore;
+
----------------
aaron.ballman wrote:
> I don't think this should be a DefaultIgnore diagnostic -- if the user wrote the attribute, they should get the diagnostic when appropriate.
Makes sense.

================
Comment at: lib/Sema/SemaChecking.cpp:8262
@@ +8261,3 @@
+    /// e.g. (x ? f : g)(y)
+    if (isa<CallExpr>(E)) {
+      FunctionDecl* fn = cast<CallExpr>(E)->getDirectCallee();
----------------
aaron.ballman wrote:
> Should use `if (const auto *CE = dyn_cast<CallExpr>(E)) {`
Done.

================
Comment at: lib/Sema/SemaChecking.cpp:8263-8264
@@ +8262,4 @@
+    if (isa<CallExpr>(E)) {
+      FunctionDecl* fn = cast<CallExpr>(E)->getDirectCallee();
+      if (!fn)
+        return;
----------------
aaron.ballman wrote:
> Then you can do `if (const auto *Fn = CE->getDirectCallee()) {`
Done.

================
Comment at: lib/Sema/SemaChecking.cpp:8269
@@ +8268,3 @@
+        S.Diag(fn->getLocation(), diag::note_entity_declared_at)
+                        << fn->getDeclName();
+        return;
----------------
aaron.ballman wrote:
> You can pass in `fn` directly, the diagnostics engine will properly get the name out of it because it's derived from `NamedDecl`.
Thanks, didn't notice that.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1316
@@ +1315,3 @@
+    S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only)
+      <<Attr.getName() << SourceRange() << SR;
+    return;
----------------
aaron.ballman wrote:
> Formatting seems off -- you should run the patch through clang-format. Also, why are you passing an empty `SourceRange`?
Ok, I ran clang-format.
Good spot, it seems that I don't need that `SourceRange`.

================
Comment at: test/ReturnNonBoolTest.c:74
@@ +73,3 @@
+  // no good way to get those from the program state.
+  if (restricted_wrap(2)) // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
----------------
zaks.anna wrote:
> I do not understand why this is a false positive. In restricted_wrap, r can be any value. You only return '0' if r is '-1', but it could be '-2' or '100', which are also not bool and this values would just get returned.
> 
> You should be able to query the state to check if a value is a zero or one using code like this from CStringChecker.cpp:
> "
>   SValBuilder &svalBuilder = C.getSValBuilder();
>   DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
>   return state->assume(svalBuilder.evalEQ(state, *val, zero))
> "
> 
I have replaced this test case with another one that illustrates the problem I am referring to clearer. Ideally it would be great to have some indicator to tell the StaticAnalyzer that we have handled all the dangerous return values, and from this point it is safe to use it as a boolean. You can use explicit cast to bool or `rc != 0` every time you want to use it, but it is not very convenient. Do you have any suggestions on this matter?

As for your proposal, it is not very difficult to add, however, it is not very likely to be useful in real codebases for the same reason as in the testcase. Do you still think it should be added?

================
Comment at: test/ReturnNonBoolTestCompileTime.cpp:40
@@ +39,1 @@
+double InvalidAttributeUsage() RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to return values that are integers}}
\ No newline at end of file

----------------
aaron.ballman wrote:
> Can you end the file with a newline?
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D24507