[Suggestion] NonNull generator for parameters

[4everTheOne]

One important factor to keep the quality of the software is to make sure is handles unintended behavior. @MysterAitch suggested at #2707 to add @Nullable and @NotNull to indicate if a given variable can take null values.

In this PR is suggested a way to improve the quality by providing a generator that creates the assertions required for the annotated variables. Lets take a look at an example.

class Foo {
   public void doOperation(String text, String optionalInformation) {
      ...
   }
}

To make sure the example above can only take non-null values we simply add a @NotNull annotation to the parameter:

class Foo {
   public void doOperation(@NotNull String text, @Nullable String optionalInformation) {
      ...
   }
}

By doing this, when the Generator is run, the preconditions will be added automatically making it looks like this:

class Foo {
   public void doOperation(@NotNull String text, @Nullable String optionalInformation) {
      Preconditions.checkNotNull(text, "Parameter text can't be null.");
      ...
   }
}

How the generator works

At the moment the generator only support method and constructor parameters but can be expanded for other types like variable declarations or even field declaration.

The generator get all methods and constructors in the class, and check if the parameter is annotated with @NotNull if it's, then it will generate a call at the top of the method to Preconditions.checkNotNull to make sure no null value is present. The logic for the constructor follows the same guidelines as a method, but with a tiny difference. If the the first statement in the constructor is a ExplicitConstructorInvocationStmt then it adds the assertion after to avoid compilation issues.

I would like to hear your opinions! :)

[MysterAitch]

Just a quick comment -- how does this behave when running the generators multiple times? e.g., are existing/redundant null checks removed?

[codecov]

Codecov Report

Merging #3127 (b7737a0) into master (7ff0d10) will decrease coverage by 1.033%.
The diff coverage is 70.992%.

@@               Coverage Diff               @@
##              master     #3127       +/-   ##
===============================================
- Coverage     58.332%   57.299%   -1.034%     
  Complexity      2670      2670               
===============================================
  Files            634       635        +1     
  Lines          34418     33557      -861     
  Branches        5847      5785       -62     
===============================================
- Hits           20077     19228      -849     
+ Misses         12284     12276        -8     
+ Partials        2057      2053        -4     

Flag	Coverage Δ
AlsoSlowTests	57.299% <70.992%> (-1.034%)	⬇️
javaparser-core	52.446% <63.263%> (-1.405%)	⬇️
javaparser-symbol-solver	36.752% <18.511%> (+0.695%)	⬆️
jdk-10	57.295% <70.992%> (-1.031%)	⬇️
jdk-11	57.295% <70.992%> (-1.028%)	⬇️
jdk-12	57.295% <70.992%> (-1.031%)	⬇️
jdk-13	57.295% <70.992%> (-1.031%)	⬇️
jdk-14	57.295% <70.992%> (-1.031%)	⬇️
jdk-15	57.295% <70.992%> (-1.031%)	⬇️
jdk-16	57.295% <70.992%> (-1.031%)	⬇️
jdk-8	57.296% <70.964%> (-1.034%)	⬇️
jdk-9	57.295% <70.992%> (-1.031%)	⬇️
macos-latest	57.287% <70.896%> (-1.034%)	⬇️
ubuntu-latest	57.278% <70.992%> (-1.040%)	⬇️
windows-latest	57.278% <70.896%> (-1.034%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...src/main/java/com/github/javaparser/JavaToken.java	89.749% <ø> (ø)
...a/com/github/javaparser/ParseProblemException.java	90.909% <ø> (ø)
...c/main/java/com/github/javaparser/ParseResult.java	95.000% <ø> (ø)
...rc/main/java/com/github/javaparser/ParseStart.java	100.000% <ø> (ø)
...src/main/java/com/github/javaparser/Processor.java	66.666% <ø> (ø)
...src/main/java/com/github/javaparser/Providers.java	75.000% <ø> (ø)
...c/main/java/com/github/javaparser/ast/DataKey.java	33.333% <ø> (ø)
.../github/javaparser/ast/expr/ArrayCreationExpr.java	47.619% <0.000%> (+1.107%)	⬆️
...ub/javaparser/ast/nodeTypes/NodeWithArguments.java	50.000% <ø> (ø)
...ub/javaparser/ast/nodeTypes/NodeWithBlockStmt.java	100.000% <ø> (ø)
... and 218 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ff0d10...b7737a0. Read the comment docs.

[4everTheOne]

Just a quick comment -- how does this behave when running the generators multiple times? e.g., are existing/redundant null checks removed?

At the moment, it doesn't repeat if matches exactly, for example if the assertion is already present. But there's is room for improving. At the moment if we change the error message it will add a new one instead of replacing it.

[4everTheOne]

All polished and ready for review! :)

[jlerbsc]

comment.getRange().isPresent() can be replaced by comment.hasRange()

[4everTheOne]

Thanks for the feedback jlerbsc! I think that could be a improvement for the visitor generators, to use the corresponding "has" method instead of using isPresent() 👍

[MysterAitch]

comment.getRange().isPresent() can be replaced by comment.hasRange()

Those ones are generated iirc

[jlerbsc]

Hi @4everTheOne You have a lot of interesting PR waiting with conflicts. Do you have time to resolve these conflicts especially this one so I can merge it? Thanks.

[4everTheOne]

Hi @jlerbsc!
Thanks for the feedback! I'll try to sort it out.
I'll ping you when its done ;)

[jlerbsc]

I've spent some time looking again at your previous PRs which are generally very good in intent. Often the problem is that the impact is far too great for me to validate your work. It would certainly be necessary to proceed in small steps, trying to impact public apis as little as possible. Thank you for your investment.

[4everTheOne]

@jlerbsc, this PR has been updated :)

[jlerbsc]

Do you think that this evolution can have an impact on the functioning of applications that use JavaParser? I am asking this question to name the next version of JavaParser.

[4everTheOne]

I don't see any impact on it. The most sensitive class here is the StaticJavaParser, which is the class that contains the annotation NonNull. But annotation were only added when it makes sense for the parameter to not be null.

But let me know, if you see any impact at StaticJavaParser :)

[jlerbsc]

I was thinking a difference at runtime.

[jlerbsc]

Thank you for your contribution.