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

Prevent build failures on downstream dependencies due to changes in upstream #62

Closed
nimakarimipour opened this issue Sep 4, 2022 · 2 comments · Fixed by #63
Closed
Assignees
Labels
enhancement New feature or request

Comments

@nimakarimipour
Copy link
Member

nimakarimipour commented Sep 4, 2022

Is your feature request related to a problem? Please describe.

If annotator processes the target module with --downstream-dependency-analysis-activated flag, it will collects effects of making public methods @Nullable on downstream dependencies and considers the overall effect while making inference decisions. Some annotations can reduce the number of errors locally far more than the number of triggered errors on downstream dependencies and can have a positive overall impact, hence, these annotations will get injected to the target module (even though this will lead to build errors on downstream dependencies).

It is a common use case where the user would like to run the AutoAnnotator on target module with following constraints:

  • No modification on downstream dependencies is allowed.
  • No build failures on downstream dependencies after enrolling the target module.

Describe the solution you'd like

A configuration mode which if enabled, it guarantees that no changes in upstream will trigger an error on downstream dependencies. This should also cover detection of flow of @Nullable back to upstream. See example below:

Target Module:

class Foo{

     public Object returnNullBad(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     public Object returnNullGood(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     public void takeNullGood(Object param){ // Called only in downstream dependencies.
              if(param != null){
                     param.deref();
              }
     }

     public void takeNullBad(Object){ // Called only in downstream dependencies.
             param.deref();
     }
}

Downstream Dependency Module:

class Dep{

     Object field = new Object();
     Foo foo = new Foo();
     
     public void bar(){
             field = foo.returnNullBad();
             foo.takeNullGood(foo.returnNullBad());
             foo.takeNullGood(foo.returnNullGood());
             foo.takeNullBad(foo.returnNullGood());
     }
}

If Annotator only focus on the overall impact, it will annotate the Target module as below:

class Foo{

     @Nullable   // This will resolve 5 errors locally and triggers 1 unresolvable and 1 resolvable error (on Dep) overall: -4
     public Object returnNullBad(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     @Nullable // This will resolve 5 errors locally and 1 resolvable error (change on Target) overall: -5
     public Object returnNullGood(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     // This will resolve 1 error on downstream dependency and create no error locally
     public void takeNullGood(@Nullable Object param){ // Called only in downstream dependencies.
              if(param != null){
                     param.deref();
              }
     }

     // Since @Nullable injection will create an error locally and the overall is +1,
     // it will not get injected, however, this will leave the error on downstream unresolved.
     public void takeNullBad(Object){ // Called only in downstream dependencies.
             param.deref();
     }
}

With the requested configuration mode, annotator should annotate Target module as below:

class Foo{

     @NullUnmarked //To resolve the error on downstream dependency.
     public Object returnNullBad(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     @Nullable
     public Object returnNullGood(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     public void takeNullGood(@Nullable Object param){ // Called only in downstream dependencies.
              if(param != null){
                     param.deref();
              }
     }

     @NullUnmarked //To resolve the error on downstream dependency.
     public void takeNullBad(Object){ // Called only in downstream dependencies.
             param.deref();
     }
}

Describe alternatives you've considered
None.

Additional context
Not-Applicable.

@nimakarimipour nimakarimipour added the enhancement New feature or request label Sep 4, 2022
@nimakarimipour nimakarimipour self-assigned this Sep 4, 2022
@nimakarimipour
Copy link
Member Author

nimakarimipour commented Sep 4, 2022

Update


This will feature will be supported with guarantees that no errors will trigger on downstream dependencies without injection of @NullUnmarked annotations on target, that will be implemented in future PRs.

For now the output will be below:

class Foo{

     public Object returnNullBad(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     @Nullable
     public Object returnNullGood(){
              // Assume there are 5 branches ending with return null;
              return null;
     }

     public void takeNullGood(@Nullable Object param){ // Called only in downstream dependencies.
              if(param != null){
                     param.deref();
              }
     }

     //To resolve the error on downstream dependency.
     public void takeNullBad(@Nullable Object){ // Called only in downstream dependencies.
             param.deref();
     }
}

@nimakarimipour
Copy link
Member Author

Update

The alternative approach here is that Annotator treat all impacted parameters as triggered fixes when applying a fix tree, hence, either all impacted parameters will be @Nullable or none will get annotated. This explicitly in favor of Strict mode.

nimakarimipour added a commit that referenced this issue Sep 9, 2022
…is. (#63)

This Resolves #62 by adding a configuration mode `Strict`.

With this PR, we can now configure the analysis mode which will impact inference decisions.

We have four different modes of analysis:
1. `Local`: Only considers the local effect of fixes.
2. `Lower Bound`: Considers the lower bounds of number of errors on downstream dependencies.
3. `Upper Bound`: Considers the upper bound of number of errors on downstream dependencies.
4.  `Strict`: Guarantees that all errors in downstream due to changes in upstream will be resolved.

To set the above modes via command line, pass `-am` or `--analysis-mode` followed by one of the options below:
- `default`
- `upper_bound`
- `lower_bound`
- `strict`

Or set one of the values above In the `.json` config file at:
```json
"DOWNSTREAM_DEPENDENCY_ANALYSIS":{
     "ANALYSIS_MODE": "mode"
}
```
nimakarimipour added a commit that referenced this issue Sep 11, 2022
… of the iteration (#68)

#### ~~This PR is build upon GH-62.~~ GH-62 is landed.

This PR resolves #56 by introducing `ReportCache` class and breaking down `explore` method in annotator. With this PR, at the analysis it runs reruns the `inference/injection` phase without exclusion of already processed `fixes`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant