pigeon objc static analysis problem with ternary conditional nil checks

[gaaclarke]

 - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:59:51: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:111:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:112:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:132:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:133:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:152:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:153:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
    - WARN  | [iOS] xcodebuild:  /private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/packages/video_player/video_player/ios/Classes/messages.m:169:37: warning: Converting a pointer value of type 'NSNumber * _Nullable' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue

From @jmagman:

Instead of (self.textureId ? self.textureId : [NSNull null]) you need to say (self.textureId != nil ? self.textureId : [NSNull null])

Spawned from:
flutter/plugins#2922

[jmagman]

Would be nice to run the static analyzer in CI for https://github.com/flutter/packages/tree/cf96cba3c50a410e9b450d0a24ea9d7e1c020c09/packages/pigeon/platform_tests/ios_unit_tests/ios to catch future issues.

[gaaclarke]

@jmagman I was looking into it. It's not clear what linter cocoapods uses. I was playing around with oclint but might take a bit of work to setup since it doesn't seem to support commandline compiler arguments. Otherwise I'll have to generate a fake podspec to run "pod lib lint" =T

[jmagman]

It's just the clang static analyzer with default warnings from a newly generated Xcode project. You don't need to use CocoaPods to see it.

[jmagman]

If the unit tests were running, I think you could add analyze to https://github.com/flutter/packages/blob/c9053f1fea56efd5a7d77bd5063994a573743ab2/packages/pigeon/run_tests.sh#L92-L97 combined with GCC_TREAT_WARNINGS_AS_ERRORS=YES.

[gaaclarke]

After the ios_unit_tests have been reenabled i tried:

cd platform_tests/ios_unit_tests/ios
xcodebuild analyze DEVELOPMENT_TEAM=$DEVELOPMENT_TEAM GCC_TREAT_WARNINGS_AS_ERRORS=YES

Despite "messages.m" containing:

- (NSDictionary *)toMap {
  return [NSDictionary
      dictionaryWithObjectsAndKeys:(self.query ? self.query : [NSNull null]), @"query",
                                   (self.anInt ? self.anInt : [NSNull null]), @"anInt",
                                   (self.aBool ? self.aBool : [NSNull null]), @"aBool", nil];
}

The analyzer didn't trigger any messages.

I tried to trigger problems in messages.m, like adding unused ivars, but couldn't get the analyzer to complain.

edit: Running the analyzer inside of xcode didn't flag the issue either.

[gaaclarke]

I was finally able to get the analyzer to trigger a problem by turning on CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER = YES to prove it works. It seems like the reported analyzer problem may be dependent on analyzer settings (of which I saw none that seemed applicable in the project settings) or a different version of xcode.

[jmagman]

It should be coming from CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION, which is on in a default Flutter (and Xcode) project.
I was able to reproduce the analyzer warning from flutter/plugins#2922 by removing the nil conditional check in -[FLTTextureMessage toMap] and running the analyzer Product > Analyze in Xcode (it's not a recently added clang warning, you're on a reasonably new version of Xcode so you should see it?).

- (NSDictionary *)toMap {
  return [NSDictionary
      dictionaryWithObjectsAndKeys:(self.textureId ? self.textureId : [NSNull null]),
                                   @"textureId", nil];
}

[gaaclarke]

I have CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION set to YES but still doesn't trigger a problem in 11.5.1. My guess is that clang redefined that warning. I also suspect my xcode version is newer than the one being used by CI (I can't find the link to the failure since there was a force push to verify).

I can still turn on the analyzer and we should be good as long as the analyzers never conflict and we accept there may be certain issues it doesn't catch that other variants will flag like this one.

[jmagman]

@gaaclarke The CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER is different. CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION is:

Warn when a number object, such as an instance of `NSNumber`, `CFNumberRef`, `OSNumber`, or `OSBoolean` is compared or converted to a primitive value instead of another object.

Did you try it on the video_player? That's where it actually showed up in the wild and where I reproduced it. I will check out the platform test and see why it's different.

[gaaclarke]

@jmagman Right, I know it's different. I was just showing that the analyzer flagged that problem, but not the one for CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION.