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

Annonymous class in field initializer needs to treat surrounding class as under initialization #904

Open
mh-dm opened this issue Sep 5, 2016 · 10 comments

Comments

@mh-dm
Copy link

mh-dm commented Sep 5, 2016

No error about clear NPE when calling function in annonymous class from constructor.
Discovered while discussing #903. Tested with v2.1.1.

class Foo {
  private final Object mBar;
  private final Runnable mRunnable =
        new Runnable() {
          @Override
          public void run() {
            // No problem, even though it NPEs.
            mBar.toString();
          }
        };

  public Foo() {
    mRunnable.run();
    mBar = "";
  }

  public static void main(String[] args) {
    new Foo();
  }
}
@wmdietl wmdietl changed the title No error about clear NPE when calling function in annonymous class from constructor Annonymous class in field initializer needs to treat surrounding class as under initialization Sep 5, 2016
@wmdietl
Copy link
Member

wmdietl commented Sep 5, 2016

Final fields without initializers might get set after the anonymous class is used, resulting in an NPE.
Final fields with initializers should be OK to access.
In general, the mBar access in run should be treated as Foo.this.mBar where the Foo instance is under initialization.

Test case added at checker/tests/nullness/init/Issue904.java.

@mh-dm
Copy link
Author

mh-dm commented Sep 5, 2016

Also applies to inner classes. This related case also doesn't have any errors:

class Foo {
  private final Object mBar;
  class Inner {
      void run() {
        // No problem, even though it NPEs.
        mBar.toString();
      }
    };

  public Foo() {
    new Inner().run();
    mBar = "";
  }

  public static void main(String[] args) {
    new Foo();
  }
}

@mh-dm
Copy link
Author

mh-dm commented Sep 5, 2016

Also, to avoid too many false positives if this issue is fixed, can the following be added as a test case as well?

class Foo {
  private final Object mBar;
  private final Runnable mRunnable =
        new Runnable() {
          @Override
          public void run() {
            mBar.toString();
          }
        };
  private class Inner {
      void run() {
        mBar.toString();
      }
    };

  public Foo() {
    mBar = "";
  }

  public void shouldGiveNoErrors() {
    mRunnable.run();
    new Inner().run();
  }
}

(It currently has no errors, as intended)

@wmdietl wmdietl self-assigned this Sep 7, 2016
@mernst mernst added this to the High milestone Sep 7, 2016
@NazguL86
Copy link

NazguL86 commented Dec 7, 2018

Resuming back this thread as it's still an open issue, here's another example I'm running into:

public class AccountStateCheck  {

    @Nullable
    private  JobFinishedCallback mJobFinishedCallback;

    private JobFinishedCallback mClassJobFinishedListener = new JobFinishedCallback() {
       @Override
        public void onSuccess() {
            mJobFinishedCallback = null;
            mJobFinishedCallback.onSuccess();  // <-- NOT CAUGHT
        }
    };
...

This is still an open issue, can we please have an update?
Annonymous class in field initializer are pretty common and would be a miss to all CheckerFramework's customers to not have the null checker running on those.

@wmdietl
Copy link
Member

wmdietl commented Dec 7, 2018

@NazguL86 Can you please provide a complete example?
I tried the following:

import org.checkerframework.checker.nullness.qual.Nullable;
class Test {
  @Nullable Object f;
  Runnable r = new Runnable() {
      public void run() {
          f = null;
          f.toString();
      }
  };
}

For which the Nullness Checker gives: Error: [dereference.of.nullable] dereference of possibly-null reference f on the f.toString() line.
So my example must miss something essential to reproduce your issue.

@NazguL86
Copy link

NazguL86 commented Dec 7, 2018

Thanks for the quick follow up. The same example you copied it's not throwing any errors for me, so perhaps this has been addressed in a newer version of CheckerFramework that I'm not consuming yet. Would you be able to point the version / change that addressed this issue by any chance?

@wmdietl
Copy link
Member

wmdietl commented Dec 7, 2018

I tried with master and the 2.5.4 release, both of which report the error.
I don't remember anything recent that would have changed this.
What version are you using?

@NazguL86
Copy link

NazguL86 commented Dec 7, 2018

I have tried using 2.5.3 and 2.5.6 versions, will try with latest 2.5.8 next.

@NazguL86
Copy link

NazguL86 commented Dec 8, 2018

I think I figured out the difference, it seems that it's due to using the "onlyDefs" option making the CheckerFramework behave differently for anonymous inner initializers.
I'm currently using CheckerFramework on Android and added the following:

build.gradle

android {
...
 defaultConfig {
 ...
 javaCompileOptions {
            annotationProcessorOptions {
                arguments = ["onlyDefs": "^com.example.test.Test"]
           }
}
}

This doesn't signal the error on the anonymous class initializer, while if I run CF on the entire package it does. Unfortunately I have to use 'onlyDefs' since the project I'm working is very large and not fully annotated, so I need to target specific classes via 'onlyDefs'.

@wmdietl
Copy link
Member

wmdietl commented Dec 8, 2018

I've filed #2227 to keep track of this issue. Let's continue discussion there.

smillst added a commit that referenced this issue Sep 15, 2020
This pull request corrects the type of both implicit and explicit `this` references by correctly computing the enclosing type of `this`.

This pull request fixes two kinds of bugs: 
1. The type of `Outer.this` when used in an inner class.   (Issues #352, #2208, and #3561.)
2. For the Initialization Checker, the type of `Outer.this` when used in an anonymous class declared in a constructor. (Issues #354, part of #904, and #3408)

#409 (and other duplicate issues) is a related bug in the Initialization Checker, but isn't fixed by this PR.

Closes #352, closes #354, closes #2208, closes #3266, closes #3408 and closes #3561.
cpovirk pushed a commit to cpovirk/checker-framework that referenced this issue Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants