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

[Nullability Annotations to Java Classes] Enable and Configure UnknownNullness Android Lint Check #19116

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Aug 31, 2023

Parent #18904
Closes #19113

This PR enabled and configures the extra interop related UnknownNullness Android Lint check to help detect and flag some of the nullability related interoperability issues, that is, issues in Java for Kotlin consumption.

For more info see:


To test:

  • There is nothing much to test here, verifying that all the CI checks are successful should be enough
    • Focus more on the Lint related ones.
  • You could also check that the Interoperability:Kotlin Interoperability related section on the Lint report is shown and reporting some few thousand of warnings (WordPress + Jetpack).
    • For example, locally, running Lint on the jetpack + wasabi + debug variant, I got 4265 such warnings, configured to be informational only so that they don't fail the build, see screenshot below:

image


Regression Notes

  1. Potential unintended areas of impact

    • N/A
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • See To test section above.
  3. What automated tests I added (or what prevented me from doing so)

    • N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

With that change the extra interop related 'UnknownNullness' Android
Lint check gets enabled, which helps detect and flag some of the
nullability interoperability issues for this project.

For more info see:
- Docs: https://developer.android.com/kotlin/interop#supported_checks
- Check: https://googlesamples.github.io/android-custom-lint-rules/
checks/UnknownNullness.md.html

------------------------------------------------------------------------

You will also notice that this same 'UnknownNullness' configuration has
been added on all the rest of the Android related 'library' modules.
This is done because otherwise Lint is failing with the below
'CannotEnableHidden' related error:

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Execution failed for task ':WordPress:lintJetpackWasabiDebug'.
> Lint found errors in the project; aborting build.

  Fix the issues identified by lint, or add the issues to the lint
  baseline via `gradlew updateLintBaseline`.
  For more details, see https://developer.android.com/studio/write/lint
  #snapshot

  Lint found 1 errors, 0 warnings (403 errors filtered by baseline
  baseline.xml). First failure:

  /Users/.../WordPress/build.gradle:294: Error: Issue UnknownNullness
  was configured with severity warning in WordPress, but was not enabled
  (or was disabled) in library analytics [CannotEnableHidden]
          enable 'UnknownNullness'
                  ~~~~~~~~~~~~~~~

     Explanation for issues of type "CannotEnableHidden":
     Any issues that are specifically disabled in a library cannot be
     re-enabled in a dependent project. To fix this you need to also
     enable the issue in the library project.

     (This also applies for issues that are off by default; they cannot
     just be enabled in a dependent project; they must also be enabled
     in all the libraries the project depends on.)

  The full lint text report is located at:
    /Users/.../WordPress/build/intermediates/
    lint_intermediate_text_report/jetpackWasabiDebug/
    lint-results-jetpackWasabiDebug.txt
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@ParaskP7 ParaskP7 added this to the Future milestone Aug 31, 2023
@ParaskP7 ParaskP7 requested a review from mkevins August 31, 2023 11:15
@ParaskP7 ParaskP7 self-assigned this Aug 31, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 31, 2023

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr19116-001b076
Commit001b076
Direct Downloadjetpack-prototype-build-pr19116-001b076.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 31, 2023

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr19116-001b076
Commit001b076
Direct Downloadwordpress-prototype-build-pr19116-001b076.apk
Note: Google Login is not supported on these builds.

The default severity for 'UnknownNullness' is 'Warning'. This change
updates that to 'Informational'. This is done for two main reasons:
1. To treat this kind of warnings as informational only, a light typo
like of a warning in AS.
2. To avoid failing the build since all Lint warnings are treated as
errors for this project (see 'warningsAsErrors = true' configuration).
@ParaskP7 ParaskP7 force-pushed the analysis/enable-unknown-nullness-lint-check branch from 65acac2 to 001b076 Compare August 31, 2023 12:30
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working for me when I run ./gradlew lint, but it isn't showing up in the IDE, for whatever reason 🤷‍♂️ . For that, I had to enable the rule in the settings, which introduced this diff:

diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml
index d727ff611a..dd9b6310c3 100644
--- a/.idea/inspectionProfiles/Project_Default.xml
+++ b/.idea/inspectionProfiles/Project_Default.xml
@@ -1,6 +1,7 @@
 <component name="InspectionProjectProfileManager">
   <profile version="1.0">
     <option name="myName" value="Project Default" />
+    <inspection_tool class="AndroidLintUnknownNullness" enabled="true" level="WARNING" enabled_by_default="true" />
     <inspection_tool class="ClassName" enabled="true" level="WEAK WARNING" enabled_by_default="true">
       <scope name="Tests" level="INFORMATION" enabled="true" />
     </inspection_tool>

I was a bit surprised that this wasn't showing up as informational automatically in Android Studio, so feel free to include this as well, or ignore.
Do we also want to do this, or is something else causing th

@@ -291,6 +291,7 @@ android {
checkGeneratedSources = true
lintConfig file("${project.rootDir}/config/lint/lint.xml")
baseline file("${project.rootDir}/config/lint/baseline.xml")
enable += 'UnknownNullness'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for using the += enabling here 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All yours, thanks for suggesting it Matt! ❤️

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Sep 1, 2023

👋 @mkevins and thanks for the review! 🙇 ❤️ 🚀

This is working for me when I run ./gradlew lint, but it isn't showing up in the IDE, for whatever reason 🤷‍♂️ .

Yeap, I noticed that too, but I chose to ignore it since we will anyway have our own custom Android Lint rule enabled soon, which will anyway do that for us.

IMHO, having that on the Lint report and it reporting the total number is enough for now, at least for the work currently in-progress from my side.

For that, I had to enable the rule in the settings, which introduced this diff:

Nicely done Matt and good find. 🌟

To be honest, personally, I am against updating any .idea files, or adding them to VCS in the first place (see one of my converstations on that in Slack: C02QANACA/p1607524467466400). Thus, I will never chose that path, unless of course, it is something totally necessary. Thus, me choosing to ignore this here.

I was a bit surprised that this wasn't showing up as informational automatically in Android Studio...

Yes, same here, but then again, I understanding that this isn't Android's official-official check, just an extra custom rule on their side. Thus, maybe they just don't want that to be as embedded with the AS warnings... 🤷

...so feel free to include this as well, or ignore.

As per my above reasonings, I would personally like to ignore that.

PS: One more reason to ignore that at this point of time, and until someone asks for it, is because this might conflict with our own custom Android Lint rule and checks, that is, when we end-up enabling those.

Do we also want to do this, or is something else causing th

😅 ❓ 😊

@mkevins
Copy link
Contributor

mkevins commented Sep 3, 2023

I'm not sure why my comment got cut off 😅 , but I think it said "the missing highlights?" at the end. Anyway, I agree, especially since we'll have our own rule soon, no need to modify the .idea/ files. 👍

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Sep 4, 2023

I'm not sure why my comment got cut off 😅 , but I think it said "the missing highlights?" at the end. Anyway, I agree, especially since we'll have our own rule soon, no need to modify the .idea/ files. 👍

Awesome @mkevins , once more thanks for the excellent review and for always sharing great suggestions! 🥇

@ParaskP7 ParaskP7 merged commit 44a6d06 into trunk Sep 4, 2023
@ParaskP7 ParaskP7 deleted the analysis/enable-unknown-nullness-lint-check branch September 4, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nullability Annotations to Java Classes - Enable and Configure UnknownNullness Android Lint Check
3 participants