1253844 - Detect typos that swapped bool / integer type arguments.

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mats Palmgren (inactive)]

In bug 1253534 Mark found a call to:
  static bool IsSingleLineTextControl(bool aExcludePassword, uint32_t aType)
that looks like this:
  IsSingleLineTextControl(mType, false)
(where 'mType' has the type uint8_t)

That's obviously a typo.
It would be nice to be able to automatically detect such typos.

I think the criteria is a call where at least one argument of type bool
is coerced into an integer type and at least one argument of integer type
that is coerced into a bool type.

Comment 1

[Mark Capella [:capella]]

Can this magic "static analysis" help with things like: ?

Bug 1253812 - Return types possibly swapped through AndroidBridge
              (GetScreenOrientation <---> GetScreenAngle)

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mark Capella [:capella] from comment #1)
> Can this magic "static analysis" help with things like: ?
> 
> Bug 1253812 - Return types possibly swapped through AndroidBridge
>               (GetScreenOrientation <---> GetScreenAngle)

Theoretically there could be a static analysis which checks for functions with a single return statement, which perform no action other than to forward arguments to another function, and then implicitly cast its type. I think that would catch that particular case. It should be done in a separate bug though.

Comment 3

[Mats Palmgren (inactive)]

I wonder if we could make it even stricter and forbid all bool->int coercions?
So if you want to do that you'd have to be explicit about it.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I took a look at prohibiting ImplicitCastExpr nodes which contain bool->int coercions. However, for some reason clang seems to generate the implicit cast expression even when the cast is performed explicitly. For example:

int x = true; 

should error, as true is a bool. An ImplicitCastExpr is generated in this situation, and the error is caught. In contrast,

int x = (int)true;

should pass, as it is explicitly cast. However, instead I get the AST:

    `-DeclStmt 0x7fae2407a118 <line:51:3, col:30>
      `-VarDecl 0x7fae2407a050 <col:3, col:26> col:7 x 'int' cinit
        `-CStyleCastExpr 0x7fae2407a0f0 <col:21, col:26> 'int' <NoOp>
          `-ImplicitCastExpr 0x7fae2407a0d8 <col:26> 'int' <IntegralCast>
            `-CXXBoolLiteralExpr 0x7fae2407a0b0 <col:26> '_Bool' true

where the implicit cast is inserted before the c-style cast and the c-style cast is changed to a no-op. I'm not sure what to do about this. I could make no-op casts add any casts which they wrap to a hashmap of acceptable casts, which are ignored, but that seems super sketchy and unlikely to work in all situations.

botond/ehsan, any ideas for why this might be happening?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: DRAFT bool->int implicit cast analysis

This isn't a finished patch (as it doesn't work as per the above comment), but I figure I'll attach it anyways.

Comment 6

[Botond Ballo [:botond]]

(In reply to Michael Layzell [:mystor] from comment #4)
> botond/ehsan, any ideas for why this might be happening?

I'm afraid not. I would suggest posting the two lines of code and their corresponding ASTs to [e-mail], and asking what is the intended way to distinguish between the two for static analysis purposes. There ought to be a simple answer, and the folks on the list will know it.

Comment 7

[(no longer active)]

Hmm, that AST looks a bit crazy to me!  I second Botond's suggestion.  :-)

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Sent an email to cfe-dev. We'll see if we get any interesting responses.

Comment 9

[(no longer active)]

Any responses yet?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Reject implicit bool to integer casts

In a turn of events which will surprise nobody, the updated patch finds lots of implicit bool->int conversions in our tree. I don't have the time right now to go through and make all of them explicit, so this patch is probably going to sit here for a while.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Is this the same as http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html ?

Comment 12

[Andi [:andi]]

Yes that checker covers what thing bug is all about and also offers the possibility of an auto-fix, if you really want to do that.

Testing the checker on the provided tests case we get:

/Users/abpostelnicu/Desktop/test.cpp:20:13: warning: implicit conversion bool -> 'char' [readability-implicit-bool-conversion]
  char s0 = b; // expected-error {{Invalid implicit bool to integer coersion}}
            ^~
            static_cast<char>()
/Users/abpostelnicu/Desktop/test.cpp:21:14: warning: implicit conversion bool -> 'short' [readability-implicit-bool-conversion]
  short s1 = b; // expected-error {{Invalid implicit bool to integer coersion}}
             ^~
             static_cast<short>()
/Users/abpostelnicu/Desktop/test.cpp:22:12: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  int s2 = b; // expected-error {{Invalid implicit bool to integer coersion}}
           ^~
           static_cast<int>()
/Users/abpostelnicu/Desktop/test.cpp:23:13: warning: implicit conversion bool -> 'long' [readability-implicit-bool-conversion]
  long s3 = b; // expected-error {{Invalid implicit bool to integer coersion}}
            ^~
            static_cast<long>()
/Users/abpostelnicu/Desktop/test.cpp:25:22: warning: implicit conversion bool -> 'unsigned char' [readability-implicit-bool-conversion]
  unsigned char u0 = b; // expected-error {{Invalid implicit bool to integer coersion}}
                     ^~
                     static_cast<unsigned char>()
/Users/abpostelnicu/Desktop/test.cpp:26:23: warning: implicit conversion bool -> 'unsigned short' [readability-implicit-bool-conversion]
  unsigned short u1 = b; // expected-error {{Invalid implicit bool to integer coersion}}
                      ^~
                      static_cast<unsigned short>()
/Users/abpostelnicu/Desktop/test.cpp:27:17: warning: implicit conversion bool -> 'unsigned int' [readability-implicit-bool-conversion]
  unsigned u2 = b; // expected-error {{Invalid implicit bool to integer coersion}}
                ^~
                static_cast<unsigned int>()
/Users/abpostelnicu/Desktop/test.cpp:28:22: warning: implicit conversion bool -> 'unsigned long' [readability-implicit-bool-conversion]
  unsigned long u3 = b; // expected-error {{Invalid implicit bool to integer coersion}}
                     ^~
                     static_cast<unsigned long>()
/Users/abpostelnicu/Desktop/test.cpp:30:13: warning: implicit conversion bool -> 'char' [readability-implicit-bool-conversion]
  takesChar(b); // expected-error {{Invalid implicit bool to integer coersion}}
            ^~
            static_cast<char>()
/Users/abpostelnicu/Desktop/test.cpp:31:14: warning: implicit conversion bool -> 'short' [readability-implicit-bool-conversion]
  takesShort(b); // expected-error {{Invalid implicit bool to integer coersion}}
             ^~
             static_cast<short>()
/Users/abpostelnicu/Desktop/test.cpp:32:12: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  takesInt(b); // expected-error {{Invalid implicit bool to integer coersion}}
           ^~
           static_cast<int>()
/Users/abpostelnicu/Desktop/test.cpp:33:13: warning: implicit conversion bool -> 'long' [readability-implicit-bool-conversion]
  takesLong(b); // expected-error {{Invalid implicit bool to integer coersion}}
            ^~
            static_cast<long>()
/Users/abpostelnicu/Desktop/test.cpp:34:14: warning: implicit conversion bool -> 'unsigned char' [readability-implicit-bool-conversion]
  takesUChar(b); // expected-error {{Invalid implicit bool to integer coersion}}
             ^~
             static_cast<unsigned char>()
/Users/abpostelnicu/Desktop/test.cpp:35:15: warning: implicit conversion bool -> 'unsigned short' [readability-implicit-bool-conversion]
  takesUShort(b); // expected-error {{Invalid implicit bool to integer coersion}}
              ^~
              static_cast<unsigned short>()
/Users/abpostelnicu/Desktop/test.cpp:36:13: warning: implicit conversion bool -> 'unsigned int' [readability-implicit-bool-conversion]
  takesUInt(b); // expected-error {{Invalid implicit bool to integer coersion}}
            ^~
            static_cast<unsigned int>()
/Users/abpostelnicu/Desktop/test.cpp:37:14: warning: implicit conversion bool -> 'unsigned long' [readability-implicit-bool-conversion]
  takesULong(b); // expected-error {{Invalid implicit bool to integer coersion}}
             ^~
             static_cast<unsigned long>()
/Users/abpostelnicu/Desktop/test.cpp:39:26: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  InitializedWithInt i = b; // expected-error {{Invalid implicit bool to integer coersion}}
                         ^~
                         static_cast<int>()
/Users/abpostelnicu/Desktop/test.cpp:40:23: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  (InitializedWithInt(b)); // expected-error {{Invalid implicit bool to integer coersion}}
                      ^~
                      static_cast<int>()
/Users/abpostelnicu/Desktop/test.cpp:49:7: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  if (b == exp) {} // expected-error {{Invalid implicit bool to integer coersion}}
      ^
      static_cast<int>()
/Users/abpostelnicu/Desktop/test.cpp:50:14: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
  if (exp == b) {} // expected-error {{Invalid implicit bool to integer coersion}}
             ^~
             static_cast<int>()

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1253844 - Enable the readability-implicit-bool-conversion checker r?andi

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fe284f84898
Enable the readability-implicit-bool-conversion checker r=andi

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6fe284f84898