-2

I have this code:

@Override
public void onBindViewHolder(ViewHolder holder, int position) {
    if (position < getItemCount()) {
        holder.bind(mServiceInfo.agencies.get(position));
    }
}

@Override
public int getItemCount() {
    return mServiceInfo != null && mServiceInfo.agencies != null ? mServiceInfo.agencies.size() : 0;
}

Android Studio warns me in the onBindViewHolder method, saying that my mServiceInfo.agencies.get call might raise a NPE.

There is no way this can happen, because position is always a positive integer. I've even changed the test condition to: position >= 0 && position < getItemCount() but the warning is still shown.

Here's what it looks like:

Screenshot of warning in IDE window

How can I configure the IDE not to raise a warning here?

Please note that:

  • I don't want to disable the lint check or disable it for this statement. I know that lint checks can be disabled, that's not the point of my question here. Also, I like to avoid disabling checks because in maintenance the code can be changed and the comment asking the IDE not to care about one check may remain, which would cause undesired/unexpected behavior.
  • Apart from concurrency, there is no condition which would cause an NPE in the onBindViewHolder, so the IDE warning is not valid here
  • I do care about warnings, especially when it's about checking NPEs, because they can easily show where I did wrong and avoid a possible bug. Warnings are very important and I tend to have zero warnings when compiling, especially about when they detect possible bugs.
  • It is possible to change how the IDE behaves vs. NPE, this kind of answer is exactly what I had in mind when posting this question
Community
  • 1
  • 1
Benoit Duffez
  • 11,839
  • 12
  • 77
  • 125
  • 1
    Why are you so concerned with a warning? It is not an error. – Phantômaxx Feb 14 '16 at 11:15
  • 2
    It is saying you may get a null value from that container, does not have anything to do with your position value – Trash Can Feb 14 '16 at 11:17
  • @HrundiV.Bakshi: I like clean code and clean IDE status – Benoit Duffez Feb 14 '16 at 11:22
  • @Dummy: if either `mServiceInfo` or `mServiceInfo.agencies` is null, `getItemCount()` will return 0. `position` is a positive integer, so `position < getItemCount()` will never be true if there is a null object. – Benoit Duffez Feb 14 '16 at 11:23
  • As @DavidMedenjak suggested, instruct Lint not to bore you anymore. – Phantômaxx Feb 14 '16 at 11:25
  • @HrundiV.Bakshi: I know how to disable a lint check for a statement, I wanted the IDE to figure this on its own. – Benoit Duffez Feb 14 '16 at 11:29
  • Then rewrite the IDE. Even if I think it's actually correct. – Phantômaxx Feb 14 '16 at 11:30
  • 1
    If either mServiceInfo or mServiceInfo.agencies is null, you will get a null pointer exception on them unless your implementation returns 0 when they are null – Trash Can Feb 14 '16 at 11:31
  • @Dummy: the implementation is in the question. It does return 0 if either is null. – Benoit Duffez Feb 14 '16 at 11:35
  • You are not showing us enough code to make the determination that neither `mServiceInfo` nor `mServiceInfo.agencies` can never be `null`. For all we know Android Studio's warning is entirely valid (eg think of what can happen under concurrent access) – Mark Rotteveel Feb 14 '16 at 11:43
  • They can be null, but `getItemCount` handles their nullity. – Benoit Duffez Feb 14 '16 at 11:44
  • 1
    See my comment edit. You could have a [TOCTTOU problem](https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use) when a concurrent thread does make one of them `null`. – Mark Rotteveel Feb 14 '16 at 11:45
  • Please show me a combination of `position`, `mServiceInfo` and `mServiceInfo.agencies` that will raise a NPE in `onBindViewHolder`. There is none. – Benoit Duffez Feb 14 '16 at 11:45
  • Ah, thanks for the first valid point. Fair enough, I didn't think about concurrency. Thanks. – Benoit Duffez Feb 14 '16 at 11:47
  • Sure there is: Thread1 calls `getItemCount` which returns 1, Thread2 then makes `mServiceInfo` or `mServiceInfo.agencies` `null`, Thread1 dereferences them and gets a `NullPointerException`. – Mark Rotteveel Feb 14 '16 at 11:47
  • You haven't mentioned concurrency when I wrote that comment. Concurrency is a very valid point in this case. But I'm sure the IDE wouldn't remove the warning if my code was synchronized to ensure that no thread can touch either object between the two calls. So my question remains. – Benoit Duffez Feb 14 '16 at 11:51

1 Answers1

2

The warning occurs because the static analysis of your code says one of the fields may be null. This is either because of accessed fields not set in the constructor, @Nullable annotations, or some other static analysis things.

First, be sure you are handling the NPE and that it can not occur. The warning is there for a reason. If you checked that, you have 4 options:

  • Actually do a null check
  • Refactor your code, remove @Nullable, always initialize the field
  • Disable the lint check completely, or
  • Disable the check just for some parts

I recommentd the last option, since it will also work with version control systems and other work stations. (If you are sure that you know what you are doing.)

  1. Press Alt+Enter
  2. Select Inspection 'Constant Conditions & Exceptions' options
  3. Chose your option.

You will see a preview of how far the annotation will reach, usually a single comment for the statement will suffice:

enter image description here

This will add the following comment above, which you could also add manually:

//noinspection ConstantConditions
getSupportActionBar().setTitle("My title");
David Medenjak
  • 33,993
  • 14
  • 106
  • 134
  • That's rather a workaround, I'd rather have the IDE understand that the statement wouldn't raise any NPE – Benoit Duffez Feb 14 '16 at 11:21
  • The IDE / lint is actually checking your code. If you have some `@Nullable` annotated code it will raise this warning if not checking the result. Also if you don't initialize your fields in every possible path, and more. The only options you have is rewrite your code, add a null check or suppress the warning. – David Medenjak Feb 14 '16 at 11:24