`-Wbitwise-instead-of-logical` is emitted 4 times for a single `&` operator

[Quuxplusone]

Bugzilla Link	PR52226
Status	NEW
Importance	P enhancement
Reported by	Nico Weber (nicolasweber@gmx.de)
Reported on	2021-10-19 12:34:34 -0700
Last modified on	2021-10-19 13:38:09 -0700
Version	unspecified
Hardware	PC All
CC	david.bolvansky@gmail.com, llvm-bugs@lists.llvm.org, neeilans@live.com, richard-llvm@metafoo.co.uk, rjmccall@apple.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

% cat test.mm
// objc/objc.h does this based on __OBJC_BOOL_IS_BOOL, which
// clang/lib/Basic/Targets/AArch64.cpp defines by setting
// `UseSignedCharForObjCBool = false;`.
typedef bool BOOL;
#define YES __objc_yes

@interface Foo
@property(nonatomic, assign) BOOL shouldEnableSave;
- (void)foo;
@end

@implementation Foo
@synthesize shouldEnableSave;
- (void)foo {
  self.shouldEnableSave = [self checkIfValidSite] & [self checkIfValidPassword];
}
- (BOOL)checkIfValidSite { return YES; }
- (BOOL)checkIfValidPassword { return YES; }
@end

% out/gn/bin/clang -c test.mm --target=arm64-darwin-macos -Wall
test.mm:15:27: warning: use of bitwise '&' with boolean operands [-Wbitwise-
instead-of-logical]
  self.shouldEnableSave = [self checkIfValidSite] & [self checkIfValidPassword];
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                  &&
test.mm:15:27: note: cast one or both operands to int to silence this warning
test.mm:15:27: warning: use of bitwise '&' with boolean operands [-Wbitwise-
instead-of-logical]
  self.shouldEnableSave = [self checkIfValidSite] & [self checkIfValidPassword];
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                  &&
test.mm:15:27: note: cast one or both operands to int to silence this warning
test.mm:15:27: warning: use of bitwise '&' with boolean operands [-Wbitwise-
instead-of-logical]
  self.shouldEnableSave = [self checkIfValidSite] & [self checkIfValidPassword];
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                  &&
test.mm:15:27: note: cast one or both operands to int to silence this warning
test.mm:15:27: warning: use of bitwise '&' with boolean operands [-Wbitwise-
instead-of-logical]
  self.shouldEnableSave = [self checkIfValidSite] & [self checkIfValidPassword];
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                  &&
test.mm:15:27: note: cast one or both operands to int to silence this warning

This is a great warning, but emitting it once should be sufficient :)

[Quuxplusone]

Almost zero experience with ObjC++ but what I see... AST is really weird here

https://godbolt.org/z/Wrq6sEe1M

  | `-CompoundStmt <col:13, line:16:1>
  |   `-PseudoObjectExpr <line:15:3, col:79> 'bool'
  |     |-BinaryOperator <col:3, col:79> 'int' '='
  |     | |-ObjCPropertyRefExpr <col:3, col:8> '<pseudo-object type>' lvalue objcproperty Kind=PropertyRef Property="shouldEnableSave" Messaging=Setter
  |     | | `-OpaqueValueExpr <col:3> 'Foo *'
  |     | |   `-ImplicitCastExpr <col:3> 'Foo *' <LValueToRValue>
  |     | |     `-DeclRefExpr <col:3> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     | `-OpaqueValueExpr <col:27, col:79> 'int'
  |     |   `-BinaryOperator <col:27, col:79> 'int' '&'
  |     |     |-ImplicitCastExpr <col:27, col:49> 'int' <IntegralCast>
  |     |     | `-ObjCMessageExpr <col:27, col:49> 'BOOL':'bool' selector=checkIfValidSite
  |     |     |   `-ImplicitCastExpr <col:28> 'Foo *' <LValueToRValue>
  |     |     |     `-DeclRefExpr <col:28> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |     `-ImplicitCastExpr <col:53, col:79> 'int' <IntegralCast>
  |     |       `-ObjCMessageExpr <col:53, col:79> 'BOOL':'bool' selector=checkIfValidPassword
  |     |         `-ImplicitCastExpr <col:54> 'Foo *' <LValueToRValue>
  |     |           `-DeclRefExpr <col:54> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |-OpaqueValueExpr <col:3> 'Foo *'
  |     | `-ImplicitCastExpr <col:3> 'Foo *' <LValueToRValue>
  |     |   `-DeclRefExpr <col:3> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |-OpaqueValueExpr <col:27, col:79> 'int'
  |     | `-BinaryOperator <col:27, col:79> 'int' '&'
  |     |   |-ImplicitCastExpr <col:27, col:49> 'int' <IntegralCast>
  |     |   | `-ObjCMessageExpr <col:27, col:49> 'BOOL':'bool' selector=checkIfValidSite
  |     |   |   `-ImplicitCastExpr <col:28> 'Foo *' <LValueToRValue>
  |     |   |     `-DeclRefExpr <col:28> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |   `-ImplicitCastExpr <col:53, col:79> 'int' <IntegralCast>
  |     |     `-ObjCMessageExpr <col:53, col:79> 'BOOL':'bool' selector=checkIfValidPassword
  |     |       `-ImplicitCastExpr <col:54> 'Foo *' <LValueToRValue>
  |     |         `-DeclRefExpr <col:54> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |-OpaqueValueExpr <col:27, col:79> 'bool'
  |     | `-ImplicitCastExpr <col:27, col:79> 'bool' <IntegralToBoolean>
  |     |   `-OpaqueValueExpr <col:27, col:79> 'int'
  |     |     `-BinaryOperator <col:27, col:79> 'int' '&'
  |     |       |-ImplicitCastExpr <col:27, col:49> 'int' <IntegralCast>
  |     |       | `-ObjCMessageExpr <col:27, col:49> 'BOOL':'bool' selector=checkIfValidSite
  |     |       |   `-ImplicitCastExpr <col:28> 'Foo *' <LValueToRValue>
  |     |       |     `-DeclRefExpr <col:28> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     |       `-ImplicitCastExpr <col:53, col:79> 'int' <IntegralCast>
  |     |         `-ObjCMessageExpr <col:53, col:79> 'BOOL':'bool' selector=checkIfValidPassword
  |     |           `-ImplicitCastExpr <col:54> 'Foo *' <LValueToRValue>
  |     |             `-DeclRefExpr <col:54> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |     `-ObjCMessageExpr <col:8> 'void' selector=setShouldEnableSave:
  |       |-OpaqueValueExpr <col:3> 'Foo *'
  |       | `-ImplicitCastExpr <col:3> 'Foo *' <LValueToRValue>
  |       |   `-DeclRefExpr <col:3> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |       `-OpaqueValueExpr <col:27, col:79> 'bool'
  |         `-ImplicitCastExpr <col:27, col:79> 'bool' <IntegralToBoolean>
  |           `-OpaqueValueExpr <col:27, col:79> 'int'
  |             `-BinaryOperator <col:27, col:79> 'int' '&'
  |               |-ImplicitCastExpr <col:27, col:49> 'int' <IntegralCast>
  |               | `-ObjCMessageExpr <col:27, col:49> 'BOOL':'bool' selector=checkIfValidSite
  |               |   `-ImplicitCastExpr <col:28> 'Foo *' <LValueToRValue>
  |               |     `-DeclRefExpr <col:28> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'
  |               `-ImplicitCastExpr <col:53, col:79> 'int' <IntegralCast>
  |                 `-ObjCMessageExpr <col:53, col:79> 'BOOL':'bool' selector=checkIfValidPassword
  |                   `-ImplicitCastExpr <col:54> 'Foo *' <LValueToRValue>
  |                     `-DeclRefExpr <col:54> 'Foo *' lvalue ImplicitParam 0x557b26599ee0 'self' 'Foo *'

So for this AST, 4 warnings are technically correct.

Maybe John McCall (rjmccall) could give us some hints how we can fix this issue?

Otherwise, if this is very annoying in practise for ObjC++, I could simply
disable this warning for ObjC/ObjC++.

[Quuxplusone]

(I'd rather have the warnings 4 times than not have it, for what it's worth.)

[Quuxplusone]

There's something in this diagnostic that's walking the expression tree, and it's probably naively walking from an OpaqueValueExpr to its source expression. This must be getting done explicitly because the source expression of an OVE is not part of its children(). Visiting the source expression like this can cause the source expression to be visited multiple times when the OVE appears at multiple places in the expression tree (and allowing that is why OVE exists, so that isn't a bug). OVEs always appear within larger constructs that "bind" them, and generally the fix to these bugs is to walk into the OVE's source expression at this binding point rather than unconditionally. If we don't have a recursive walker that does this already, that would be reasonable to add.

[Quuxplusone]

Thanks for the explanation, logic behind is a bit more clear now.

It is implemented in AnalyzeImplicitConversions (SemaChecking.cpp). There are many other warnings there, so they probably have same issue with OVE, it seems.