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

Added Lint Check for missing TiView implementation #111

Closed
wants to merge 9 commits into from

Conversation

mannodermaus
Copy link

This is an implementation based on the proposal made in #100. It introduces custom lint checks for ThirtyInch with a UAST-based Detector that seeks to point out illegal configuration of TiActivity, TiFragment, CompositeActivity & CompositeFragment sub-classes. Basically, it works like this:

  • Visit every non-abstract sub-class of either of the classes mentioned above
  • Retrieve the TiView sub-interface that the sub-class is connected to
  • Validate that the sub-class implements this TiView by checking its signature

Note that there is an exception to this rule for Ti- classes, wherein a user can override TiViewProvider#provideView() to deviate from the default behaviour. The Detector verifies that, and doesn't report an issue on sub-classes that also override this method.

Integration with CompositeAndroid works in a more low-level fashion, where the default constructor of the Composite- sub-class is analyzed for its registration of a TiPlugin via addPlugin(). If found, the TiView is extracted from there.

Please take a close look at the behaviour of the Detector, and also at potential loopholes inside the unit tests.

"com.pascalwelsch.compositeandroid.activity.CompositeActivity",
"com.pascalwelsch.compositeandroid.fragment.CompositeFragment"));

static final Issue ISSUE = Issue.create(
Copy link
Author

Choose a reason for hiding this comment

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

This should probably be split into two separate Issue descriptions with the same ID, which provide distinct descriptions for Ti and Composite use cases. Otherwise, the part about provideView() is just confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep normal Ti and composite should be separated, especially because we may get rid of composite #109

Copy link
Contributor

@passsy passsy left a comment

Choose a reason for hiding this comment

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

Thank you very much! I'll look into this in detail this week. Also @jreehuis will look over this as he was working on this in parallel.

I wouldn't mind when the lint module was written in kotlin. It could simplify the code drastically in some cases.

@@ -34,4 +34,6 @@ ext {
junitVersion = '4.12'
mockitoVersion = '1.10.19'
assertjVersion = '2.5.0'

lintVersion = '25.4.0-alpha7'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the alpha version required for UAST? Isn't there a stable one?

Copy link
Author

Choose a reason for hiding this comment

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

The latest stable release of the Lint libraries (25.3.0) doesn't include UAST yet, no. I suppose it'll be out once AS 3.0 is stable, though.

"com.pascalwelsch.compositeandroid.activity.CompositeActivity",
"com.pascalwelsch.compositeandroid.fragment.CompositeFragment"));

static final Issue ISSUE = Issue.create(
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep normal Ti and composite should be separated, especially because we may get rid of composite #109


testCompile "com.android.tools.lint:lint:$lintVersion"
testCompile "com.android.tools.lint:lint-tests:$lintVersion"
testCompile 'org.easytesting:fest-assert-core:2.0M10'
Copy link
Contributor

Choose a reason for hiding this comment

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

please use AssertJ as in all other modules

@passsy passsy requested a review from jreehuis July 23, 2017 22:38
@mannodermaus
Copy link
Author

Thanks for the initial feedback! I'd love to rework the module into Kotlin, as it would absolutely allow for cleaner structure in several places.

@mannodermaus
Copy link
Author

I have separated the Detector into two distinct cases for Ti and CompositeAndroid. The implementations themselves are in Kotlin now, as well. The only remaining piece of Java code is present in the unit tests for the Detectors, pretty much only because of IJ's nice syntax highlighting on @Language("JAVA") strings. Ready for review!

Copy link
Contributor

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Beside of that. Not tested yet...

import com.android.tools.lint.detector.api.*
import java.util.*

enum class Issues(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like enum. Especially if we can just create adata class instead.
Addional, I don't see the reason why it is a enum. We cal also creta a "normal" class,,

Copy link
Author

Choose a reason for hiding this comment

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

The Issues type is supposed to declare the limited set of available detections that the ThirtyInch Lint module can analyze. Both data and "normal" classes wouldn't easily allow for this defined set of options, unless we used constants, which I wouldn't encourage. A sealed class could work, yet each implementation would have the same duplicated properties anyway.

"public interface TiView {\n" +
"}");

// private final TestFile caBasePluginStub = java("" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have outcommented tests?
Can be removed then, right?

Copy link
Author

Choose a reason for hiding this comment

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

In the process of separating Lint detection around the different available use cases, these were left behind.

import com.intellij.psi.PsiClassType
import com.intellij.psi.PsiJavaCodeReferenceElement
import com.intellij.psi.PsiType
import org.jetbrains.uast.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't import "everthing" from the uast package. Just the things we need.

Copy link
Author

Choose a reason for hiding this comment

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

AS' default Kotlin code style encourages wildcard imports when at least 5 members of a class are being used. I have adjusted the IDE's settings and rearranged the import statements.

Copy link
Contributor

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

If I running gradlew clean assemble publishToMavenLocal I get the following excepton (for each package):

Could not load custom rule jar file /Users/stefma/Developer/Android/ThirtyInch/thirtyinch/build/intermediates/bundles/default/lint.jar
java.lang.NoClassDefFoundError: com/android/tools/lint/detector/api/Detector$UastScanner
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:760)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:760)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at net.grandcentrix.thirtyinch.IssueRegistry.getIssues(IssueRegistry.kt:6)
        at com.android.tools.lint.client.api.JarFileIssueRegistry.<init>(JarFileIssueRegistry.java:125)
        at com.android.tools.lint.client.api.JarFileIssueRegistry.get(JarFileIssueRegistry.java:90)
        at com.android.tools.lint.client.api.LintDriver.registerCustomDetectors(LintDriver.java:603)
        at com.android.tools.lint.client.api.LintDriver.analyze(LintDriver.java:521)
        at com.android.tools.lint.client.api.LintDriver.analyze(LintDriver.java:483)
        at com.android.tools.lint.LintCliClient.run(LintCliClient.java:143)
        at com.android.build.gradle.internal.LintGradleClient.run(LintGradleClient.java:197)
        at com.android.build.gradle.tasks.Lint.runLint(Lint.java:340)
        at com.android.build.gradle.tasks.Lint.lintSingleVariant(Lint.java:307)
        at com.android.build.gradle.tasks.Lint.lint(Lint.java:119)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:73)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.doExecute(DefaultTaskClassInfoStore.java:141)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:134)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:123)
        at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:632)
        at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:615)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:95)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:76)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:55)
        at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:62)
        at org.gradle.api.internal.tasks.execution.ValidatingTaskExecuter.execute(ValidatingTaskExecuter.java:58)
        at org.gradle.api.internal.tasks.execution.SkipEmptySourceFilesTaskExecuter.execute(SkipEmptySourceFilesTaskExecuter.java:88)
        at org.gradle.api.internal.tasks.execution.ResolveTaskArtifactStateTaskExecuter.execute(ResolveTaskArtifactStateTaskExecuter.java:46)
        at org.gradle.api.internal.tasks.execution.SkipTaskWithNoActionsExecuter.execute(SkipTaskWithNoActionsExecuter.java:51)
        at org.gradle.api.internal.tasks.execution.SkipOnlyIfTaskExecuter.execute(SkipOnlyIfTaskExecuter.java:54)
        at org.gradle.api.internal.tasks.execution.ExecuteAtMostOnceTaskExecuter.execute(ExecuteAtMostOnceTaskExecuter.java:43)
        at org.gradle.api.internal.tasks.execution.CatchExceptionTaskExecuter.execute(CatchExceptionTaskExecuter.java:34)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker$1.execute(DefaultTaskGraphExecuter.java:236)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker$1.execute(DefaultTaskGraphExecuter.java:228)
        at org.gradle.internal.Transformers$4.transform(Transformers.java:169)
        at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:106)
        at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:61)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:228)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:215)
        at org.gradle.execution.taskgraph.AbstractTaskPlanExecutor$TaskExecutorWorker.processTask(AbstractTaskPlanExecutor.java:77)
        at org.gradle.execution.taskgraph.AbstractTaskPlanExecutor$TaskExecutorWorker.run(AbstractTaskPlanExecutor.java:58)
        at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor.process(DefaultTaskPlanExecutor.java:32)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter.execute(DefaultTaskGraphExecuter.java:113)
        at org.gradle.execution.SelectedTaskExecutionAction.execute(SelectedTaskExecutionAction.java:37)
        at org.gradle.execution.DefaultBuildExecuter.execute(DefaultBuildExecuter.java:37)
        at org.gradle.execution.DefaultBuildExecuter.access$000(DefaultBuildExecuter.java:23)
        at org.gradle.execution.DefaultBuildExecuter$1.proceed(DefaultBuildExecuter.java:43)
        at org.gradle.execution.DryRunBuildExecutionAction.execute(DryRunBuildExecutionAction.java:32)
        at org.gradle.execution.DefaultBuildExecuter.execute(DefaultBuildExecuter.java:37)
        at org.gradle.execution.DefaultBuildExecuter.execute(DefaultBuildExecuter.java:30)
        at org.gradle.initialization.DefaultGradleLauncher$3.execute(DefaultGradleLauncher.java:196)
        at org.gradle.initialization.DefaultGradleLauncher$3.execute(DefaultGradleLauncher.java:193)
        at org.gradle.internal.Transformers$4.transform(Transformers.java:169)
        at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:106)
        at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:56)
        at org.gradle.initialization.DefaultGradleLauncher.doBuildStages(DefaultGradleLauncher.java:193)
        at org.gradle.initialization.DefaultGradleLauncher.doBuild(DefaultGradleLauncher.java:119)
        at org.gradle.initialization.DefaultGradleLauncher.run(DefaultGradleLauncher.java:102)
        at org.gradle.launcher.exec.GradleBuildController.run(GradleBuildController.java:71)
        at org.gradle.tooling.internal.provider.ExecuteBuildActionRunner.run(ExecuteBuildActionRunner.java:28)
        at org.gradle.launcher.exec.ChainingBuildActionRunner.run(ChainingBuildActionRunner.java:35)
        at org.gradle.launcher.exec.InProcessBuildActionExecuter.execute(InProcessBuildActionExecuter.java:41)
        at org.gradle.launcher.exec.InProcessBuildActionExecuter.execute(InProcessBuildActionExecuter.java:26)
        at org.gradle.tooling.internal.provider.ContinuousBuildActionExecuter.execute(ContinuousBuildActionExecuter.java:75)
        at org.gradle.tooling.internal.provider.ContinuousBuildActionExecuter.execute(ContinuousBuildActionExecuter.java:49)
        at org.gradle.tooling.internal.provider.ServicesSetupBuildActionExecuter.execute(ServicesSetupBuildActionExecuter.java:44)
        at org.gradle.tooling.internal.provider.ServicesSetupBuildActionExecuter.execute(ServicesSetupBuildActionExecuter.java:29)
        at org.gradle.launcher.daemon.server.exec.ExecuteBuild.doBuild(ExecuteBuild.java:67)
        at org.gradle.launcher.daemon.server.exec.BuildCommandOnly.execute(BuildCommandOnly.java:36)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.WatchForDisconnection.execute(WatchForDisconnection.java:47)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.ResetDeprecationLogger.execute(ResetDeprecationLogger.java:26)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.RequestStopIfSingleUsedDaemon.execute(RequestStopIfSingleUsedDaemon.java:34)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.ForwardClientInput$2.call(ForwardClientInput.java:74)
        at org.gradle.launcher.daemon.server.exec.ForwardClientInput$2.call(ForwardClientInput.java:72)
        at org.gradle.util.Swapper.swap(Swapper.java:38)
        at org.gradle.launcher.daemon.server.exec.ForwardClientInput.execute(ForwardClientInput.java:72)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.LogAndCheckHealth.execute(LogAndCheckHealth.java:55)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.LogToClient.doBuild(LogToClient.java:60)
        at org.gradle.launcher.daemon.server.exec.BuildCommandOnly.execute(BuildCommandOnly.java:36)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.EstablishBuildEnvironment.doBuild(EstablishBuildEnvironment.java:72)
        at org.gradle.launcher.daemon.server.exec.BuildCommandOnly.execute(BuildCommandOnly.java:36)
        at org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
        at org.gradle.launcher.daemon.server.exec.StartBuildOrRespondWithBusy$1.run(StartBuildOrRespondWithBusy.java:50)
        at org.gradle.launcher.daemon.server.DaemonStateCoordinator$1.run(DaemonStateCoordinator.java:297)
        at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
        at org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.ClassNotFoundException: com.android.tools.lint.detector.api.Detector$UastScanner
        at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        ... 118 more

I don't know if that is also the reason why it doesn't work? :)

bildschirmfoto 2017-08-04 um 13 51 29

}

repositories {
maven { url "https://dl.bintray.com/android/android-tools" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like such "special cases".
But just checked. lint-tests isn't published to jcenter 😞 .
However. Would be great if we add this information to it.. So we can hopefully remove it if its published.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I agree that it's unfortunate that we have to include the additional repository, just so we can access Lint-specific dependencies. A comment and, potentially, a link to an issue over at b.android.com would be helpful here!

@mannodermaus
Copy link
Author

Unsure about the NoClassDefFoundError, but it seems like the bundled Lint API in that project is running on an older (and most likely, stable) version, either because of the IDE itself or the Gradle plugin. What versions of AS and the Android Gradle Plugin are you experiencing that on?

@StefMa
Copy link
Contributor

StefMa commented Aug 4, 2017

I am not on my work macbook anymore. So I don't know it exactly. But I use the latest stable of android studio 2.3.3 I think 🤔?!
And I use the same gradle plugin as you do since it is defined in the build.gradle ☺️

Sent from my Google Nexus 5X using FastHub

@StefMa
Copy link
Contributor

StefMa commented Sep 12, 2017

@aurae
I've tried to it again with AS 3.0-Beta5.
I don't get the NoClassDefFoundError anymore.

But unfortunately I don't see the lint warning.
Which I've already mentioned here #111 (review)

I've also tested it with AS 3.0-Beta5 with the new plugin (3.0-Beta5 of course :)):
And it doesn't work...

@passsy
Copy link
Contributor

passsy commented Sep 12, 2017

Also interesting: googlesamples/android-custom-lint-rules#10

@mannodermaus
Copy link
Author

Great find @passsy, let's keep an eye on that.

Unfortunately, I'm currently unable to attend to this PR myself, so I'm unsure as to why the rule wouldn't trigger in its current state. I'm sure we will be able to fix that soon, though!

jreehuis pushed a commit to jreehuis/ThirtyInch that referenced this pull request Aug 27, 2018
@jreehuis jreehuis mentioned this pull request Aug 27, 2018
@mannodermaus
Copy link
Author

Superseded by #158

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants