1510972 - Java Warning: Boolean method is always inverted

Status: RESOLVED WONTFIX

(GeckoView :: General, defect, P5)

Description

[Emily Toop (:fluffyemily)]

Reports methods with a boolean return type, which are only used in a negated context. Due to performance reasons some methods might not be reported during in-editor highlighting.
For example: 
class C {
  boolean inverted() {
    return true;
  }

  void f() {
    if (!inverted()) {
      return;
    }
  }
  boolean member = !inverted();
} 

Affected Classes:

Cae608Decoder
Car708Decoder
DefaultOggSeeker
DefaultTrackOutput
DvbSubtitleReader
Environment
GeckoEditable
GeckoMediaDrmBridgeV21
GeckoSessionTestRule
InputDeviceUtils
MediaCodecInfo
MediaCodecRenderer
MediaDrmProxy
ParsableNalUnitBitArray
StackScroller

Comment 1

[Reece Martin]

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Reece Martin from comment #1)

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

Hi Reece, thanks for your help! I can assign this bug to you after you post a patch.

Have you built the "GeckoViewExample" test app yet? Downloading the code and build it are the first steps. Here are instructions:

https://mozilla.github.io/geckoview/contributor/geckoview-quick-start

If you have questions, you can ask in this bug report or join the #mobile channel on Mozilla's IRC server:

https://wiki.mozilla.org/IRC

If you don't have an IRC client, IRCCloud is a free and nice web client:

https://www.irccloud.com/

Comment 3

[Reece Martin]

(In reply to Chris Peterson [:cpeterson] from comment #2)

(In reply to Reece Martin from comment #1)

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

Hi Reece, thanks for your help! I can assign this bug to you after you post a patch.

Have you built the "GeckoViewExample" test app yet? Downloading the code and build it are the first steps. Here are instructions:

https://mozilla.github.io/geckoview/contributor/geckoview-quick-start

If you have questions, you can ask in this bug report or join the #mobile channel on Mozilla's IRC server:

https://wiki.mozilla.org/IRC

If you don't have an IRC client, IRCCloud is a free and nice web client:

https://www.irccloud.com/

Sounds good Chris, I'll get on it.

Comment 4

[Reece Martin]

Trying to get started on this bug, and there are a couple of things I am stuck on/confused about:

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Reece Martin from comment #4)

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Can you share an example of such a method? Perhaps we don't need to change all the reported methods or the method can be changed to return a more explicit success/failure type using an enum instead of a boolean. Methods with boolean parameters or return values are often a "code smell" because a method with two different code paths for a special case often needs to handle more special cases in the future. :)

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

If there are no tests for this code, then I don't think you need to write new tests... unless you would like to. :) Running and writing tests will add a lot of new challenges if this is your first Firefox bug.

Emily, what tool are you using to detect these inverted boolean methods?

Comment 6

[Reece Martin]

(In reply to Chris Peterson [:cpeterson] from comment #5)

(In reply to Reece Martin from comment #4)

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Can you share an example of such a method? Perhaps we don't need to change all the reported methods or the method can be changed to return a more explicit success/failure type using an enum instead of a boolean. Methods with boolean parameters or return values are often a "code smell" because a method with two different code paths for a special case often needs to handle more special cases in the future. :)

One example would be the Class SplineStackScroller in the StackScroller.java file in the package mozilla.gecko.gfx, with the method update that returns true if the update has been done and false if it wasn't done. There is also a boolean field in this same class mFinished that represents whether animation is currently in progress or not, and this was flagged as well.

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

If there are no tests for this code, then I don't think you need to write new tests... unless you would like to. :) Running and writing tests will add a lot of new challenges if this is your first Firefox bug.

Got it, thanks!

Comment 7

[Reece Martin]

I've just done a check for all the methods that were flagged, and most were in fact methods that seem appropriate to return the boolean value as they do now with some acting as an accessor to a private variable. Here is a select sample of method names for reference:

• isSupported
• shouldInitCodec
• isInstance
• contains
• isEmpty
• equals
• shouldProcessKey

I don't believe these should be simply inverted, and I'm not sure if using an enum type to return a simple boolean like this is necessary.

If I should change them to enum return types, how should I go about doing it? Creating a type for every single method involved?

Comment 8

[Chris Peterson [:cpeterson]]

I've just done a check for all the methods that were flagged, and most were in fact methods that seem appropriate to return the boolean value as they do now with some acting as an accessor to a private variable.
...
I don't believe these should be simply inverted, and I'm not sure if using an enum type to return a simple boolean like this is necessary.

Interesting. Sounds like this Java warning is not useful if it hasn't found any real bugs or confusing code. In that case, perhaps the appropriate fix for this bug is a code patch that disables this warning. :) Is this warning reported by javac? Is there is a command-line option to disable this warning?

Comment 9

[Reece Martin]

(In reply to Chris Peterson [:cpeterson] from comment #8)

Interesting. Sounds like this Java warning is not useful if it hasn't found any real bugs or confusing code. In that case, perhaps the appropriate fix for this bug is a code patch that disables this warning. :) Is this warning reported by javac? Is there is a command-line option to disable this warning?

I just did a rebuild using Mach and I did not see any of these warnings being reported in the console (the only ones reported were C and C++ warnings), so I doubt there is an option to disable these unless I am doing something wrong with the build.

Comment 10

[Chris Peterson [:cpeterson]]

Looks like this warning is from the Android Studio IDE, not javac.

https://www.jetbrains.com/help/idea/2016.1/invert-boolean.html
https://stackoverflow.com/questions/52648487/suppresswarnings-for-boolean-method-methodname-is-always-inverted

The warnings can be suppressed with a method annotation like @SuppressWarnings("BooleanMethodIsAlwaysInverted"). There's probably a way to disable them globally, but suppressing them for each method is good place to start. Here are some code examples from a random project:

https://github.com/jonasoreland/runnerup/pull/524/commits/073bb9cf1560b581169d9de521c164a724a8f39c

Comment 11

[Chris Peterson [:cpeterson]]

Correction: I spoke with the GeckoView developers and they think we should just disable this "BooleanMethodIsAlwaysInverted" warning globally instead of annotating the methods with warnings.

So your patch could create a new lint.xml file to disable this warning. We can use lint.xml to disable other bogus warnings in the future. Here are Android Studio instructions for using lint.xml:

https://developer.android.com/studio/write/lint.html#sample-lint.xml-file

Comment 12

[Reece Martin]

I did some digging around trying to make this work, but unfortunately, I've run into a few problems. Since this specific warning comes from the Android Studio/IntelliJ inspector instead of the Android linter, lint.xml is unable to disable the warning (it tells me that it does not recognize the issue ID "BooleanMethodIsAlwaysInverted". I tried digging around in the inspector, but besides being able to save the setting locally as well as exporting the setting (which requires manual import in order to be applied), there is no way to actually have it saved as a permanent setting in the repository. I'll try digging around some more, but if the GeckoView developers are okay with SuppressWarnings annotations, I can go in that route as well.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Reece Martin from comment #12)

I did some digging around trying to make this work, but unfortunately, I've run into a few problems. Since this specific warning comes from the Android Studio/IntelliJ inspector instead of the Android linter, lint.xml is unable to disable the warning (it tells me that it does not recognize the issue ID "BooleanMethodIsAlwaysInverted". I tried digging around in the inspector, but besides being able to save the setting locally as well as exporting the setting (which requires manual import in order to be applied), there is no way to actually have it saved as a permanent setting in the repository.

I don't have Android Studio, so I can't help here. Where does the Android Studio save your local setting? I would be surprised there isn't a way to save Android Studio settings to a file that can be checked into version control.

This Android doc page talks about disabling lint checks from Gradle. Perhaps that is an option?

https://developer.android.com/studio/write/lint#gradle

I'll try digging around some more, but if the GeckoView developers are okay with SuppressWarnings annotations, I can go in that route as well.

I think suppressing the warnings using @SuppressWarnings("BooleanMethodIsAlwaysInverted") annotations is much better than leaving the warnings around. :)

Comment 14

[Reece Martin]

(In reply to Chris Peterson [:cpeterson] from comment #13)

I don't have Android Studio, so I can't help here. Where does the Android Studio save your local setting? I would be surprised there isn't a way to save Android Studio settings to a file that can be checked into version control.

There is a file that I can export, but it would require manual importing by every other person in order for it to take effect.

This Android doc page talks about disabling lint checks from Gradle. Perhaps that is an option?

Unfortunately this doesn't work for the same reason that lint.xml doesn't work: it is not a lint warning.

I think suppressing the warnings using @SuppressWarnings("BooleanMethodIsAlwaysInverted") annotations is much better than leaving the warnings around. :)

I will go ahead and do that!

Comment 15

[Reece Martin]

Attachment: 1510972.patch

I've attached a patch that uses the @SuppressWarnings method of suppressing this bug!

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 9106786 [details] [diff] [review]
1510972.patch

Review of attachment 9106786 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just some questions below. I'm not an engineer on the GeckoView, so I won't be able to grant your patch an official "r+" review approval. But I can help you get your patch in shape so your real review will be easy. :)

For your real view, you will need to use the Phabricator code review system instead of attaching a patch file in Bugzilla. Phabricator is easier than it looks. Here are instructions for creating a Phabricator account (using your Bugzilla account) and then installing the `moz-phab` client.

https://moz-conduit.readthedocs.io/en/latest/

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/Environment.java
@@ +57,1 @@
>      public boolean isDebugBuild() {

Where is `isDebugBuild()` called with an inverted logic check? None of the references to `isDebugBuild()` I see in searchfox invert the return value:

https://searchfox.org/mozilla-central/search?q=isDebugBuild&case=true&path=mobile

::: mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/extractor/DefaultTrackOutput.java
@@ +183,4 @@
>    /**
>     * Returns whether the buffer is empty.
>     */
> +  @SuppressWarnings("BooleanMethodIsAlwaysInverted")

I don't think we should edit any of the files in the mobile/android/geckoview/src/thirdparty/ directory. Exoplayer is a third-party library. We'll probably need to update to a newer version of Explayer someday. Any code changes we make to our copy will make updating more difficult.

I think we should just accept these Exoplayer warnings for now. This is an example of why it would be nice if we could disable the BooleanMethodIsAlwaysInverted warnings globally.

Comment 17

[Polly McEldowney [:pollymce]]

This bug is 5 years old and the classes don't all exist any more.
The patch looks like it's just suppressing warnings.
Closing out as WONTFIX.
Please reopen if anyone feels this is still an issue and would like to update this bug with a more up to date list of classes.