Ensure all packages declare package-info.java with null-safety annotations

[edyda99]

Overview

This commit adds the JavadocPackage Checkstyle module to the project's configuration file (checkstyle.xml). The module is configured to emit a warning message when a package-info.java file is missing, rather than throwing an exception. In addition, suppressions have been added to prevent the JavadocPackage checks from being applied to packages outside of the src/main directory. These suppressions ensure that the module only checks the packages that are relevant to the project and prevents false positives from being reported.

Deliverables

• Configure the JavadocPackage Checkstyle module to require package-info.java files for all packages under src/main.
• Determine if it is possible to require that package-info.java files include null-safety annotations – for example, via Checkstyle.
• If it's possible to require that package-info.java files include null-safety annotations, configure the necessary infrastructure.
• Introduce missing package-info.java files with package-level Javadoc and null-safety annotations.

[simonbasle]

thanks for the suggestion. Can you provide more details about the choices you made for checkstyle modules?

[simonbasle]

(didn't see the issue reference, which probably provides some context. my bad)

[simonbasle]

@edyda99 I think the RegexpJavaSingleLine variant does the job (unless you've spotted occurrences of multilines nullability annotations?) The other one is also harder to read and has its own caveats with comments, so I'd go with the first one.

[edyda99]

Hi @simonbasle, I will go with RegexpJavaSingleLine.
No, I didn't spot any occurrences of multilines nullability, but it was about preventing it from happening.
So should I remove the whitespace warning? ( I would suggest keeping it and increasing the severity to "error", so whenever someone writes @\n<annotation_name> an error will be thrown)

[simonbasle]

( I would suggest keeping it and increasing the severity to "error", so whenever someone writes @\n<annotation_name> an error will be thrown)

Perhaps a simpler one line regexp like @\s*$ (at then any number of whitespace then the end of the line) would fit the bill? That said, I think this PR can focus on the actual null-safety annotations.

[edyda99]

Done!
In case all was good,
I will start with the last point:
'Introduce missing package-info.java files with package-level Javadoc and null-safety annotations.'
But I have two questions:

• Do you suggest doing micro commits for each missing package-info.java?
• What do I need to rely on before adding null-safety annotations on a newly added package-info?

[simonbasle]

• Do you suggest doing micro commits for each missing package-info.java?

Not necessarily, if that's the only thing in the commit.

• What do I need to rely on before adding null-safety annotations on a newly added package-info?

It depends on how many package-info are missing. I would locally prepare the change and run some static analysis trying to find null-safety issues raising from that change. If the build doesn't break, I'm not sure I would make it a goal of this PR to fix the null-safety warnings though. Wdyt @sbrannen (also cc @sdeleuze)

[sbrannen]

• Do you suggest doing micro commits for each missing package-info.java?

Not necessarily, if that's the only thing in the commit.

I agree with @simonbasle.

Ideally the final result will be 3-4 commits.

• enforce that package-info.java files exist and contain Javadoc
  • build should fail
• introduce missing package-info.java files
  • build should pass
• enforce that package-info.java files declare @NonNullApi and @NonNullFields
  • build should fail
• introduce missing @NonNullApi and @NonNullFields declarations
  • build should pass

Of course, we may squash all commits in the PR into a single commit if that turns out to be easier.

• What do I need to rely on before adding null-safety annotations on a newly added package-info?

I don't understand the question. Can you please expound?

It depends on how many package-info are missing. I would locally prepare the change and run some static analysis trying to find null-safety issues raising from that change. If the build doesn't break, I'm not sure I would make it a goal of this PR to fix the null-safety warnings though. Wdyt @sbrannen (also cc @sdeleuze)

Ensuring that proper null-safety semantics are enforced within code in newly annotated packages is beyond the scope of this PR/issue. I've created #30083 to address that.

[sbrannen]

Thanks for working on this!

It's coming along nicely.

I've requested a few minor changes.

After that, all that's left is ensuring that the build fails for violations and that all missing package-info.java files have been created (so that the build passes again).

[sbrannen]

You also need to suppress violations in the framework-docs module, since that source code is only used in the reference manual.

And for the following packages in spring-core.

• spring-core/src/main/java/org/springframework/asm
• spring-core/src/main/java/org/springframework/cglib
• spring-core/src/main/java/org/springframework/objenesis
• spring-core/src/main/java/org/springframework/javapoet
• spring-core/src/main/java/org/springframework/lang

[edyda99]

Hi again,

First sorry for the continuous Force pushes, but I wanted to reduce my commits.
I reduced them to 4 micro commits, with the last one containing the full commit message.
All comments are resolved.

The merge request is ready for the final review

Thank you again!

[sbrannen]

Thanks for making the latest changes, @edyda99!

Except for creation of the missing package-info.java files, the PR is effectively complete.

While reviewing the work locally on my machine, I have discovered a few issues with regular expressions, the wrong severity, additional packages that need to be excluded (suppressed), etc.

I have therefore converted this PR to a draft to signal that I will make the final changes locally after merging in your work.

[edyda99]

@sbrannen Ok Great
Could you suggest something else for me?

[sbrannen]

This has been merged into main in 268e3fe and 7e32f50 and revised and completed in 99e54fe.

The latter turned out to be a bit more work than expected. 👀

Thanks again for the contribution, @edyda99!

[sbrannen]

Could you suggest something else for me?

We have an ideal-for-contribution label for such suggestions. Though, unfortunately there's only one issue in that category right now.

In light of that, you can search for existing issues that appeal to you and for which you feel qualified to tackle. Then ask on the issue if a PR would be welcome.