Skip to content

Commit eacb6c0

Browse files
authored
Merge pull request #21 from wordpress-mobile/do_not_mock_detectors
Introduce lint checks for mocking data and sealed classes
2 parents b374448 + 656fe14 commit eacb6c0

File tree

11 files changed

+279
-50
lines changed

11 files changed

+279
-50
lines changed

WordPressLint/README.md

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
Below you can read the AI-generated, and human-improved explanation of the shadow JAR configuration, that's
2+
result of [#21](https://github.com/wordpress-mobile/WordPress-Lint-Android/pull/21).
3+
4+
# Shadow JAR Logic Explanation
5+
6+
## Overview
7+
This document explains the logic behind the Shadow JAR configuration in the WordPressLint/build.gradle file, specifically the `shadowJar`, `cleanServiceFile` task, and publishing setup.
8+
9+
## The Problem Being Solved
10+
11+
The WordPress Lint library has a unique requirement:
12+
1. It needs to include **selected** lint rules from Slack's lint library (not all of them)
13+
2. It must bundle all necessary classes into a single JAR file for distribution, **without declaring Slack Lint as a regular dependency**, to prevent Android Lint from loading its full issue registry and enabling all of its checks.
14+
3. It needs to ensure proper service registration with Android Lint framework
15+
16+
## How Android Lint Service Discovery Works
17+
18+
Android Lint uses Java's ServiceLoader mechanism to discover lint rule providers. It looks for files in:
19+
```
20+
META-INF/services/com.android.tools.lint.client.api.IssueRegistry
21+
```
22+
23+
These files contain fully qualified class names of classes that implement `IssueRegistry`.
24+
25+
## The WordPress Approach
26+
27+
### 1. WordPressIssueRegistry Implementation
28+
```kotlin
29+
class WordPressIssueRegistry : IssueRegistry() {
30+
private val slackIssueRegistry = SlackIssueRegistry()
31+
32+
override val issues: List<Issue>
33+
get() {
34+
val allOwnIssues = listOf(/* WordPress-specific issues */)
35+
val selectedSlackIssues = slackIssueRegistry.issues.filter { /* only specific ones */ }
36+
return allOwnIssues + selectedSlackIssues
37+
}
38+
}
39+
```
40+
41+
**Key Point**: WordPress programmatically creates a SlackIssueRegistry instance and selectively includes only the rules it wants.
42+
43+
### 2. Service Registration
44+
The WordPress service file contains only:
45+
```
46+
org.wordpress.android.lint.WordPressIssueRegistry
47+
```
48+
49+
## The Shadow JAR Process
50+
51+
### Step 1: shadowJar Task
52+
```gradle
53+
shadowJar {
54+
mergeServiceFiles()
55+
}
56+
```
57+
58+
**What happens:**
59+
- Creates a "fat JAR" containing all dependencies (including Slack lint library)
60+
- `mergeServiceFiles()` combines all `META-INF/services/*` files from all JARs
61+
- **Problem**: This would merge WordPress's service file with Slack's service file, resulting in:
62+
```
63+
org.wordpress.android.lint.WordPressIssueRegistry
64+
slack.lint.SlackIssueRegistry
65+
```
66+
67+
### Step 2: cleanServiceFile Task
68+
```gradle
69+
tasks.register('cleanServiceFile') {
70+
doLast {
71+
// Extract the JAR
72+
// Find the service file
73+
// Remove lines containing "slack.lint.SlackIssueRegistry"
74+
// Repackage the JAR
75+
}
76+
}
77+
```
78+
79+
**What happens:**
80+
- Extracts the shadow JAR to a temporary directory
81+
- Locates the merged service file
82+
- Removes any lines containing `slack.lint.SlackIssueRegistry`
83+
- Repackages the JAR
84+
85+
**Result**: The final JAR contains only:
86+
```
87+
org.wordpress.android.lint.WordPressIssueRegistry
88+
```
89+
90+
## Why This Approach?
91+
92+
### Without the cleanServiceFile task:
93+
1. Both `WordPressIssueRegistry` and `SlackIssueRegistry` would be registered
94+
2. Android Lint would load both registries
95+
3. This could cause conflicts, performance issues, or unexpected behavior. And actually enabling all issues from `SlackIssueRegistry`
96+
97+
### With the cleanServiceFile task:
98+
1. Only `WordPressIssueRegistry` is registered as a service
99+
2. WordPress has full control over which Slack rules are included
100+
3. No duplication or conflicts
101+
4. Clean, predictable behavior
102+
103+
## Publishing Configuration: Why `components["shadow"]` instead of `components.java`?
104+
105+
```gradle
106+
publishing {
107+
publications {
108+
maven(MavenPublication) {
109+
from(components["shadow"]) // Uses the cleaned shadow JAR
110+
groupId = 'org.wordpress'
111+
artifactId = 'lint'
112+
}
113+
}
114+
}
115+
```
116+
117+
### The Key Difference
118+
119+
**If we used `from components.java`:**
120+
- Would publish only the compiled classes from this project (WordPress lint rules)
121+
- Dependencies would be listed as external dependencies in the POM file
122+
- Consumers would need to resolve and download the Slack lint library separately
123+
- The original service file would be published (not the cleaned one)
124+
- Result: A regular JAR that requires dependency resolution and all Slack rules are enabled
125+
126+
**Using `from(components["shadow"])`:**
127+
- Publishes the complete shadow JAR with all dependencies included (fat JAR)
128+
- All Slack lint classes are embedded directly in the published JAR
129+
- The cleaned service file is included (with SlackIssueRegistry removed)
130+
- Consumers get a single, self-contained JAR with no external dependencies
131+
- Result: A fat JAR ready for immediate use and only selected Slack rules are enabled
132+
133+
### Why This Matters
134+
135+
1. **Not all Slack rules are enabled**: Only selected ones in `WordPressIssueRegistry`
136+
1. **Service File Integrity**: The cleaned service file (after `cleanServiceFile` task) must be the one that gets published, not the original
137+
2. **Self-Contained Distribution**: Consumers don't need to worry about resolving the Slack lint dependency
138+
3. **Controlled Dependency**: The Slack dependency is embedded and controlled, not exposed as a transitive dependency
139+
4. **Simplified Consumption**: One JAR file contains everything needed to run the WordPress lint rules
140+
141+
### The Publishing Flow
142+
143+
1. `shadowJar` task creates the fat JAR with merged dependencies
144+
2. `cleanServiceFile` task removes unwanted service registrations
145+
3. `components["shadow"]` references the final, cleaned shadow JAR
146+
4. Publishing uploads this processed JAR to the repository
147+
148+
This ensures that what gets published is exactly what was intended: a self-contained JAR with WordPress rules + selected Slack rules, with proper service registration.
149+
150+
## Summary
151+
152+
This configuration solves a complex dependency management problem:
153+
- **Goal**: Include selected rules from Slack lint library in WordPress lint
154+
- **Challenge**: Avoid service registration conflicts and rule duplication
155+
- **Solution**: Use Shadow JAR to bundle dependencies, then clean up service files to ensure only WordPress registry is registered
156+
- **Result**: A single JAR with WordPress rules + selected Slack rules, properly registered with Android Lint
157+
158+
The seemingly complex build logic is actually an elegant solution to ensure clean service discovery while reusing valuable lint rules from the Slack library.

WordPressLint/build.gradle

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ plugins {
44
id 'com.android.lint'
55
id 'maven-publish'
66
id "com.automattic.android.publish-to-s3"
7+
id "com.gradleup.shadow" version '8.3.8'
78
}
89

910
lint {
@@ -21,18 +22,50 @@ repositories {
2122

2223
dependencies {
2324
compileOnly "com.android.tools.lint:lint-api:$lintVersion"
24-
25+
implementation "com.slack.lint:slack-lint-checks:0.9.0"
2526
testImplementation "junit:junit:4.13.2"
27+
testImplementation "com.android.tools.lint:lint-api:$lintVersion"
2628
testImplementation "com.android.tools.lint:lint-tests:$lintVersion"
2729
}
2830

31+
shadowJar {
32+
mergeServiceFiles()
33+
}
34+
35+
shadowJar.finalizedBy('cleanServiceFile')
36+
37+
tasks.register('cleanServiceFile') {
38+
doLast {
39+
def jarFile = file(shadowJar.archiveFile.get())
40+
def unzipDir = layout.buildDirectory.dir("tmp/cleanedJar").get().asFile
41+
42+
copy {
43+
from zipTree(jarFile)
44+
into unzipDir
45+
}
46+
47+
def serviceFile = file("$unzipDir/META-INF/services/com.android.tools.lint.client.api.IssueRegistry")
48+
if (serviceFile.exists()) {
49+
serviceFile.text = serviceFile.readLines()
50+
.findAll { !it.contains("slack.lint.SlackIssueRegistry") }
51+
.join('\n')
52+
}
53+
54+
ant.zip(destfile: jarFile, basedir: unzipDir)
55+
}
56+
}
57+
2958
sourceCompatibility = JavaVersion.VERSION_17
3059
targetCompatibility = JavaVersion.VERSION_17
3160

61+
compileKotlin {
62+
kotlinOptions.jvmTarget = "17"
63+
}
64+
3265
publishing {
3366
publications {
3467
maven(MavenPublication) {
35-
from components.java
68+
from(components["shadow"])
3669

3770
groupId = 'org.wordpress'
3871
artifactId = 'lint'

WordPressLint/src/main/java/org/wordpress/android/lint/MissingNullAnnotationDetector.kt

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ class MissingNullAnnotationDetector : Detector(), SourceCodeScanner {
3232
)
3333

3434
override fun createUastHandler(context: JavaContext) = with(context) {
35-
if (!isJava(uastFile?.sourcePsi)) {
35+
val sourcePsi= uastFile?.sourcePsi
36+
if (!(sourcePsi != null && isJava(sourcePsi.language))) {
3637
return UElementHandler.NONE
3738
}
3839
object : UElementHandler() {
@@ -131,11 +132,26 @@ private val UMethod.isEnum
131132
get() = returnType is PsiEnumConstant
132133

133134
/* UAnnotated Extensions */
134-
private val UAnnotated.isNullAnnotated
135-
get() = uAnnotations.any { annotation ->
136-
MissingNullAnnotationDetector.acceptableNullAnnotations.any { nullAnnotation ->
137-
annotation.qualifiedName == nullAnnotation
135+
private val UAnnotated.isNullAnnotated: Boolean
136+
get() {
137+
// Check UAST annotations first, as this is the primary source
138+
val uastHasAnnotation = uAnnotations.any { annotation ->
139+
MissingNullAnnotationDetector.acceptableNullAnnotations.any { nullAnnotation ->
140+
annotation.qualifiedName == nullAnnotation
141+
}
138142
}
143+
144+
// If it's a UMethod and UAST didn't find it, check PSI annotations
145+
if (this is UMethod && !uastHasAnnotation) {
146+
return (sourcePsi as? com.intellij.psi.PsiMethod)?.annotations?.any { annotation ->
147+
MissingNullAnnotationDetector.acceptableNullAnnotations.any { nullAnnotation ->
148+
annotation.qualifiedName == nullAnnotation
149+
}
150+
} ?: false
151+
}
152+
153+
// Otherwise, return the result from the UAST check
154+
return uastHasAnnotation
139155
}
140156

141157
/* Issue.Companion Extensions */

WordPressLint/src/main/java/org/wordpress/android/lint/WordPressIssueRegistry.kt

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,25 +3,41 @@ package org.wordpress.android.lint
33
import com.android.tools.lint.client.api.IssueRegistry
44
import com.android.tools.lint.client.api.Vendor
55
import com.android.tools.lint.detector.api.CURRENT_API
6+
import com.android.tools.lint.detector.api.Issue
7+
import slack.lint.SlackIssueRegistry
8+
import slack.lint.mocking.DataClassMockDetector
9+
import slack.lint.mocking.SealedClassMockDetector
610

711
class WordPressIssueRegistry : IssueRegistry() {
12+
13+
private val slackIssueRegistry = SlackIssueRegistry()
14+
815
override val api = CURRENT_API
916
override val minApi = MIN_API
10-
override val issues
11-
get() = listOf(
17+
override val issues: List<Issue>
18+
get() {
19+
val allOwnIssues = listOf(
1220
MissingNullAnnotationDetector.MISSING_FIELD_ANNOTATION,
1321
MissingNullAnnotationDetector.MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION,
1422
MissingNullAnnotationDetector.MISSING_METHOD_PARAMETER_ANNOTATION,
15-
MissingNullAnnotationDetector.MISSING_METHOD_RETURN_TYPE_ANNOTATION,
16-
)
23+
MissingNullAnnotationDetector.MISSING_METHOD_RETURN_TYPE_ANNOTATION
24+
)
25+
26+
val allSlackIssues = slackIssueRegistry.issues
27+
val selectedSlackIssues = allSlackIssues.filter { issue ->
28+
issue.id == DataClassMockDetector.issue.id || issue.id == SealedClassMockDetector.issue.id
29+
}
30+
31+
return allOwnIssues + selectedSlackIssues
32+
}
1733

1834
override val vendor = Vendor(
19-
vendorName = "WordPress Android",
20-
feedbackUrl = "https://github.com/wordpress-mobile/WordPress-Lint-Android/issues",
21-
identifier = "org.wordpress.android.lint",
35+
vendorName = "WordPress Android",
36+
feedbackUrl = "https://github.com/wordpress-mobile/WordPress-Lint-Android/issues",
37+
identifier = "org.wordpress.android.lint",
2238
)
2339

2440
companion object {
2541
const val MIN_API = 10
2642
}
27-
}
43+
}

WordPressLint/src/test/java/org/wordpress/android/lint/MissingNullAnnotationDetectorTest.kt

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ class MissingNullAnnotationDetectorTest {
2020
.issues(MissingNullAnnotationDetector.MISSING_FIELD_ANNOTATION)
2121
.run()
2222
.expect("""
23-
src/test/ExampleClass.java:4: Information: Missing null annotation [MissingNullAnnotationOnField]
23+
src/test/ExampleClass.java:4: Hint: Missing null annotation [MissingNullAnnotationOnField]
2424
String mExampleField = "example";
2525
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
26-
0 errors, 0 warnings
26+
0 errors, 0 warnings, 1 hint
2727
"""
2828
.trimIndent()
2929
)
@@ -93,10 +93,10 @@ class MissingNullAnnotationDetectorTest {
9393
.issues(MissingNullAnnotationDetector.MISSING_METHOD_RETURN_TYPE_ANNOTATION)
9494
.run()
9595
.expect("""
96-
src/test/ExampleClass.java:4: Information: Missing null annotation [MissingNullAnnotationOnMethodReturnType]
96+
src/test/ExampleClass.java:4: Hint: Missing null annotation [MissingNullAnnotationOnMethodReturnType]
9797
String getMessage() {
9898
~~~~~~~~~~
99-
0 errors, 0 warnings
99+
0 errors, 0 warnings, 1 hint
100100
"""
101101
.trimIndent()
102102
)
@@ -156,10 +156,10 @@ class MissingNullAnnotationDetectorTest {
156156
.issues(MissingNullAnnotationDetector.MISSING_METHOD_PARAMETER_ANNOTATION)
157157
.run()
158158
.expect("""
159-
src/test/ExampleClass.java:4: Information: Missing null annotation [MissingNullAnnotationOnMethodParameter]
159+
src/test/ExampleClass.java:4: Hint: Missing null annotation [MissingNullAnnotationOnMethodParameter]
160160
String getMessage(String name) {
161161
~~~~~~~~~~~
162-
0 errors, 0 warnings
162+
0 errors, 0 warnings, 1 hint
163163
"""
164164
.trimIndent()
165165
)
@@ -216,10 +216,10 @@ class MissingNullAnnotationDetectorTest {
216216
.issues(MissingNullAnnotationDetector.MISSING_CONSTRUCTOR_PARAMETER_ANNOTATION)
217217
.run()
218218
.expect("""
219-
src/test/ExampleClass.java:4: Information: Missing null annotation [MissingNullAnnotationOnConstructorParameter]
219+
src/test/ExampleClass.java:4: Hint: Missing null annotation [MissingNullAnnotationOnConstructorParameter]
220220
ExampleClass(String name) {}
221221
~~~~~~~~~~~
222-
0 errors, 0 warnings
222+
0 errors, 0 warnings, 1 hint
223223
"""
224224
.trimIndent()
225225
)

build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,5 @@ plugins {
33
}
44

55
ext {
6-
lintVersion = '31.1.2'
6+
lintVersion = '31.11.0'
77
}

gradle/wrapper/gradle-wrapper.jar

-19.2 KB
Binary file not shown.

gradle/wrapper/gradle-wrapper.properties

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
distributionBase=GRADLE_USER_HOME
22
distributionPath=wrapper/dists
3-
distributionUrl=https\://services.gradle.org/distributions/gradle-8.2.1-all.zip
3+
distributionSha256Sum=ed1a8d686605fd7c23bdf62c7fc7add1c5b23b2bbc3721e661934ef4a4911d7c
4+
distributionUrl=https\://services.gradle.org/distributions/gradle-8.14.3-all.zip
45
networkTimeout=10000
56
validateDistributionUrl=true
67
zipStoreBase=GRADLE_USER_HOME

0 commit comments

Comments
 (0)