Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Feature: Lint Checks #158

Merged
merged 26 commits into from
Sep 24, 2018
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
81221da
initial setup of the thirty inch-lint module
jreehuis-gcx Aug 1, 2018
b6bfdad
first Implementation for Java inspired by #111
jreehuis-gcx Aug 1, 2018
616ad0b
configures gradle to include the TI lint checks in the TI lib
jreehuis-gcx Aug 27, 2018
770dfd6
fixes not executed lint checks in AS
jreehuis-gcx Aug 27, 2018
040e117
create own TI lint category
jreehuis-gcx Aug 27, 2018
aebadbe
improves the highlighted editor element for the TiViewMissing lint issue
jreehuis-gcx Aug 27, 2018
0166387
some comment cleanup
jreehuis-gcx Aug 27, 2018
d88c24c
adds comments about the correct lintVersion
jreehuis-gcx Aug 27, 2018
2b8e3f0
also register the Lint-Registry-v2
jreehuis-gcx Aug 27, 2018
6fe43a9
removes the empty README.md
jreehuis-gcx Aug 28, 2018
3d43e9b
removes obsolete kotlin stdlib inclusion
jreehuis-gcx Aug 28, 2018
9b34699
removes obsolete defaultTasks
jreehuis-gcx Aug 28, 2018
7b5ac3e
removes obsolete sourceCompatibility
jreehuis-gcx Aug 28, 2018
da0b916
improves the issue val overriding syntax
jreehuis-gcx Aug 28, 2018
fa159cb
corrects comment
jreehuis-gcx Aug 28, 2018
dfafaa0
adds Kolin Tests
jreehuis-gcx Sep 13, 2018
a0ce173
adds a more complex inheritance scenario as test
jreehuis-gcx Sep 13, 2018
d339edf
fixes false positive when having a base presenter class
jreehuis-gcx Sep 13, 2018
fd26e60
adds missing lint version specification
jreehuis-gcx Sep 13, 2018
38892fa
fixes wrong test conditions
jreehuis-gcx Sep 14, 2018
d641968
also implements the basePresenter tests for the fragment
jreehuis-gcx Sep 14, 2018
620feca
Improves the TI Category priority to align with Category.CORRECTNESS
jreehuis-gcx Sep 14, 2018
ce5da87
makes the findViewInterface method signature more abstract
jreehuis-gcx Sep 17, 2018
ea76784
implements more robust view type determination
jreehuis-gcx Sep 17, 2018
bf9f5c2
adds more tests for the better view type implementation
jreehuis-gcx Sep 17, 2018
139f3f8
enables the ViewNotImplementedCheck for the TiDialogFragment
jreehuis-gcx Sep 17, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ buildscript {
google()
}
dependencies {
classpath "com.android.tools.build:gradle:3.1.3"
classpath "com.android.tools.build:gradle:3.1.3" // if you update this, also update the lintVersion below
classpath "com.vanniktech:gradle-android-junit-jacoco-plugin:0.10.0"
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
}
Expand Down Expand Up @@ -45,4 +45,9 @@ ext {
assertjVersion = '2.8.0'
supportTestVersion = '1.0.1'
espressoVersion = '3.0.1'

// According to https://github.com/googlesamples/android-custom-lint-rules/tree/master/android-studio-3
// the lint version should match to the used Android Gradle Plugin by the formula "AGP Version X.Y.Z + 23.0.0"
// E.g. "AGP Version 3.1.3 + 23.0.0 = Lint Version 26.1.3"
lintVersion = '26.1.3'
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also calculate this by ourselfs, right? 🤔
Just extract the AGP version and "add" the values to 23.0.0 ...

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can calculate this ourself. But I would prefer to change that still manually:

  • It is stated "should" and so the actual version might differ from the formula
  • Google recently just decoupled the Support Lib Versions and so it might be that this version gets uncoupled from the build tools version aswell

}
3 changes: 2 additions & 1 deletion settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ include(
":thirtyinch-rx2",
":thirtyinch-test",
":thirtyinch-kotlin",
":thirtyinch-lint",
":sample",
":plugin-test"
)
)
7 changes: 7 additions & 0 deletions thirtyinch-lint/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.idea
build/
.gradle
gradle
gradlew
gradlew.bat
.idea/workspace.xml
48 changes: 48 additions & 0 deletions thirtyinch-lint/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
plugins {
id "org.jetbrains.kotlin.jvm"
id "jacoco"
}

repositories {
jcenter()
}

configurations {
lintChecks
}

dependencies {
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8"

compileOnly "com.android.tools.lint:lint-api:$lintVersion"
compileOnly "com.android.tools.lint:lint-checks:$lintVersion"

testImplementation 'junit:junit:4.12'
testImplementation "org.assertj:assertj-core:$assertjVersion"

testImplementation "com.android.tools.lint:lint:$lintVersion"
testImplementation "com.android.tools.lint:lint-tests:$lintVersion"

lintChecks files(jar)
}

jar {
manifest {
attributes("Manifest-Version": 1.0)
attributes("Lint-Registry": "net.grandcentrix.thirtyinch.lint.TiLintRegistry")
// The TI checks are build with the new 3.0 APIs (including UAST) so we should also register the v2 lint registry.
attributes("Lint-Registry-v2": "net.grandcentrix.thirtyinch.lint.TiLintRegistry")
}
}

compileKotlin {
kotlinOptions {
jvmTarget = "1.8"
}
}

compileTestKotlin {
kotlinOptions {
jvmTarget = "1.8"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package net.grandcentrix.thirtyinch.lint

import com.android.tools.lint.detector.api.Category
import com.android.tools.lint.detector.api.Detector
import com.android.tools.lint.detector.api.Implementation
import com.android.tools.lint.detector.api.Issue
import com.android.tools.lint.detector.api.Scope
import com.android.tools.lint.detector.api.Severity

private val CATEGORY_TI = Category.create("ThirtyInch", 90)

sealed class TiIssue(
val id: String,
val briefDescription: String,
val category: Category,
val priority: Int,
val severity: Severity
) {

object MissingView : TiIssue(
id = "MissingTiViewImplementation",
briefDescription = "TiView Implementation missing in class",
category = CATEGORY_TI,
priority = 8,
severity = Severity.ERROR
)

fun asLintIssue(detectorCls: Class<out Detector>, description: String = briefDescription): Issue =
Issue.create(
id,
briefDescription,
description,
category,
priority,
severity,
Implementation(
detectorCls,
Scope.JAVA_FILE_SCOPE
)
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package net.grandcentrix.thirtyinch.lint

import com.android.tools.lint.client.api.IssueRegistry
import com.android.tools.lint.detector.api.Issue
import net.grandcentrix.thirtyinch.lint.detector.MissingViewInCompositeDetector
import net.grandcentrix.thirtyinch.lint.detector.MissingViewInThirtyInchDetector

class TiLintRegistry : IssueRegistry() {
override val issues: List<Issue>
get() = listOf(
MissingViewInThirtyInchDetector.ISSUE.apply {
setEnabledByDefault(true)
},
MissingViewInCompositeDetector.ISSUE.apply {
setEnabledByDefault(true)
}
)

override val api: Int = com.android.tools.lint.detector.api.CURRENT_API
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package net.grandcentrix.thirtyinch.lint.detector

import com.android.tools.lint.detector.api.Detector
import com.android.tools.lint.detector.api.Issue
import com.android.tools.lint.detector.api.JavaContext
import com.android.tools.lint.detector.api.TextFormat
import com.intellij.psi.PsiClass
import com.intellij.psi.PsiClassType
import com.intellij.psi.PsiType
import com.intellij.psi.util.PsiUtil
import org.jetbrains.uast.UClass
import org.jetbrains.uast.getUastContext

// Base class for Lint checks centered around the notion of "TiView not implemented"
abstract class BaseMissingViewDetector : Detector(), Detector.UastScanner {

/**
* The Issue that the detector is connected to, reported on illegal state detection
*/
abstract val issue: Issue

/**
* The list of super-classes to detect.
* We're forcing sub-classed Detectors to implement this by means of redeclaration
*/
abstract override fun applicableSuperClasses(): List<String>

/**
* Tries to extract the PsiType of the TiView sub-class that is relevant for the given declaration.
* The relevant super-class (from applicableSuperClasses()) & its resolved variant are given as well.
*/
abstract fun tryFindViewInterface(context: JavaContext, declaration: UClass, extendedType: PsiClassType,
resolvedType: PsiClass): PsiType?

/**
* Whether or not to allow the absence of an "implements TiView" clause on the given declaration.
* The View interface is given as well to allow for further introspection into the setup of the class at hand.
* When false is returned here, Lint will report the Issue connected to this Detector on the given declaration.
*/
abstract fun allowMissingViewInterface(context: JavaContext, declaration: UClass, viewInterface: PsiType): Boolean

final override fun visitClass(context: JavaContext, declaration: UClass) {
if (!context.isEnabled(issue)) {
return
}
// Don't trigger on abstract classes
if (PsiUtil.isAbstractClass(declaration.psi)) {
return
}
// Extract the MVP View type from the declaration
tryFindViewInterface(context, declaration)?.let { viewInterface ->
// Check if the class implements that interface as well
if (!tryFindViewImplementation(context, declaration, viewInterface)) {
// Interface not implemented; check if alternate condition applies
if (!allowMissingViewInterface(context, declaration, viewInterface)) {
// Invalid state: Report issue for this class
declaration.nameIdentifier?.run {
context.report(
issue,
context.getLocation(this.originalElement),
issue.getBriefDescription(TextFormat.TEXT))
}
}
}
}
}

private fun tryFindViewInterface(context: JavaContext, declaration: UClass): PsiType? {
for (extendedType in declaration.extendsListTypes) {
extendedType.resolveGenerics().element?.let { resolvedType ->
return tryFindViewInterface(context, declaration, extendedType, resolvedType)
}
}
return null
}

private fun tryFindViewImplementation(context: JavaContext, declaration: UClass,
viewInterface: PsiType): Boolean {
for (implementedType in declaration.implementsListTypes) {
if (implementedType == viewInterface) {
return true
}
implementedType.resolve()?.let { resolvedType ->
val uastContext = declaration.getUastContext()
return tryFindViewImplementation(context, uastContext.getClass(resolvedType), viewInterface)
}
}
return false
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package net.grandcentrix.thirtyinch.lint.detector

import com.android.tools.lint.detector.api.Issue
import com.android.tools.lint.detector.api.JavaContext
import com.intellij.psi.PsiClass
import com.intellij.psi.PsiClassType
import com.intellij.psi.PsiJavaCodeReferenceElement
import com.intellij.psi.PsiType
import net.grandcentrix.thirtyinch.lint.TiIssue.MissingView
import org.jetbrains.uast.UBlockExpression
import org.jetbrains.uast.UCallExpression
import org.jetbrains.uast.UClass
import org.jetbrains.uast.UExpression
import org.jetbrains.uast.getUastContext

private const val ADD_PLUGIN_METHOD = "addPlugin"
private const val TI_ACTIVITY_PLUGIN_NAME = "TiActivityPlugin"
private const val TI_FRAGMENT_PLUGIN_NAME = "TiFragmentPlugin"
private val CA_CLASS_NAMES = listOf(
"com.pascalwelsch.compositeandroid.activity.CompositeActivity",
"com.pascalwelsch.compositeandroid.fragment.CompositeFragment"
)

class MissingViewInCompositeDetector : BaseMissingViewDetector() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know if it make sense because we wanted to drop the support vor CompositeAndroid anyway, right @passsy?
But as long as it is implemented now and work we could leave it like it is...

Copy link
Author

Choose a reason for hiding this comment

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

Correct, as long it is still present we should support it.

companion object {
val ISSUE = MissingView.asLintIssue(
MissingViewInCompositeDetector::class.java,
"When using ThirtyInch, a class extending CompositeActivity or CompositeFragment " +
"has to implement the TiView interface associated with it in its signature, " +
"if it applies the respective plugin as well."
)
}

override fun applicableSuperClasses() = CA_CLASS_NAMES

override val issue: Issue = MissingViewInThirtyInchDetector.ISSUE

override fun tryFindViewInterface(context: JavaContext, declaration: UClass, extendedType: PsiClassType,
resolvedType: PsiClass): PsiType? {
// Expect TiPlugin to be applied in the extended CA class
// Found default constructor
val defaultConstructor = declaration.constructors.firstOrNull { it.typeParameters.isEmpty() }

defaultConstructor?.let {
val uastContext = declaration.getUastContext()
val body = uastContext.getMethodBody(defaultConstructor)
return tryFindViewFromCompositeConstructor(context, declaration, body)
}
return null
}

private fun tryFindViewFromCompositeConstructor(context: JavaContext, declaration: UClass,
expression: UExpression?): PsiType? {
if (expression == null) {
return null
}
when (expression) {
is UBlockExpression -> {
// Unwrap block statements; the first resolvable result is returned
expression.expressions
.mapNotNull { tryFindViewFromCompositeConstructor(context, declaration, it) }
.forEach { return it }
}
is UCallExpression -> {
// Inspect call sites
if (ADD_PLUGIN_METHOD == expression.methodName && expression.valueArgumentCount == 1) {
// Expect a plugin to be used as the only argument to this method
val argument = expression.valueArguments[0]
if (argument is UCallExpression) {
val argReference = argument.classReference ?: return null
val resolvedName = argReference.resolvedName
if (TI_ACTIVITY_PLUGIN_NAME == resolvedName || TI_FRAGMENT_PLUGIN_NAME == resolvedName) {
// Matching names. Finally, find the type parameters passed to the plugin
val psiReference = argReference.psi as PsiJavaCodeReferenceElement? ?: return null
val parameterTypes = psiReference.typeParameters
if (parameterTypes.size != 2) {
return null
}
return parameterTypes[1]
}
}
}
}
}
return null
}

override fun allowMissingViewInterface(context: JavaContext, declaration: UClass, viewInterface: PsiType) = false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package net.grandcentrix.thirtyinch.lint.detector

import com.android.tools.lint.detector.api.Issue
import com.android.tools.lint.detector.api.JavaContext
import com.intellij.psi.PsiClass
import com.intellij.psi.PsiClassType
import com.intellij.psi.PsiType
import net.grandcentrix.thirtyinch.lint.TiIssue.MissingView
import org.jetbrains.uast.UClass

private const val TI_VIEW_FQ = "net.grandcentrix.thirtyinch.TiView"
private const val PROVIDE_VIEW_METHOD = "provideView"
private val TI_CLASS_NAMES = listOf(
"net.grandcentrix.thirtyinch.TiActivity",
"net.grandcentrix.thirtyinch.TiFragment"
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a TiDialogFragment which will not be noticed as issue.
Please add it 👍

)

class MissingViewInThirtyInchDetector : BaseMissingViewDetector() {
companion object {
val ISSUE = MissingView.asLintIssue(
MissingViewInThirtyInchDetector::class.java,
"When using ThirtyInch, a class extending TiActivity or TiFragment " +
"has to implement the TiView interface associated with it in its signature, " +
"or implement `provideView()` instead to override this default behaviour."
)
}

override fun applicableSuperClasses() = TI_CLASS_NAMES

override val issue: Issue = ISSUE

override fun tryFindViewInterface(context: JavaContext, declaration: UClass, extendedType: PsiClassType,
resolvedType: PsiClass): PsiType? {
// Expect <P extends TiPresenter, V extends TiView> signature in the extended Ti class
val parameters = extendedType.parameters
val parameterTypes = resolvedType.typeParameters
if (parameters.size != 2 || parameterTypes.size != 2) {
return null
}

// Check that the second type parameter is actually a TiView
val parameterType = parameterTypes[1]
val parameter = parameters[1]
return parameterType.extendsListTypes
.map { it.resolveGenerics().element }
.filter { TI_VIEW_FQ == it?.qualifiedName }
.map { parameter }
.firstOrNull()
}

override fun allowMissingViewInterface(context: JavaContext, declaration: UClass,
viewInterface: PsiType): Boolean {
// Interface not implemented; check if provideView() is overridden instead
return declaration.findMethodsByName(PROVIDE_VIEW_METHOD, true)
.any { viewInterface == it.returnType }
}
}
Loading