Detect typos that swapped bool / integer type arguments.

RESOLVED FIXED in Firefox 64

Status

defect
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: mats, Assigned: sylvestre)

Tracking

unspecified
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Can this magic "static analysis" help with things like: ?

Bug 1253812 - Return types possibly swapped through AndroidBridge
              (GetScreenOrientation <---> GetScreenAngle)
(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.
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.
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?
Flags: needinfo?(ehsan)
Flags: needinfo?(botond)
This isn't a finished patch (as it doesn't work as per the above comment), but I figure I'll attach it anyways.
(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 cfe-dev@lists.llvm.org, 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.
Flags: needinfo?(botond)
Hmm, that AST looks a bit crazy to me!  I second Botond's suggestion.  :-)
Flags: needinfo?(ehsan)
Sent an email to cfe-dev. We'll see if we get any interesting responses.
Any responses yet?
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.
Attachment #8749359 - Attachment is obsolete: true
Product: Core → Firefox Build System
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>()
Assignee: nobody → sledru
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fe284f84898
Enable the readability-implicit-bool-conversion checker r=andi
https://hg.mozilla.org/mozilla-central/rev/6fe284f84898
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1478840
You need to log in before you can comment on or make changes to this bug.