Migrate from lombok API to Psi APIs #227

eyedol · 2016-12-03T03:54:13Z

This fixes #226

Bump lint library version.
Migrate CallNeedsPermissionDector to use the new Psi APIs.
Migrate NoCorrespondingNeedsPermissionDector to the new Psi APIs.
Migrate CallOnRequestPermissionsResultDetector to the new Psi APIs.
Add test for CallNeedsPermissionDector migration.
Add test for NoCorrespondingNeedsPermissionDector migration.
Add test for CallOnRequestPermissionsResultDetector migration.

@hotchemi for a review before I cause enough mess :-)

eyedol · 2016-12-03T04:00:57Z

I'm getting an error when I run lint sample on the command line. I'm suspecting a bug in the lint tool. For some reason the packaged lint.jar is missing the Psi packages. This issue made me open this initial PR so I can be pointed in the right direction if I'm doing something obviously wrong.

Could not load custom rule jar file ~/.android/lint/lint.jar
java.lang.NoClassDefFoundError: com/android/tools/lint/detector/api/Detector$JavaPsiScanner
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
	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 permissions.dispatcher.PermissionsDispatcherIssueRegistry.getIssues(PermissionsDispatcherIssueRegistry.java:13)
	at com.android.tools.lint.client.api.JarFileIssueRegistry.<init>(JarFileIssueRegistry.java:104)
	at com.android.tools.lint.client.api.JarFileIssueRegistry.get(JarFileIssueRegistry.java:80)
	at com.android.tools.lint.client.api.LintDriver.registerCustomDetectors(LintDriver.java:481)
	at com.android.tools.lint.client.api.LintDriver.analyze(LintDriver.java:415)
	at com.android.tools.lint.client.api.LintDriver.analyze(LintDriver.java:377)
	at com.android.tools.lint.LintCliClient.run(LintCliClient.java:131)
	at com.android.tools.lint.Main.run(Main.java:674)
	at com.android.tools.lint.Main.main(Main.java:118)
Caused by: java.lang.ClassNotFoundException: com.android.tools.lint.detector.api.Detector$JavaPsiScanner
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 20 more

hotchemi · 2016-12-04T22:35:43Z

@eyedol Thx! Let me take a look.

hotchemi · 2016-12-04T22:36:38Z

Is this still a WIP? I dunno the reason but seems ci had passed.

hotchemi · 2016-12-04T22:48:34Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

+        return Collections.<Class<? extends PsiElement>>singletonList(PsiClass.class);
+    }
+
+    public CallNeedsPermissionDetector() {


Is this constructor needed?

Yeah. It's being used by its corresponding test class. Plus the documentation on writing a lint-check recommends adding it.

eyedol · 2016-12-05T03:11:45Z

@hotchemi Yes still a WIP. I'm yet to migrate CallOnRequestPermissionsResultDetector class. I see that it implements Detector.ClassScanner and I haven't seen its equivalent for Psi. Any reason why you're scanning the class file instead of the Java file? I want to migrate it to Detector.JavaPsiScanner and want to justify if it's okay.

shiraji · 2016-12-05T04:21:28Z

#131

I'm not sure the new api works well or not. But when I used JavaScanner, there are many reports the lint produced red lines. The problem is when I implemented the lint, it does not happen...

hotchemi · 2016-12-05T08:02:54Z

@eyedol Thx. Can you continue to convert CallOnRequestPermissionsResultDetector?

eyedol · 2016-12-05T12:03:48Z

@shiraji Thanks for the comment. I see. It's more to circumvent around an issue then. I'll use the Detector.JavaPsiScanner class then.

@hotchemi Yeah, I'll migrate it. I won't be able to get to it until tomorrow though.

hotchemi · 2016-12-05T22:53:56Z

Got it, Thx!

felipecsl · 2016-12-06T00:49:37Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java


    static List<String> generatedClassNames = new ArrayList<String>();

+    static List<String> METHODS = Collections.emptyList();


private static final

Well, it's a little bit not intuitive to add something to final list for me, so how about make this variable being just static and rename like normal variable?

if you omit the getApplicableMethodNames() method, then you don't need this at all

felipecsl · 2016-12-06T00:50:17Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

-    private static class AnnotationChecker extends ForwardingAstVisitor {
+    @Override
+    public List<String> getApplicableMethodNames() {
+        return METHODS;


if you never plan to add explicit method names here, it's safe to omit this method

Good to know.

felipecsl · 2016-12-06T00:51:15Z

lint/src/main/java/permissions/dispatcher/NoCorrespondingNeedsPermissionDetector.java

+
+        private Set<PsiAnnotation> needsPermissionAnnotations;
+
+        private Set<PsiAnnotation> onShowRationaleAnnotations;


felipecsl · 2016-12-06T00:52:00Z

lint/src/test/java/permissions/dispatcher/CallNeedsPermissionDetectorTest.java

+        baz.append("public class Baz {\n");
+        baz.append("public void fooBar() {\n");
+        baz.append("}\n");
+        baz.append("}");


there's no need to use a StringBuilder here. Just concatenate strings directly

It was a bit messy concatenating them together using + because of the code formatter.

felipecsl · 2016-12-06T00:54:06Z

build.gradle

+        // Needed for lint-tests and testutils 25.2.0 packages
+        maven {
+            url 'https://oss.sonatype.org/content/repositories/releases/'
+        }


use mavenCentral() instead

felipecsl · 2016-12-06T00:55:39Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

+            if (classes.length > 0 && classes[0].getName() != null) {
+                String qualifiedName = classes[0].getName();
+                if (qualifiedName.contains("PermissionsDispatcher") || qualifiedName
+                        .contains("PermissionsDispatcher")) {


you got the exact same check twice in this if statement

forgot to remove this duplicated check?

hotchemi · 2016-12-06T02:23:56Z

lint/build.gradle

@@ -24,3 +24,8 @@ jar {
        attributes("Lint-Registry": "permissions.dispatcher.PermissionsDispatcherIssueRegistry")
    }
 }
+
+// Enable test build status on stdout
+tasks.matching { it instanceof Test }.all {


Is this needed?

hotchemi · 2016-12-06T02:25:48Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java


    static List<String> generatedClassNames = new ArrayList<String>();

+    static List<String> METHODS = Collections.emptyList();


Well, it's a little bit not intuitive to add something to final list for me, so how about make this variable being just static and rename like normal variable?

hotchemi · 2016-12-06T02:26:05Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

+        return Collections.<Class<? extends PsiElement>>singletonList(PsiClass.class);
+    }
+
+    public CallNeedsPermissionDetector() {


eyedol · 2016-12-06T12:42:41Z

Ported the last detector class. I'll appreciate a review @shiraji @hotchemi @felipecsl

hotchemi

Thx for your great work!

hotchemi · 2016-12-06T16:30:54Z

build.gradle

@@ -14,5 +14,7 @@ buildscript {
 allprojects {
    repositories {
        jcenter()
+        // Needed for lint-tests and testutils 25.2.0 packages


This line is not needed:D

I agree :-D

hotchemi · 2016-12-06T16:31:18Z

library/build.gradle

@@ -16,6 +16,10 @@ android {
        targetCompatibility JavaVersion.VERSION_1_6
    }

+    lintOptions {
+        abortOnError false


Why this line is needed?

yeah this should definitely be removed :)

@hotchemi It was complaining earlier from a broken build. I had to set it as a workaround. It slid through by mistake. Removed.

hotchemi · 2016-12-06T17:10:26Z

lint/src/test/java/permissions/dispatcher/CallOnRequestPermissionsResultDetectorTest.java

+    @Override
+    protected List<Issue> getIssues() {
+        return ImmutableList.of(CallOnRequestPermissionsResultDetector.ISSUE);
+


Please remove this line.

It's an abstract method from LintDetectorTest that needs to be implemented.

hotchemi · 2016-12-06T17:14:52Z

lint/src/main/java/permissions/dispatcher/CallOnRequestPermissionsResultDetector.java

-            if (hasRuntimePermissionAnnotation && !hasOnRequestPermissionResultCall) {
-                context.report(ISSUE, context.getLocation(classDeclaration), "Generated onRequestPermissionsResult method not called");
+    private static boolean checkMethodCall(PsiMethod method, PsiClass psiClass) {
+        // FIXME: I'm sure there is a better way of checking if the on onRequestPermissionsResult


Any idea for this?

felipecsl

There's a bunch of comments that haven't been addressed yet, but making progress. Good work 👍

felipecsl · 2016-12-06T18:56:24Z

library/build.gradle

@@ -16,6 +16,10 @@ android {
        targetCompatibility JavaVersion.VERSION_1_6
    }

+    lintOptions {
+        abortOnError false


yeah this should definitely be removed :)

felipecsl · 2016-12-06T18:57:17Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

        private final JavaContext context;
+
        private Set<String> matchingAnnotationTypeNames;


i know you didnt change this line, but while you're at it, make it final

eyedol · 2016-12-06T22:38:54Z

Thanks guys. I have fixed the comments.

felipecsl

Still some stuff missing

felipecsl · 2016-12-06T22:47:21Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

-    private static class AnnotationChecker extends ForwardingAstVisitor {
+    @Override
+    public List<String> getApplicableMethodNames() {
+        return methods;


this still can be omitted

If I remove test breaks.

ok never mind then

felipecsl · 2016-12-06T22:47:52Z

lint/src/main/java/permissions/dispatcher/CallNeedsPermissionDetector.java

+            if (classes.length > 0 && classes[0].getName() != null) {
+                String qualifiedName = classes[0].getName();
+                if (qualifiedName.contains("PermissionsDispatcher") || qualifiedName
+                        .contains("PermissionsDispatcher")) {


forgot to remove this duplicated check?

- Bump lint library version - Migrate `CallNeedsPermissionDector` to the new Psi API - Migrate `NoCorrespondingNeedsPermissionDector` to the new Psi API - Add maven repository for needed depenencies for test compiles - Add test for the migrations - Show test status on the console. Useful on CI - Remove @VisibleOnTesting annotation detector default constructors. - Hand create mock classes in the test class instead of loading them from a separate package - Migrate `CallOnRequestPermissionsResultDetector` to the new Psi API - Upgrade android gradle plugin - Pull dependencies from `mavencentral`

hotchemi · 2016-12-07T02:07:30Z

@eyedol Thx!!! Can I merge?

shiraji · 2016-12-07T02:28:46Z

what library-source.jar for?

eyedol · 2016-12-07T04:48:25Z

@hotchemi It looks good to me. You can merge.

@shiraji good question. It came with my fork. See this

shiraji · 2016-12-07T05:03:56Z

I think it hasn't updated it because there were no changes. LGTM! Thanks for your contribution!

hotchemi · 2016-12-07T05:05:10Z

@shiraji Well, actually we have shameful dependency relationship between each modules so that we have workaround...will be fixed on #27.
ref: https://github.com/hotchemi/PermissionsDispatcher/blob/master/library/build.gradle#L81

hotchemi · 2016-12-07T05:06:52Z

@eyedol Really appreciate for your awesome work!!!!!
@felipecsl Thx for your joining to code review! I'm gonna release new ver and notice tonight(JST):D

felipecsl · 2016-12-07T05:08:05Z

Awesome, thank you very much guys!!

eyedol force-pushed the 226-update-custom-rules-to-new-api branch 2 times, most recently from a6bd02f to 3b620d8 Compare December 4, 2016 12:30

hotchemi reviewed Dec 4, 2016

View reviewed changes

shiraji mentioned this pull request Dec 5, 2016

Update custom lint rules to the new PSI API #226

Closed

eyedol mentioned this pull request Dec 5, 2016

Port lint checks from Lombok to PSI JakeWharton/timber#191

Merged

felipecsl reviewed Dec 6, 2016

View reviewed changes

hotchemi reviewed Dec 6, 2016

View reviewed changes

eyedol force-pushed the 226-update-custom-rules-to-new-api branch from 3b620d8 to df04784 Compare December 6, 2016 12:40

hotchemi reviewed Dec 6, 2016

View reviewed changes

felipecsl reviewed Dec 6, 2016

View reviewed changes

eyedol force-pushed the 226-update-custom-rules-to-new-api branch from df04784 to 7fdcb6f Compare December 6, 2016 22:15

felipecsl reviewed Dec 6, 2016

View reviewed changes

eyedol force-pushed the 226-update-custom-rules-to-new-api branch 2 times, most recently from 8f58687 to 1960b69 Compare December 6, 2016 22:54

eyedol force-pushed the 226-update-custom-rules-to-new-api branch from 1960b69 to 96df703 Compare December 7, 2016 00:58

hotchemi merged commit 263bf45 into permissions-dispatcher:master Dec 7, 2016

seanabraham mentioned this pull request Dec 8, 2016

NeedOnRequestPermissionsResult lint check is firing erroneously #230

Closed


		static List<String> generatedClassNames = new ArrayList<String>();

		static List<String> METHODS = Collections.emptyList();


		private Set<PsiAnnotation> needsPermissionAnnotations;

		private Set<PsiAnnotation> onShowRationaleAnnotations;

		private final JavaContext context;

		private Set<String> matchingAnnotationTypeNames;

Migrate from lombok API to Psi APIs #227

Migrate from lombok API to Psi APIs #227

Conversation

eyedol commented Dec 3, 2016 • edited Loading

eyedol commented Dec 3, 2016

hotchemi commented Dec 4, 2016

hotchemi commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyedol commented Dec 5, 2016

shiraji commented Dec 5, 2016

hotchemi commented Dec 5, 2016

eyedol commented Dec 5, 2016

hotchemi commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyedol commented Dec 6, 2016

hotchemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyedol commented Dec 6, 2016

felipecsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecsl Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotchemi commented Dec 7, 2016

shiraji commented Dec 7, 2016

eyedol commented Dec 7, 2016

shiraji commented Dec 7, 2016

hotchemi commented Dec 7, 2016 • edited Loading

hotchemi commented Dec 7, 2016

felipecsl commented Dec 7, 2016

eyedol commented Dec 3, 2016 •

edited

Loading

felipecsl Dec 6, 2016 •

edited

Loading

hotchemi commented Dec 7, 2016 •

edited

Loading