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

Add missing null annotation detector #11

Merged
merged 20 commits into from
Aug 23, 2023

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Aug 8, 2023

Description

This PR adds rules to inform about missing null annotations in Java code.

To test:

Unit tests

Run the unit tests in the project.

Testing with WordPress-Android

Following the same steps from this PR: #10, the out-dated rules should be removed, and no errors should be present.

Since the severity is informational, after running this, look around within Android Studio. Missing null annotations should be underlined in gray, and should not be present on fields / parameters with the @Inject annotation.

mkevins added 2 commits August 8, 2023 12:39
This adds a lint rule to inform when we are missing null annotations in
Java code. It currently only informs for fields, and requires Android
null annotations (`@NonNull`, and `@Nullable`).
This implements missing null annotation detection for method parameters
and method return types. Injected parameters are ignored for this rule.
@mkevins mkevins added the enhancement New feature or request label Aug 8, 2023
@mkevins mkevins requested a review from ParaskP7 August 8, 2023 07:54
@ParaskP7 ParaskP7 self-assigned this Aug 8, 2023
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @mkevins !

I have reviewed and tested this PR as per the instructions, everything (almost) works as expected, great job! 🌟 🌟 🌟


I have left a question (❓), a couple of suggestions (💡) and some minor (🔍) comments for you to consider. I am NOT going to approve this PR as I think it is better for us to at least discuss those before merging. I think it will be beneficial for us get this custom Android Lint rule to the best state possible before starting utilizing it.


EXTRAS (hence the almost above)

  1. Warning (⚠️): There are 3 Lint failures when having this custom Android Lint rule on. Those are related to the ReaderCommentListActivity.java, ReaderPostActions.java and ReaderVideoViewerActivity.java classes. I am not sure what's the actual issue with those classes, but we might need to figure it out before proceeding with adding and enabling this custom Android Lint rule. Otherwise we might not be able to utilize it. Also, suppressing this custom Android Lint rule on those classes doesn't work, those indeed need to be disabled. In more details, there are unexpected failures during lint analysis on those classes and it is reported that this is a bug in lint or one of the libraries it depends on. The message reports that com.intellij.openapi.editor.Document.getImmutableCharSequence() cannot be invoked because document is null (🤷). This Android Lint crash definitely involves the org.wordpress.android.lint.MissingNullAnnotationDetector detector.
  2. Warning (⚠️): To avoid having each client adding extra Android Lint configuration to ignore the **/generated/** build related path, let's bake that into this custom Android Lint rule itself so that any generated files are ignored.

@mkevins
Copy link
Contributor Author

mkevins commented Aug 9, 2023

  • Warning (⚠️): There are 3 Lint failures when having this custom Android Lint rule on. Those are related to the ReaderCommentListActivity.java, ReaderPostActions.java and ReaderVideoViewerActivity.java classes. I am not sure what's the actual issue with those classes, but we might need to figure it out before proceeding with adding and enabling this custom Android Lint rule. Otherwise we might not be able to utilize it. Also, suppressing this custom Android Lint rule on those classes doesn't work, those indeed need to be disabled. In more details, there are unexpected failures during lint analysis on those classes and it is reported that this is a bug in lint or one of the libraries it depends on. The message reports that com.intellij.openapi.editor.Document.getImmutableCharSequence() cannot be invoked because document is null (🤷). This Android Lint crash definitely involves the org.wordpress.android.lint.MissingNullAnnotationDetector detector.

I was unable to reproduce this in my current local state, but I'll attempt again after the other changes are committed. I also tried added each of the files you mentioned as tests (locally, and not committed), and the tests did not crash. Instead, the output was what we'd expect: all the violations present in those files were reported. Maybe you can try again after the other changes are pushed, and see if you can still reproduce it?

  • Warning (⚠️): To avoid having each client adding extra Android Lint configuration to ignore the **/generated/** build related path, let's bake that into this custom Android Lint rule itself so that any generated files are ignored.

Where do you envision this configuration being added? I don't think it should be within the scanner itself, since that would mean we'd still be traversing into those directories and possibly even parsing / building the UAST before ignoring rule at time of visiting. I imagine you have something else in mind, though, and if so, would you mind elaborating with a specific suggestion?

@mkevins mkevins requested a review from ParaskP7 August 9, 2023 06:38
@mkevins
Copy link
Contributor Author

mkevins commented Aug 9, 2023

Thanks for your feedback Petros! I've implemented most of your suggestions, and noted any exceptions. Also, I've answered most of the questions, but there are still a few unresolved (the error you encountered being the most significant). This is ready for another pass, and perhaps more debugging if you encounter the error again. It would be good to see what is different about our environments as well which would make it reproducible for you but not me. 🤔

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 9, 2023

👋 @mkevins !

I was unable to reproduce this in my current local state, but I'll attempt again after the other changes are committed. I also tried added each of the files you mentioned as tests (locally, and not committed), and the tests did not crash. Instead, the output was what we'd expect: all the violations present in those files were reported. Maybe you can try again after the other changes are pushed, and see if you can still reproduce it?

I just tried again on my side and was able to reproduce it once more. 😢

FYI: I am using this ./gradlew lintJetpackWasabiDebug command to run Lint and the output I get can be found within the WordPress/build/reports/ build folder, by opening this lint-results-jetpackWasabiDebug.html html file.

I am attaching a screenshot below for your convenience. See the 3 LintError on top, point to a Lint Failure.

image

One of those 3 LintError is ReaderCommentListActivity.java, see another screenshot of that for your convenience:

image

Let me know if you get the same by following these exact steps as I am not sure if your are using AS instead of CLI, or which variant you are using on your side (I usually try things first on jetpack + wasabi + debug). I hope this helps with your investigation. 🙏

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 9, 2023

👋 @mkevins !

Where do you envision this configuration being added? I don't think it should be within the scanner itself, since that would mean we'd still be traversing into those directories and possibly even parsing / building the UAST before ignoring rule at time of visiting. I imagine you have something else in mind, though, and if so, would you mind elaborating with a specific suggestion?

I actually do believe (if possible) that this be included within the scanner itself. Otherwise, every client of this library would need to be adding this extra lint.xml configuration below to get it working property:

    <issue id="MissingNullAnnotationOnField">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnConstructorParameter">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnMethodParameter">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnMethodReturnType">
        <ignore path="**/generated/**" />
    </issue>

What I am envisioning is that a custom Android Lint rule having a way to ignore by default anything build folder related as this **/generated/** path is locally within it. Wdyt? 🤔

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 9, 2023

I have an update on this @mkevins !

I just tried again on my side and was able to reproduce it once more. 😢

As I was trying to reply to you here, and updating the lint.xml configuration on my side, by removing the severity="informational" I unnecessary included, and adding the extra configuration I missed for the new MissingNullAnnotationOnConstructorParameter issue, I noticed that running ./gradlew lintJetpackWasabiDebug worked this time, see updated screenshot below! 🎉

image

As such, I think our first problem is gone, it is just that one needs to be careful about how to configure the lint.xml and add the extra configuration for these new custom Android Lint issues of ours. 👍

Thus, I think if we take this away and manage to exclude the build or generated folder as part of the rule scan, then this will become a win-win for how this custom Android Lint rule is to be used and configured going forward. Wdyt? 🤔

Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

Many thanks for applying my suggestions @mkevins and adding all the enhancements, approved, you rock! 🙇 ❤️ 🚀

I reviewed and tested this update, then resolved almost all comment. I just left 2 of them open, just for us to discuss on them. We can merge this PR anyway:

  1. org.jetbrains vs androidx related -> comment
  2. lint.xml and **/generated/** related -> comment + comment

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 9, 2023

Btw @mkevins , now that the WordPressIssueRegistry only points to those 4 new issues, all related to the new MissingNullAnnotationDetector custom Android Lint rule, how about deleting the 2 outdated WordPressAndroidImportInViewModelDetector and WordPressRtlCodeDetector custom Android Lint rules from the years before, just to completely start this repo over! 😃

@ParaskP7
Copy link
Contributor

Another update on this @mkevins !

I tried that again today as I was trying to gather some updated metrics for the project thread and those 3 x LintError for JP/WPAndroid start reappearing, see screenshots below... 🤷

JPAndroid

image

WPAndroid

image


Plus, see screenshot below on such 1 x LintError for FluxC:

FluxC

image

FYI: This 1 x LintError is related to FluxCImageLoader.java, see screenshot below:

image

Base automatically changed from update-project-apis to trunk August 10, 2023 23:31
@mkevins
Copy link
Contributor Author

mkevins commented Aug 22, 2023

As I was trying to reply to you here, and updating the lint.xml configuration on my side, by removing the severity="informational" I unnecessary included, and adding the extra configuration I missed for the new MissingNullAnnotationOnConstructorParameter issue, I noticed that running ./gradlew lintJetpackWasabiDebug worked this time, see updated screenshot below! 🎉

I was finally able to reproduce this issue, by using the same command ./gradlew lintJetpackWasabiDebug. Weirdly, sometimes it would succeed without error, but changing flavors would "re-trigger" the error.. so it's possible something flakey is going on with the caches.

That said, I've had some time to dig further into this, and I looked at the error report that was generated for these Java files. It seems there is a NullPointerException (how fitting 😅) when accessing the node asSourceString for a constructor parameter in the first three errors:

Stack:
  NullPointerException:ClsFileImpl.getMirror(ClsFileImpl.java:342)
  ClsElementImpl.getMirror(ClsElementImpl.java:131)
  ClsElementImpl.getText(ClsElementImpl.java:174)
  JavaAbstractUElement.asSourceString(JavaAbstractUElement.kt:23)
  MissingNullAnnotationDetectorKt.getFixes(MissingNullAnnotationDetector.kt:153)

So, based on that clue, I wrote some temporary code to aid in debugging:

Debugging patch
diff --git a/WordPressLint/src/main/java/org/wordpress/android/lint/MissingNullAnnotationDetector.kt b/WordPressLint/src/main/java/org/wordpress/android/lint/MissingNullAnnotationDetector.kt
index 14699f4..6a840b4 100644
--- a/WordPressLint/src/main/java/org/wordpress/android/lint/MissingNullAnnotationDetector.kt
+++ b/WordPressLint/src/main/java/org/wordpress/android/lint/MissingNullAnnotationDetector.kt
@@ -48,10 +48,23 @@ class MissingNullAnnotationDetector : Detector(), SourceCodeScanner {
 
             private fun visitParameter(node: UMethod, parameter: UParameter) {
                 if (parameter.requiresNullAnnotation && !parameter.isNullAnnotated) {
-                    if (node.isConstructor)
-                        report(parameter, MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION)
-                    else
+                    if (node.isConstructor) {
+//                        report(parameter, MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION)
+                        var source: String? = null
+
+                        runCatching {
+                            source = parameter.asSourceString()
+                        }
+                        source?.also {
+                            report(parameter, MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION)
+                        } ?: fakeReport(
+                                parameter,
+                                MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION,
+                                "snaplog:\n  node = $node\n  parameter = $parameter",
+                        )
+                    } else {
                         report(parameter, MISSING_METHOD_PARAMETER_ANNOTATION)
+                    }
                 }
             }
         }
@@ -140,6 +153,12 @@ private fun Issue.Companion.create(
 )
 
 /* JavaContext Extensions */
+private fun JavaContext.fakeReport(node: UElement, issue: Issue, message: String) = report(
+        issue,
+        node,
+        getLocation(node),
+        message,
+)
 private fun JavaContext.report(node: UElement, issue: Issue) = report(
         issue,
         node,
diff --git a/WordPressLint/src/test/java/org/wordpress/android/lint/MissingNullAnnotationDetectorTest.kt b/WordPressLint/src/test/java/org/wordpress/android/lint/MissingNullAnnotationDetectorTest.kt
index 1bfab47..fd1b790 100644
--- a/WordPressLint/src/test/java/org/wordpress/android/lint/MissingNullAnnotationDetectorTest.kt
+++ b/WordPressLint/src/test/java/org/wordpress/android/lint/MissingNullAnnotationDetectorTest.kt
@@ -2,11 +2,13 @@ package org.wordpress.android.lint
 
 import com.android.tools.lint.checks.infrastructure.LintDetectorTest
 import com.android.tools.lint.checks.infrastructure.TestLintTask.lint
+import org.junit.Ignore
 import org.junit.Test
 import org.wordpress.android.lint.Utils.nonNullClass
 import org.wordpress.android.lint.Utils.nullableClass
 
 
+@Ignore
 class MissingNullAnnotationDetectorTest {
     @Test
     fun `it should inform when null annotation is missing on field`() {

tl;dr is that this catches the NPE and reports a "fake" issue to provide more diagnostics about the bug directly in the lint report. In particular, information about the UMethod and UParameter being analyzed at the time of the crash. And: 🎉 this gives us some paydirt:

~/.gradle/caches/transforms-3/07106e70c7e9e12caa2c0f9e63d8e134/transformed/recyclerview-1.3.0/jars/classes.jar!/androidx/recyclerview/widget/LinearSmoothScroller.class: Information: snaplog:
   node = @android.annotation.SuppressLint(null = <noref>([!] UnknownJavaExpression (PsiLiteralExpression:"UnknownNullness")))
 public fun <anon-init>(context: android.content.Context) = UastEmptyExpression
   parameter = var context: android.content.Context [MissingNullAnnotationOnConstructorParameter from org.wordpress.android.lint]

and two others that look nearly identical, except for LinearSmoothScroller.class being replaced by StringRequest.class and WebChromeClientWithVideoPoster.class, respectively. Armed with these clues, the picture becomes more clear about what is the culprit: these all correspond to a particular pattern in the aforementioned WordPress-Android Java files where these lint rule errors originated: anonymous subclass implementations: here, here, and here.

So, it seems that lint is choking on the anonymous implementation when it tries to find the "source string" of the constructor parameter (since it doesn't really exist 😅). Good news is, we can just ignore this scenario, since there is nothing to annotate anyway. 😃 I have pushed an update for this: dfaedd1, and this is ready for another look.

@ParaskP7
Copy link
Contributor

👋 @mkevins !

I was finally able to reproduce this issue, by using the same command ./gradlew lintJetpackWasabiDebug. Weirdly, sometimes it would succeed without error, but changing flavors would "re-trigger" the error.. so it's possible something flakey is going on with the caches.

👍 + 🤷

That said, I've had some time to dig further into this, and I looked at the error report that was generated for these Java files.

🥇

It seems there is a NullPointerException (how fitting 😅) when accessing the node asSourceString for a constructor parameter in the first three errors:

How fitting indeed... 😅

So, based on that clue, I wrote some temporary code to aid in debugging:

🥇

tl;dr is that this catches the NPE and reports a "fake" issue to provide more diagnostics about the bug directly in the lint report. In particular, information about the UMethod and UParameter being analyzed at the time of the crash. And: 🎉 this gives us some paydirt:

🎉

and two others that look nearly identical, except for LinearSmoothScroller.class being replaced by StringRequest.class and WebChromeClientWithVideoPoster.class, respectively.

🤔

Armed with these clues, the picture becomes more clear about what is the culprit: these all correspond to a particular pattern in the aforementioned WordPress-Android Java files where these lint rule errors originated: anonymous subclass implementations: here, here, and here.

Very interesting, great investigation skills Matt! 💯 x 💯 ^ 💯

So, it seems that lint is choking on the anonymous implementation when it tries to find the "source string" of the constructor parameter (since it doesn't really exist 😅).

Stop choking YOU! 😅

Good news is, we can just ignore this scenario, since there is nothing to annotate anyway.

I agree! 💯

😃 I have pushed an update for this: dfaedd1, and this is ready for another look.

Did take another look, plus tested this update and everything is now working as expected, I think we are ready to start using this rule in production Matt, starting with WP/JPAndroid (🎉), that is, give-or-take this and this, which are still pending, wdyt? 🤔

@mkevins
Copy link
Contributor Author

mkevins commented Aug 23, 2023

Btw @mkevins , now that the WordPressIssueRegistry only points to those 4 new issues, all related to the new MissingNullAnnotationDetector custom Android Lint rule, how about deleting the 2 outdated WordPressAndroidImportInViewModelDetector and WordPressRtlCodeDetector custom Android Lint rules from the years before, just to completely start this repo over! 😃

I think this is a good idea, but I also think that removing this code is quite large, and thus could be handled in a separate PR (especially given the title and description of this current PR). I'll open an issue for it 👍

Update: I've opened an issue for this: #12

@mkevins
Copy link
Contributor Author

mkevins commented Aug 23, 2023

👋 @mkevins !

Where do you envision this configuration being added? I don't think it should be within the scanner itself, since that would mean we'd still be traversing into those directories and possibly even parsing / building the UAST before ignoring rule at time of visiting. I imagine you have something else in mind, though, and if so, would you mind elaborating with a specific suggestion?

I actually do believe (if possible) that this be included within the scanner itself. Otherwise, every client of this library would need to be adding this extra lint.xml configuration below to get it working property:

    <issue id="MissingNullAnnotationOnField">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnConstructorParameter">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnMethodParameter">
        <ignore path="**/generated/**" />
    </issue>
    <issue id="MissingNullAnnotationOnMethodReturnType">
        <ignore path="**/generated/**" />
    </issue>

What I am envisioning is that a custom Android Lint rule having a way to ignore by default anything build folder related as this **/generated/** path is locally within it. Wdyt? 🤔

Now that we've resolved the main blocking issue (the crash resulting from lint analysis of the unusual anonymous subclass implementations), I don't think altering the scanner is necessary for this PR. Also, in my testing, adding those lines even in the config/lint/lint.xml file of the WordPress-Android repository, did not actually resolve the lint crashes described above.

Also, our configuration within WordPress-Android already has many such exclusions, but this is likely because we are explicitly including generated sources in our build.gradle (source).

From the documentation on the Lint DSL object:

// Normally lint will skip generated sources, but you can turn it on with this flag
checkGeneratedSources true

So, considering all of this, I don't think it is in the purview of this detector to override configurations declared explicitly at the usage site. Wdyt?

@ParaskP7
Copy link
Contributor

I think this is a good idea, but I also think that removing this code is quite large, and thus could be handled in a separate PR (especially given the title and description of this current PR). I'll open an issue for it 👍

Update: I've opened an issue for this: #12

I agree, thanks for creating this issue @mkevins ! 💯

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 23, 2023

👋 @mkevins !

Now that we've resolved the main blocking issue (#11 (comment)), I don't think altering the scanner is necessary for this PR.

👍

Also, in my testing, adding those lines even in the config/lint/lint.xml file of the WordPress-Android repository, did not actually resolve the lint crashes described above.

Yea, this is not related to the Lint crashes, but actually a separate issue, apologies for the confusion. 🫂

Also, our configuration within WordPress-Android already has many such exclusions, but this is likely because we are explicitly including generated sources in our build.gradle (source).

From the documentation on the Lint DSL object:

// Normally lint will skip generated sources, but you can turn it on with this flag
checkGeneratedSources true

Hmmm... I didn't think of that, which is the fact that having checkGeneratedSources = true might be causing all this, very interesting! 🤔

So, considering all of this, I don't think it is in the purview of this detector to override configurations declared explicitly at the usage site. Wdyt?

I am convinced Matt, let's keep things as is for now and maybe revisit this later on (if needed), thanks so much for taking a closer look at it. 🥇


🚀 🚀 🚀 I THINK WE ARE READY TO MERGE THIS & START USING THIS CUSTOM ANDROID LINT RULE! 🚀 🚀 🚀

@mkevins mkevins merged commit b2786a4 into trunk Aug 23, 2023
@mkevins mkevins deleted the add/missing-null-annotation-detector branch August 23, 2023 23:28
@mkevins
Copy link
Contributor Author

mkevins commented Aug 23, 2023

Thanks for all your help getting this into shape Petros!

I THINK WE ARE READY TO MERGE THIS & START USING THIS CUSTOM ANDROID LINT RULE!

I have merged it, but I think we still need to add some GitHub action, or other CI pipeline to publish to s3 before we can use it without using mavenLocal, right?

@ParaskP7
Copy link
Contributor

Thank YOU @mkevins ! 🥇 💯 🙇

I have merged it, but I think we still need to add some GitHub action, or other CI pipeline to publish to s3 before we can use it without using mavenLocal, right?

Right, true, I meant start using this custom Android Lint rule after we short out the publishing part, another PR perhaps, after we are done with #12 , wdyt? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants