SeleniumHQ
[java] NullAway checks added
User description
Description
In this PR I'm adding NullAway nullness analyzer to the project. By default NullAway plugin is disabled, to enable them set //java:nullaway_level flag:
--//java:nullaway_level=WARN--//java:nullaway_level=ERROR
NullAway Configuration details:
-Xep:NullAway:WARN- report problems as warnings-Xep:NullAway:ERROR- report problems as errors - stop compilation when an error occurs-XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium- analyze this package (including subpackages)-XepOpt:NullAway:JSpecifyMode=true- enable support for annotations on generic types
NOTE: NullAway has a special behavior, all classes located under AnnotatedPackages are treated as annotated with @NullMarked - source. Anyway, We need to mark these classes with @NullMarked annotation manually to remain compliant with JSpecify specification.
As suggested, I also added null checking as part of the GitHub workflow.
Motivation and Context
The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions. Related issue: https://github.com/SeleniumHQ/selenium/issues/14291 To verify potential problems with the Selenium source code, we can use nullness analyzers that report code issues based on JSpecify annotations. One of plugins that is easy to configure with Bazel is NullAway, potential alternatives described here. Context: https://github.com/SeleniumHQ/selenium/pull/14372#issuecomment-2293116104
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist
- [x] I have read the contributing document.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
PR Type
enhancement, configuration changes
Description
- Added NullAway as a dependency to the project to enhance nullness analysis.
- Introduced a new
java_pluginfor NullAway in the Bazel build configuration. - Configured NullAway for
selenium-apiandselenium-supportmodules by adding it tojava_exportandjava_librarytargets. - Updated Maven install JSON to include NullAway and its dependencies.
Changes walkthrough 📝
| Relevant files | |||||||
|---|---|---|---|---|---|---|---|
| Dependencies |
| ||||||
| Configuration changes |
|
💡 PR-Agent usage: Comment
/helpon the PR to get a list of all available PR-Agent tools and their descriptions
PR Reviewer Guide 🔍
(Review updated until commit https://github.com/SeleniumHQ/selenium/commit/a1bc007a32c8988e7bb76f21dff0ed0232757bdd)
| ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
| ⚡ Key issues to review Configuration Complexity Build Configuration |
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
PR Code Suggestions ✨
| Category | Suggestion | Score |
| Enhancement |
Add an option to treat generated code as unannotated in NullAway configurationConsider adding the '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' option to java/src/org/openqa/selenium/BUILD.bazel [44-48]
Suggestion importance[1-10]: 7Why: This suggestion is valid as it enhances the NullAway configuration by reducing false positives in generated code. It is a minor improvement but can be beneficial for code quality. | 7 |
Add an option to treat generated code as unannotated in NullAway configuration for both targetsConsider adding the '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' option to java/src/org/openqa/selenium/support/BUILD.bazel [42-46]
Suggestion importance[1-10]: 7Why: The suggestion is appropriate for improving the NullAway configuration by addressing potential false positives in generated code. It is a minor enhancement but useful for maintaining code quality. | 7 | |
| Best practice |
✅
| 6 |
Why not -Xep:NullAway:ERROR, otherwise the huge output of the CI might swallow the warnings.
What do others think about this?
@joerg1985 At the moment we have ~80 warnings. I think setting the ERROR level will cause the compilation to fail
I have such steps in my mind:
- For each Selenium artifact: (
selenium-api,selenium-support,selenium-remote-driver, ...)- Add NullAway with WARNING level
- Annotate all classes in the module with the proper JSpecify annotations (the number of warnings should decrease)
- Fix bugs in the code (the number of warnings should ideally be 0)
- Change NullAway level to ERROR
What do you think?
What is the ideal way to proceed? Should we have the build fail if this check fails, or should the CI pipeline fail?
I tend to prefer the latter. What do you think?
Agree,
So, I'll modify PR:
- I'll set NullAway reporting level to
ERROR - I'll make Bazel optionally run NullAway analysis via flag/tag/argument (I need to check that)
- I'll create an extra GitHub workflow job in the
ci-java.ymlfile with NullAway analysis
Does it sound good?
Persistent review updated to latest commit https://github.com/SeleniumHQ/selenium/commit/a1bc007a32c8988e7bb76f21dff0ed0232757bdd
Hi @joerg1985, @diemol
I updated the Pull Request.
Now nullness checks are part of the workflow (NullAway set to log issues with the error level) - I added nullaway_level flag to not destroy regular builds during development.
Hello,
I just pushed two commits, with changes:
- the `trunk' branch was merged
- NullAway updated to the
0.11.3 - the code was reformatted
I'm still not sure if the changes related to NullAway in the GitHub pipeline that I proposed fit the Selenium workflow. If I get any suggestions for changes then I will try to improve the code
Is there any chance to merge this? Or should I apply some changes / split the PR into several smaller ones?
To be honest, these SpotBugs errors are confusing to me because there is no clear error message
From what I see NullAway and Spotbugs, don't have common transitive dependencies which updates could corrupt spotbugs checks.
Could you re-run GitHub Workflow to see if the problem is repeatable?
Thank you for pointing this out, I found the reason
Until my change, SpotBugs analysis has been run only for classes enclosed in the java_library.
For example: /java/src/org/openqa/selenium/cli/BUILD.bazel - SpotBugs analysis is run for *.java files.
Files that were not included in java_library but in java_export were not subject to SpotBugs analysis.
That's because java_export calls the native.java_library, which by default has not linting tests.
For example /java/src/org/openqa/selenium/edge/BUILD.bazel - no SpotBugs analysis for *.java files.
My changes have updated the implementation so that SpotBugs analysis is also run for files enclosed in the java_export.
This exposed actual flaws in the code that we were not aware of.
New bugs found by SpotBugs:
//java/src/org/openqa/selenium/chromium:chromium-lib-spotbugsH C RV: Bad attempt to compute absolute value of signed 32-bit hashcode in org.openqa.selenium.chromium.ChromiumDriver.pin(String) At ChromiumDriver.java:[line 215]- also linted in my IDE by Sonar
//java/src/org/openqa/selenium/devtools/v85:v85-lib-spotbugsD UC: Useless condition: it's known that code <= 399 (0x18f) at this point At V85Network.java:[line 140]- also linted in my IDE, conditionif (code < 300 && code > 399)is never true
//java/src/org/openqa/selenium/firefox:firefox-lib-spotbugsM D NP: Possible null pointer dereference in org.openqa.selenium.firefox.AddHasExtensions$1.zipDirectory(Path) due to return value of called method Dereferenced at AddHasExtensions.java:[line 104]- probably a false flag
//java/src/org/openqa/selenium:core-lib-spotbugsM D REC: Exception is caught when Exception is not thrown in org.openqa.selenium.net.PortProber.isFree(String, int) At PortProber.java:[line 122]- rather cosmetic change (replacecatch (Exception e)withcatch (IOException | RuntimeException e)
I think this analysis is desired.
Maybe I should activate SpotBugs for java_export and also address these 4 errors in a dedicated PR?
What do you think?
I merged trunk into my branch, SpotBugs related issues should be fixed now.
Can we re-run checks?
Apologies for the delay in responding, @mk868. I will run the CI again and merge it if everything is working well.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 58.61%. Comparing base (
a409c81) to head (4558a24). Report is 22 commits behind head on trunk.
Additional details and impacted files
@@ Coverage Diff @@
## trunk #14421 +/- ##
==========================================
+ Coverage 58.59% 58.61% +0.01%
==========================================
Files 94 94
Lines 5995 5997 +2
Branches 259 259
==========================================
+ Hits 3513 3515 +2
Misses 2223 2223
Partials 259 259
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@diemol Currently we have ~80 NullAway errors only for the java/src/org/openqa/selenium/BUILD.bazel:core library, for the whole project it's ~1250 errors.
I counted these problems using bazel build //java/... --//java:nullaway_level=WARN 2>&1 | grep "\[NullAway\]" -c
I can fix these errors progressively in separate PRs (as I have done so far, 2-3 changed files per PR), and then we can merge the trunk to this PR from time to time to see the progress.
Will it be okay?