Page MenuHomePhabricator

[Diagnostics] Added support for -Wint-in-bool-context
AbandonedPublic

Authored by xbolva00 on Jun 10 2019, 8:54 AM.

Details

Summary

I consider -Wint-in-bool-context as quite useful warning so I ported it to Clang.

-Wint-in-bool-context is on by default, it is part of GCC's -Wall so I make it same here.

Current patch passes all tests for -Wint-in-bool-context in GCC's test-suite.

Fixes PR31201

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
xbolva00 added inline comments.Jun 17 2019, 1:01 PM
include/clang/Basic/DiagnosticSemaKinds.td
5624

Bool context - places where we expect boolean expressions.

If you have idea for better terminology, I am happy to apply it.

Yeah, I will remove “&&” suggestion..

5626

Oh, inconsistency. I will use boolean context than bool context :) (if no suggestions)

5640

Yes, we could move it there.

xbolva00 updated this revision to Diff 205364.Jun 18 2019, 8:38 AM

Addressed review notes. Thanks @aaron.ballman !

xbolva00 updated this revision to Diff 205365.Jun 18 2019, 8:41 AM

Updated forgotten test file.

Is it ok now?

xbolva00 marked an inline comment as done.Jun 19 2019, 9:18 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5627

I will this line.

xbolva00 marked an inline comment as done.Jun 19 2019, 9:21 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5627
  • Ehh.. I will fix this line to use "did you mean".
ArnaudBienner added inline comments.Jun 19 2019, 3:06 PM
lib/Sema/SemaChecking.cpp
11123

Shouldn't that be "const auto*" instead?
I'm surprised dyn_cast casts away const qualifier, but FWIW having the returned pointer being const makes clearer the variable pointed by this pointer won't be modified.
But maybe this was intended to not make the code too verbose?

xbolva00 updated this revision to Diff 205696.Jun 19 2019, 4:22 PM
xbolva00 marked 3 inline comments as done.

Addressed review notes. Thanks!

aaron.ballman added inline comments.Jun 21 2019, 12:28 PM
include/clang/Basic/DiagnosticSemaKinds.td
5624

I don't have a good recommendation for a replacement for "boolean context", so I suppose we can leave it.

5640

Don't *all* of these effectively boil down to a tautological comparison?

xbolva00 marked an inline comment as done.Jun 21 2019, 12:46 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5640

So you propose put IntInBoolContext into TautologicalConstantCompare ?

aaron.ballman added inline comments.Jun 21 2019, 12:55 PM
include/clang/Basic/DiagnosticSemaKinds.td
5640

That's what I am thinking, but I was curious if you agreed.

xbolva00 marked an inline comment as done.Jun 21 2019, 1:10 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5640

But that is not enabled by -Wall :/ I would like to have this under -Wall.

xbolva00 marked an inline comment as done.Jun 21 2019, 2:06 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5640

I got it. Yes, we can "warn" even if no -Wall.

xbolva00 updated this revision to Diff 206075.Jun 21 2019, 2:07 PM

In terms of the code, I think this is ready to go. However, I'm still not happy with '*' in boolean context as a diagnostic. Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious. blah in boolean context will always evaluate to true|false optionally followed by ; did you mean yada? would make it more obvious what is wrong with the code.

Also, I'm not certain why this should be enabled by default when the other tautological comparison categories are not. I think it should follow the rest of the tautological comparisons in that regard, but I also would have expected those to be on by default. @rsmith, what are your opinions here?

include/clang/Basic/DiagnosticGroups.td
767

Spurious change.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

xbolva00 added a comment.EditedJun 27 2019, 5:26 AM

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

Alternative “* in boolean context; did you mean to compare it?”

xbolva00 updated this revision to Diff 206842.Jun 27 2019, 6:02 AM
xbolva00 marked an inline comment as done.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

Personally, my feeling is that this new diagnostic group belongs under the tautological compare group. That group is currently off by default, but I'm wondering if we want it to be on by default (in a subsequent patch), or whether we're okay having parts of it on by default and parts off by default. I'm hoping @rsmith will voice an opinion here.

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

I don't think that actually improves anything. I do not think we want a "did you mean" in this case because I'm not confident anyone can guess what a user was trying for in this situation. However, telling the user the result of the computation does give them very useful information -- it tells them the result will always be tautologically true or false (which is what the other tautological warnings do, as well). As it stands, the current diagnostic wording doesn't help the user understand what's wrong with their code. I'd like to correct that deficiency before we commit this.

xbolva00 updated this revision to Diff 207623.Jul 2 2019, 1:59 PM

Implemented suggested "mul in bool context" diagnostic.

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

Personally, my feeling is that this new diagnostic group belongs under the tautological compare group. That group is currently off by default, but I'm wondering if we want it to be on by default (in a subsequent patch), or whether we're okay having parts of it on by default and parts off by default. I'm hoping @rsmith will voice an opinion here.

“Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”

I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.

Okay, we could make it better and append “; did you mean “index * 3 != 0”?

I don't think that actually improves anything. I do not think we want a "did you mean" in this case because I'm not confident anyone can guess what a user was trying for in this situation. However, telling the user the result of the computation does give them very useful information -- it tells them the result will always be tautologically true or false (which is what the other tautological warnings do, as well). As it stands, the current diagnostic wording doesn't help the user understand what's wrong with their code. I'd like to correct that deficiency before we commit this.

Thanks

I decided to implement your suggestions.

This comment was removed by xbolva00.
RKSimon resigned from this revision.Jul 14 2019, 6:38 AM

Ping.

Seems like @rsmith is busy, so I added @jfb as a reviewer.

aaron.ballman added inline comments.Jul 28 2019, 10:00 AM
test/SemaCXX/warn-int-in-bool-context.cpp
100

Why do we want to diagnose this case in C++ but not in C?

xbolva00 marked an inline comment as done.Jul 28 2019, 10:10 AM
xbolva00 added inline comments.
test/SemaCXX/warn-int-in-bool-context.cpp
100

I think we can and should but for unknown reason (for me) this is GCC’s behaviour.

Maybe it is too noisy for C codebases?

Maybe introduce -Wenum-in-bool-context as subgroup of -Wint-in-bool-context? And enable it for C and C++?

aaron.ballman added inline comments.Jul 28 2019, 10:20 AM
test/SemaCXX/warn-int-in-bool-context.cpp
100

I think it's just a bug in GCC (they use separate frontends for C and C++, so these sort of differences crop up sometimes). I think this should be safe to diagnose the same in C and C++.

xbolva00 marked an inline comment as done.Jul 29 2019, 11:30 AM
xbolva00 added inline comments.
test/SemaCXX/warn-int-in-bool-context.cpp
97

@aaron.ballman : In C (but not in C++ ugh?) mode we have: 'use of logical '||' with constant operand' here..

/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18: warning: use of logical '||' with constant operand

if (f == apple || orange) // expected-warning {{enum constant in boolean context}}
               ^  ~~~~~~

/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18: note: use '|' for a bitwise operation

if (f == apple || orange) // expected-warning {{enum constant in boolean context}}
               ^~
               |

/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:21: warning: enum constant in boolean context

if (f == apple || orange) // expected-warning {{enum constant in boolean context}}
jfb added inline comments.Jul 29 2019, 9:31 PM
include/clang/Basic/DiagnosticSemaKinds.td
5624

IMO it's clearer to say "assigning the result of a multiplication to a boolean variable always yields {true|false}"

5627

Same here, "assigning the result of a left shift to a boolean variable always yields {true|false}"

Same for all the other ones...

test/Sema/warn-int-in-bool-context.c
27

I'm not sure the "did you mean" part is helpful. Do we have data showing that this is what people actually mean?

33

Why wouldn't think one warn?

34

Why does this one warn? It doesn't always yield the same result.

test/Sema/warn-unreachable.c
147

It seems like here the "will never be executed" warning is more useful. Do we want to emit both?

test/SemaCXX/warn-int-in-bool-context.cpp
100

Agreed.

xbolva00 marked 3 inline comments as done.Jul 30 2019, 12:39 PM
xbolva00 added inline comments.
test/Sema/warn-int-in-bool-context.c
27

No, no data, just what GCC suggests.

34

GCC warns here..

test/Sema/warn-unreachable.c
147

Useful but -Wunreachable-code is disabled by default (not part of -Wall).

xbolva00 marked an inline comment as done.Jul 30 2019, 12:42 PM
xbolva00 added inline comments.
test/Sema/warn-int-in-bool-context.c
33

Ah, right. Thanks !

I forgot there is an unary minus operator..

Will fix it.

jfb added inline comments.Jul 30 2019, 12:47 PM
test/Sema/warn-int-in-bool-context.c
27

I would drop it without knowing that it's actually what people usually mean.

34

Our goal isn't to achieve GCC warning parity. I like warnings that fire when it's extremely likely a bug. Here the diagnostic seems to be inconsistent with other instances of this diagnostic. Maybe this should warn, but then other cases should as well (such as a?3:-2 above). Or maybe it shouldn't warn at all.

test/Sema/warn-unreachable.c
147

I don't think we want two diagnostics when one will do.

xbolva00 marked 4 inline comments as done and an inline comment as not done.Jul 30 2019, 1:07 PM
xbolva00 added inline comments.
test/Sema/warn-int-in-bool-context.c
27

Leave just “<<' in boolean context“ seems not ideal for me as a diag message.

Or is it ok for you?

34

I will drop it and leave only “warn_integer_constants_in_conditional_always_true” diag.

test/Sema/warn-unreachable.c
147

I will drop mul handling then..

test/SemaCXX/warn-int-in-bool-context.cpp
97

What is your opinion here (^^) , @jfb ?

Thanks.

jfb added inline comments.Jul 30 2019, 1:13 PM
test/Sema/warn-int-in-bool-context.c
27

As I said above, I don't think you should say "boolean context". It's not something people will understand. Here we're assigning to a boolean, say that.

test/Sema/warn-unreachable.c
147

No, my feedback isn't about multiplication in particular. My feedback is that it's not useful to have both warnings. We should just emit a single warning, and it should be maximally helpful. Here it's useful to say that calledFunc() will never be executed because 0*x is always false.

test/SemaCXX/warn-int-in-bool-context.cpp
97

C, C++, ObjC, and ObjC++ should emit different diagnostics here. That being said, none of these diagnostics are particular easy to understand. I don't think we should ever say "boolean context" anywhere.

xbolva00 marked 2 inline comments as done and an inline comment as not done.Jul 30 2019, 1:57 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5624

But it is very specific.. cannot be used for example here:

bool t() { return 0 * x; }

In the place where the check is performed we know nothing whether expr is assigned/returned/etc.

I don’t believe we have to produce message with such details.

test/SemaCXX/warn-int-in-bool-context.cpp
97

So I will make this as it was - C++ only.

jfb added inline comments.Jul 30 2019, 2:04 PM
include/clang/Basic/DiagnosticSemaKinds.td
5624

All I'm saying is that "boolean context" ins't something people will understand.
Maybe "converting the result of a multiplication to a boolean always yields {true|false}"?

test/SemaCXX/warn-int-in-bool-context.cpp
97

It'll work in the other languages though, right?

xbolva00 marked 2 inline comments as done.Jul 30 2019, 2:34 PM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5624

+1, like it

test/SemaCXX/warn-int-in-bool-context.cpp
97

Sight, making it C++ only leaves C with not diagnosed case:

_Bool f() { return orange; }

And I don’t know why ‘use of logical || with constant operand’ is C only :/..

aaron.ballman added inline comments.Jul 31 2019, 8:58 AM
test/Sema/warn-int-in-bool-context.c
27

As I said above, I don't think you should say "boolean context". It's not something people will understand. Here we're assigning to a boolean, say that.

Strong +1.

As for the "did you mean", I actually see that as somewhat reasonable text. << is a reasonable typo for < (which yields a bool), similar for >> and >. However, if we had data to back up that presumption, it would be better.

test/SemaCXX/warn-int-in-bool-context.cpp
97

And I don’t know why ‘use of logical || with constant operand’ is C only :/..

It may be worth doing the svn blame dance to see if it's discussed as part of the original review. One possibility is that || can be overloaded in C++ and so perhaps || with a constant operand is not a bug. But that seems like pretty weak rationale (we can always see if there's an overloaded || and not issue the diagnostic in that circumstance).

xbolva00 marked an inline comment as done.Jul 31 2019, 9:19 AM
xbolva00 added inline comments.
test/SemaCXX/warn-int-in-bool-context.cpp
97

But even if we fix ‘use of logical .. with constants” for C++, we the break this patch (clang will produce two similar warnings) and if we remove it from this patch, some cases stay undiagnosed:

bool foo() return orange;

In order to support this case, we cant use this place to implement the check.

Not good..

C FE:

-IfStmt 0x55a225bc4dc8 <line:39:3, line:40:12>

| |-BinaryOperator 0x55a225bc4d48 <line:39:7, col:21> 'int' '||'
| | |-BinaryOperator 0x55a225bc4d08 <col:7, col:12> 'int' '=='
| | | |-ImplicitCastExpr 0x55a225bc4cf0 <col:7> 'int' <IntegralCast>
| | | | `-ImplicitCastExpr 0x55a225bc4cd8 <col:7> 'enum fruit':'enum fruit' <LValueToRValue>
| | | |   `-DeclRefExpr 0x55a225bc4c98 <col:7> 'enum fruit':'enum fruit' lvalue ParmVar 0x55a225bc4a90 'f' 'enum fruit':'enum fruit'
| | | `-DeclRefExpr 0x55a225bc4cb8 <col:12> 'int' EnumConstant 0x55a225bc4610 'apple' 'int'
| | `-DeclRefExpr 0x55a225bc4d28 <col:21> 'int' EnumConstant 0x55a225bc4520 'orange' 'int'

C++ FE:
IfStmt 0x55cfd2ae5d20 <line:39:3, line:40:12>

| |-BinaryOperator 0x55cfd2ae5ca0 <line:39:7, col:21> 'bool' '||'
| | |-BinaryOperator 0x55cfd2ab9940 <col:7, col:12> 'bool' '=='
| | | |-ImplicitCastExpr 0x55cfd2ab9910 <col:7> 'int' <IntegralCast>
| | | | `-ImplicitCastExpr 0x55cfd2ab98f8 <col:7> 'enum fruit':'fruit' <LValueToRValue>
| | | |   `-DeclRefExpr 0x55cfd2ab98b8 <col:7> 'enum fruit':'fruit' lvalue ParmVar 0x55cfd2ab96b0 'f' 'enum fruit':'fruit'
| | | `-ImplicitCastExpr 0x55cfd2ab9928 <col:12> 'int' <IntegralCast>
| | |   `-DeclRefExpr 0x55cfd2ab98d8 <col:12> 'fruit' EnumConstant 0x55cfd2ab8ef8 'apple' 'fruit'
| | `-ImplicitCastExpr 0x55cfd2ab9980 <col:21> 'bool' <IntegralToBoolean>
| |   `-DeclRefExpr 0x55cfd2ab9960 <col:21> 'fruit' EnumConstant 0x55cfd2ab8e08 'orange' 'fruit'

https://github.com/llvm-mirror/clang/blob/9b127f3b44e685cbe513595b7e0115b0884b0604/lib/Sema/SemaExpr.cpp#L6701

I will split enum constants handling to other patch in near future.

xbolva00 abandoned this revision.Aug 30 2019, 8:52 AM

It looks like bits of this landed recently. I don't know which of the commits it was, so I'm commenting here: The -Wint-in-bool context that clang emits are very useful and have found several bugs in Chromium, with just one false positive (which was in confusing code, so we changed that one too). Thank you for this useful warning :)

(https://crbug.com/1007367 lists what this founds, if you're curious.)

Happy to hear :)