Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullAway can't tell when the same if test is used to protect access to a member variable #60

Closed
NickButcher1 opened this issue Nov 24, 2017 · 3 comments

Comments

@NickButcher1
Copy link

NickButcher1 commented Nov 24, 2017

This is a simplified example because I can't share my code.

NullAway doesn't seem able to spot that this code is OK and reports

error: [NullAway] initializer method does not guarantee @nonnull fields mSomeString are initialized along all control-flow paths (remember to check for exceptions or early returns).

Obviously I could annotate mSomeString with @nullable but then I have to add a null check in someMethodUsedLater which I'd prefer to avoid. Maybe I just have to live with doing that?

This is an Android Activity.

private String mSomeString;

@Override
public void onCreate()
{
    super.onCreate();

    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N)
    {
        mSomeString = "SOMETHING";
    }
}

private someMethodUsedLater
{       
    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N)
    {
        int someValue = mSomeString.length();
    }
}
@msridhar
Copy link
Collaborator

Hi @NickButcher1, you're right that NullAway can't prove this code is safe, even though it is. I don't think we'll be adding sufficient reasoning about conditional nullness to handle this any time soon. Even if we could get it to work and run fast, I worry that in cases where it reported errors, things would get hard to understand.

For this case, I would argue that mSomeString should be @Nullable; what happens if some other method accesses the field without checking the condition first? In someMethodUsedLater, I would use a downcast, something like castToNonNull(mSomeString).length(). No matter how powerful we make NullAway, there will be patterns we can't handle, so I think adding a downcast operator is a good idea on any large code base.

@NickButcher1
Copy link
Author

Thanks for taking the time to look at this. I think you're right. I'll update my code.

@msridhar
Copy link
Collaborator

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants