Skip to content

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

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

Closed
4 tasks done
sbrannen opened this issue Mar 1, 2023 · 6 comments
Closed
4 tasks done
Assignees
Labels
status: superseded An issue that has been superseded by another type: task A general task

Comments

@sbrannen
Copy link
Member

sbrannen commented Mar 1, 2023

Overview

Since Spring Framework 5.0 (see #20099), we annotate packages in package-info.java files with null-safety annotations such as @NonNullApi; however, not all packages contain package-info.java files which prevents us from enforcing non-null semantics by default for those packages.

Scope

This applies to all packages within src/main. Conversely, this does not apply to packages in src/test.

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.
@sbrannen sbrannen self-assigned this Mar 1, 2023
@sbrannen sbrannen added the type: task A general task label Mar 1, 2023
@sbrannen sbrannen added this to the 6.0.7 milestone Mar 1, 2023
@edyda99
Copy link
Contributor

edyda99 commented Mar 5, 2023

Hi @sbrannen , this task seems to be straightforward, I would appreciate it if you allow me to do it. Thank you

@sbrannen sbrannen changed the title Configure JavadocPackage Checkstyle module to require package-info.java files Ensure all packages declare package-info.java with null-safety annotations Mar 5, 2023
@sbrannen
Copy link
Member Author

sbrannen commented Mar 5, 2023

Hi @edyda99,

this task seems to be straightforward

Indeed it does seem rather straightforward; however, it may end up being more work than you'd expect.

I have updated the title and description of this issue to reflect the scope and expected deliverables.

I would appreciate it if you allow me to do it.

Please review the updated scope and deliverables.

If you're still up to the task, please let me know and I'll assign it to you.

Ideally, we'd like to address this before 6.0.7. So please keep that in mind.

@edyda99
Copy link
Contributor

edyda99 commented Mar 5, 2023

Hi, first thank you a lot for giving me the opportunity.
Second I spent these two hours doing some readings to know more about this subject.

  • As it seems to me that the initial point requires
    <module name="JavadocPackage"> <property name="severity" value="warning"/> </module>

  • Regarding 'applies to all packages within src/main'
    There is no direct property to add to the module, there is a property 'baseDir' but it would be applied to all the modules. So I need to use filters to suppress warnings to directories other than src/main

  • Regarding nullSafety I found a documentation about RegexpSinglelineJava, which may help me.

  • And finally, regarding the last point, It needs mainly time more than skills.

Please correct me if I said anything wrong.

I will dedicate this week to this task if you allow me and will push on a daily basis so you can track my progress.

Again thank you!

@edyda99
Copy link
Contributor

edyda99 commented Mar 6, 2023

This is my pull request: #30069

@edyda99
Copy link
Contributor

edyda99 commented Mar 6, 2023

@sbrannen My mr is ready for your comments.
I haven't done yet the last step, as I am stuck in choosing which is the best way to check null-safety annotations
My first solution was RegexpSinglelineJavaCheck but it has one limitation by checking null-safe annotations made up of multiple lines for example
@\n NonNull
The second option is: RegexpMultiline which has one limitation that it won't be able to check if the annotation is commented or not

These two options are available in my commit, I am waiting for your advice.
(One remaining option is to use a complex regex with RegexpMultiline that satisfies the following: if a commented block is declared it should be before any annotation)

Thank you a lot

@sbrannen
Copy link
Member Author

sbrannen commented Mar 8, 2023

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2023
@sbrannen sbrannen removed this from the 6.0.7 milestone Mar 8, 2023
@sbrannen sbrannen added the status: superseded An issue that has been superseded by another label Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: task A general task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants