ucr-riple / NullAwayAnnotator

A tool to help adapting code bases to NullAway type system.
MIT License
13 stars 6 forks source link

Extend impacted regions by a method change #132

Closed nimakarimipour closed 1 year ago

nimakarimipour commented 1 year ago

This pull request extends the set of impacted regions for changes made to methods. Prior to this pull request, only call sites (and overriding methods) were taken into consideration as impacted regions for changes to methods. However, the use of lambda expressions or method references can actually extend the set of impacted regions and these should also be included. See example below:

public class Main {

    public void f(Foo r){ }

    public void baz1(){
        f(new Foo(){
            public void bar(Object o){
                t(o);
            }
        });
    }

    public void baz2(){
        f(e -> t(e));
    }

     public void baz3(){
        f(this::t);
    }

    public void t(Object o){ }
}

interface Foo{
    void bar(Object o);
}

For example, if the o parameter of the bar method in Foo is annotated as @Nullable, the regions baz2 and baz3 were previously not included as impacted regions since there is no trace of bar in these methods in the current implementation. As a result, the triggered errors (such as passing @Nullable for t(Object)) will not be caught by the annotator. This pull request addresses this issue and extends the set of impacted regions to include baz2() and baz3() for changes made to Foo#bar().

This PR also renames the output call_graph.tsv to method_impacted_region_map.tsv to better reflect its new content as it is no longer limited to just call sites, but now also includes the additional regions.

msridhar commented 1 year ago

Good catch. I think the essence of the issue here is that lambdas (and method references) implicitly create an overriding method, which could have new errors when annotations are added to the functional interface method. To account for this conservatively, we make the containing method of the lambda an impacted region.

msridhar commented 1 year ago

@nimakarimipour is this ready for another review?

nimakarimipour commented 1 year ago

@msridhar Yes it is now.