Fix #657 by splitting field initialization region into smaller regions

[nimakarimipour]

Note: This PR can wait until the next release of NullAway (0.10.2).

This PR resolves #657 by splitting the field initialization region into smaller regions. We currently only consider the value "null", for all errors/fixes reported in this regions. In large classes which they have many number of field, this can easily create a lot builds as they all have the value "null" for enclosing method causing conflicts in regions.

This PR generalizes the concept of enclosing method to enclosing member, and breaks a single regions of field initialization regions into a separate region for each field declaration and reduce the conflicts in regions dramatically.

The equivalent for enclosing member is described:
The enclosing member for fix or error is:

• If it is triggered inside a method, the enclosing member will be the enclosing method.
• If it is enclosed by a field declaration statement, the enclosing member will be the declared field.
• If none of the above, the enclosing member will be "null" (used for static initialization region).

With the current approach the following errors will have the values below:

class C {
    Field foo1 = Field(null); // Passing nullable, enclMethod = "null"
    Field foo2 = null; // assigning nullable, enclMethod = "null"
    static Field foo3 3 = null; // assigning nullable, enclMethod = "null"
    {
          foo3 = null; // assigning nullable, enclMethod = "null"
    }

   void bar(){
         this.foo1 = null; // assigning nullable, enclMethod = "bar()"
   }
}

From the point of view of Annotator, that is 4 conflicts in regions as they all have enclMethod = "null".

This can be greatly improved with the new approach suggested and changed to below:

class C {
    Field foo1 = Field(null); // Passing nullable, enclMember = "foo1"
    Field foo2 = null; // assigning nullable, enclMember = "foo2"
    static Field foo3 = null; // assigning nullable, enclMember = "foo3"
    {
          Field3 = null; // assigning nullable, enclMember = "null"
    }

   void bar(){
         this.foo1 = null; // assigning nullable, enclMember = "bar()"
   }
}

None of the error reported above have any conflicts and can be allocated to separate regions.

[msridhar]

Note: This PR can wait until the next release of NullAway (0.10.2).

Do you mean until after that release?

[lazaroclapp]

Note: This PR can wait until the next release of NullAway (0.10.2).

Do you mean until after that release?

For context, we want to have a NullAway release with #656 (probably NullAway 0.10.2) and a NullAwayAutoAnnotator release with some version of ucr-riple/NullAwayAnnotator#72, then test them internally and upgrade them in lockstep.

@nimakarimipour , question, does having this change in NullAway require a corresponding change in NullAwayAutoAnnotator?

I think, long term, if need to keep upgrading the two in lockstep, we might either need them to be on the same repo with tight coupling and integration tests between the two components, or we need to make all these file formats versioned and keep compatibility within the NullAwayAutoAnnotator for multiple NullAway releases, it's becoming a bit of an issue that we need to coordinate releases between the two projects.

[nimakarimipour]

Note: This PR can wait until the next release of NullAway (0.10.2).

Do you mean until after that release?

Yes, the reason is that it requires a change in Annotator and I think we should make pipeline internally stable to work with NullAway 0.10.2 and Annotator 1.3.3 for now, and then apply both changes in NullAway 0.10.3 and Annotator 1.3.4.

[nimakarimipour]

Note: This PR can wait until the next release of NullAway (0.10.2).

Do you mean until after that release?

For context, we want to have a NullAway release with #656 (probably NullAway 0.10.2) and a NullAwayAutoAnnotator release with some version of nimakarimipour/NullAwayAnnotator#72, then test them internally and upgrade them in lockstep.

@nimakarimipour , question, does having this change in NullAway require a corresponding change in NullAwayAutoAnnotator?

I think, long term, if need to keep upgrading the two in lockstep, we might either need them to be on the same repo with tight coupling and integration tests between the two components, or we need to make all these file formats versioned and keep compatibility within the NullAwayAutoAnnotator for multiple NullAway releases, it's becoming a bit of an issue that we need to coordinate releases between the two projects.

Yes it requires a change in Annotator, actually I thought about making NullAway output its current version in a file and Annotator either try to be compatible with that or terminate with a message informing what NullAway/Annotator version should be used.

[lazaroclapp]

LGTM, except for a minor nit about comment styles 🙂

[lazaroclapp]

This comment format (starting with /**) indicates that this is the javadoc documentation for method. I think this is just an unrelated comment about both of the fields below, so I would just have it as:

// Finding values for these properties is costly and are not needed by default, hence, they are
// not final and are only initialized at request.

[nimakarimipour]

Thank you for noticing this :) 9e6f062

[lazaroclapp]

Yes it requires a change in Annotator, actually I thought about making NullAway output its current version in a file and Annotator either try to be compatible with that or terminate with a message informing what NullAway/Annotator version should be used.

I think it's worth doing this as a follow up PR before the next NullAway release, at least the compatibility check. Would save us some surprises in the future...