Avoiding Pitfalls Collaboratively

High-quality software is important for tech companies. It is getting challenged because hundreds of engineers are working on the shared big code base. Unit testing, automation testing and static analysis are very helpful to create error-free code. Static analysis is a technique to detect software errors by analyzing existing code without running.

However, there are pitfalls which cannot be detected by existing tools. Some pitfalls could be related to company coding standards or how to use third-party libraries. For example, string resources should be sorted alphabetically by a string key. This talk will about a way to avoid those pitfalls collaboratively by utilizing Android custom lint, which is a one of static analysis tool. With Android custom lint, developers easily avoid pitfalls with a nice explanation such as which line and why.

Presented at droidcon Tunisia 2016 http://www.droidcon.tn/avoiding-pitfalls-collaboratively/.

Transcript

1. Avoiding Pitfalls Collaboratively Sangsoo Nam sangsoo@spotify.com
2. Experience
3. Backend Data(Json) Java Class { "name" : "Marilyn Monroe" } Jackson public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } }
4. Backend Data(Json) Java Class { "name" : "Marilyn Monroe", "birth_year" : 1926 } Jackson public class Person { public final String name; public final int year; public Person(@JsonProperty("name") String name, @JsonProperty("birth_year") int year) { this.name = name; this.year = year; } }
5. Crash…!?
6. Good! Crash!
7. Backend Data(Json) Java Class { "name" : "Marilyn Monroe", "birth_year" : 1926 } public class Person { public final String name; public final int year; public Person(@JsonProperty("name") String name, @JsonProperty("birth_year") int year) { this.name = name; } } Java Class public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } } Jackson
8. Backend Data(Json) Java Class { "name" : "Marilyn Monroe", "birth_year" : 1926 } public class Person { public final String name; public final int year; public Person(@JsonProperty("name") String name, @JsonProperty("birth_year") int year) { this.name = name; } } Java Class public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } } Jackson UnrecognizedPropertyException
9. Backend Data(Json) Java Class { "name" : "Marilyn Monroe", "birth_year" : 1926 } public class Person { public final String name; public final int year; public Person(@JsonProperty("name") String name, @JsonProperty("birth_year") int year) { this.name = name; } } Java Class @JsonIgnoreProperties(ignoreUnknown = true) public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } } Jackson
10. How to share your knowledge?
11. Experience > Knowledge
12. What if others will see …?
14. Brief Description Location
15. Brief Description Location Explanation
16. Android Custom Lint (Static Code Analysis Tool) ./gradlew lint
17. Agenda • Key Concepts (Issue, Detector and IssueRegistry) • Practical Examples • Tips • Setup & Test
18. Issue, Detector and IssueRegistry
19. Issue A definition of problem you want to show public static Issue ISSUE = Issue.create( "JacksonIgnoreProperties", // ID "Should use @JsonIgnoreProperties(ignoreUnknown = true)", // Brief Description "By default, @JsonIgnoreProperties(ignoreUnknown = false)...", // Explanation Category.CORRECTNESS, // Category 6, // Priority Severity.ERROR, // Severity new Implementation( JacksonIgnorePropertiesDetector.class, // Detector Scope.JAVA_FILE_SCOPE // Scope ) );
20. Responsible to find issues and report Detector ClassScanner JavaScanner XmlScanner Detector …
21. Responsible to find issues and report Detector ClassScanner JavaScanner XmlScanner Java File Scope Class File Scope Manifest File or Resource File Scope
22. IssueRegistry Provides a list of issues public class CustomIssueRegistry extends IssueRegistry { @Override public List<Issue> getIssues() { return Arrays.asList( JacksonIgnorePropertiesJavaDetector.ISSUE ); } }
23. Practical Examples
24. @JsonIgnoreProperties
25. We want to avoid public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } } We don’t care public class Person { public final String name; public Person(String name) { this.name = name; } }
26. We expect @JsonIgnoreProperties(ignoreUnknown = true) public class Person { public final String name; public Person(@JsonProperty("name") String name) { this.name = name; } } @JsonProperty ignoreUnknown = true Constructor
27. Issue with Java Scope public static Issue ISSUE = Issue.create( "JacksonIgnoreProperties", // ID "Should use @JsonIgnoreProperties(ignoreUnknown = true)", // Brief Description "By default, @JsonIgnoreProperties(ignoreUnknown = false)...", // Explanation Category.CORRECTNESS, // Category 6, // Priority Severity.ERROR, // Severity new Implementation( JacksonIgnorePropertiesDetector.class, // Detector Scope.Java_FILE_SCOPE // Scope ) );
28. Detector with Java Scanner public class JacksonIgnorePropertiesDetector extends Detector implements Detector.JavaScanner { public static Issue ISSUE = {...} @Override public AstVisitor createJavaVisitor(JavaContext context) { return new JavaVisitor(context); } private static class JavaVisitor extends ForwardingAstVisitor { public JavaVisitor(JavaContext context) {...} @Override public boolean visitClassDeclaration(ClassDeclaration node) {...} @Override public boolean visitConstructorDeclaration(ConstructorDeclaration node) {...} } }
29. visitClassDeclaration @Override public boolean visitClassDeclaration(ClassDeclaration node) { for (Annotation annotation : node.astModifiers().astAnnotations()) { if (annotation.astAnnotationTypeReference().getTypeName() .equals("JsonIgnoreProperties")) { for (Node valueNode : annotation.getValues("ignoreUnknown")) { if (valueNode instanceof BooleanLiteral && Objects.equals(true, ((BooleanLiteral) valueNode).astValue())) { mHasJsonIgnorePropertiesAnnotationWithTrueValue = true; return false; } } } } return false; }
31. visitConstructorDeclaration @Override public boolean visitConstructorDeclaration(ConstructorDeclaration node) { boolean hasJsonPropertyAnnotation = false; LoopVariables: for (VariableDefinition variableDefinition : node.astParameters()) { for (Annotation annotation : variableDefinition.astModifiers().astAnnotations()) { if (annotation.astAnnotationTypeReference().getTypeName().equals("JsonProperty")) { hasJsonPropertyAnnotation = true; break LoopVariables; } } } if (hasJsonPropertyAnnotation && !mHasJsonIgnorePropertiesAnnotationWithTrueValue) { mContext.report(ISSUE, mContext.getLocation(node.getParent()), "Should use @JsonIgnoreProperties(ignoreUnknown = true)"); // Message } return true; }
32. @NonNull and @Nullable
33. We want to avoid public class Person { public final String name; public final int age; public Person(@JsonProperty("name") String name, @JsonProperty("age") int age) { this.name = name; this.age = age; } } We don’t care public class Person { public final String name; public final int age; public Person(String name, int age) { this.name = name; this.age = age; } }
34. We expect public class Person { public final String name; public final int age; public Person(@NonNull @JsonProperty("name") String name, @JsonProperty("age") int age) { this.name = name; this.age = age; } } Use @NonNull or @Nullable Don’t care a primitive type
35. Issue with Java Scope public static Issue ISSUE = Issue.create( "JacksonConstructorWithNonNullOrNullable", "Should use @NotNull or @Nullable annotation", "Without @NonNull or @Nullable annotations, it is not obvious to say a value is " + "not null or nullable. It could make any wrong assumption. To avoid that, " + "annotations should be used", Category.CORRECTNESS, 6, Severity.ERROR, new Implementation( JacksonConstructorNonNullJavaDetector.class, Scope.JAVA_FILE_SCOPE ) );
36. Detector with Java Scanner private static class JavaVisitor extends ForwardingAstVisitor { @Override public boolean visitConstructorDeclaration(ConstructorDeclaration node) { for (VariableDefinition variableDefinition : node.astParameters()) { if (variableDefinition.astTypeReference().isPrimitive()) { // Skip primitive value continue; } Set<String> typeNames = new HashSet<>(); for (Annotation annotation : variableDefinition.astModifiers().astAnnotations()) { typeNames.add(annotation.astAnnotationTypeReference().getTypeName()); } if (typeNames.contains("JsonProperty")) { if (typeNames.contains("NonNull") || typeNames.contains("Nullable")) { continue; } mContext.report(ISSUE, mContext.getLocation(variableDefinition), "Should use @NotNull or @Nullable annotation"); } } return false; } }
37. Register and Unregister
38. FollowService.java public class FollowService { private Set<Listener> mListeners = new HashSet<>(); public void registerListener(Listener listener) { mListeners.add(listener); } public void unregisterListener(Listener listener) { mListeners.remove(listener); } ... }
39. We want to avoid public class FollowFragment extends Fragment{ private FollowService mFollowService; ... @Override public void onStart() { super.onStart(); mFollowService.registerListener(mListener); } @Override public void onStop() { super.onStop(); } }
40. We expect public class FollowFragment extends Fragment{ private FollowService mFollowService; ... @Override public void onStart() { super.onStart(); mFollowService.registerListener(mListener); } @Override public void onStop() { super.onStop(); mFollowService.unregisterListener(mListener); } } register unregister
41. Issue with Class Scope public static Issue ISSUE = Issue.create( "RegisterUnregister", "Should call registerListener() and unregisterListener() in the same class.", "To avoid memory leak, the class which call registerListener() should also call " + "unregisterListener(). vice versa.", Category.CORRECTNESS, 6, Severity.ERROR, new Implementation( RegisterUnregisterClassDetector.class, Scope.CLASS_FILE_SCOPE ) );
42. We expect public class FollowFragment extends Fragment{ private FollowService mFollowService; ... @Override public void onStart() { super.onStart(); mFollowService.registerListener(mListener); } @Override public void onStop() { super.onStop(); mFollowService.unregisterListener(mListener); } } Call Name Call Owner
43. Detector with Class Scanner public class RegisterUnregisterClassDetector extends Detector implements Detector.ClassScanner { public static Issue ISSUE = {...} private boolean mIsRegisterCalled; private boolean mIsUnregisterCalled; @Override public List<String> getApplicableCallNames() { return Lists.newArrayList("registerListener", "unregisterListener"); } @Override public List<String> getApplicableCallOwners() { return Lists.newArrayList("registerunregister/FollowService"); } @Override public void checkCall(ClassContext context, ClassNode cn, MethodNode m, MethodInsnNode call) { if (call.owner.equals("registerunregister/FollowService")) { if (call.name.equals("registerListener")) { mIsRegisterCalled = true; } else if (call.name.equals("unregisterListener")) { mIsUnregisterCalled = true; } } }
44. Detector with Class Scanner @Override public void afterCheckFile(Context context) { super.afterCheckFile(context); ClassContext classContext = (ClassContext) context; // If one is false. if (mIsRegisterCalled ^ mIsUnregisterCalled) { if (!mIsRegisterCalled) { context.report(ISSUE, classContext.getLocation(classContext.getClassNode()), "registerListener() should be called in the same class."); } else { context.report(ISSUE, classContext.getLocation(classContext.getClassNode()), "unregisterListener() should be called in the same class."); } } } }
45. String Resource Sorting by Name
46. We want to avoid <?xml version="1.0" encoding="utf-8"?> <resources> <string name="welcome_title">Welcome</string> <string name="hello">Hello!</string> <plurals name="songs"> <item quantity="one">%d song</item> <item quantity="other">%d songs</item> </plurals> </resources>
47. We expect <?xml version="1.0" encoding="utf-8"?> <resources> <string name="hello">Hello!</string> <plurals name="songs"> <item quantity="one">%d song</item> <item quantity="other">%d songs</item> </plurals> <string name="welcome_title">Welcome</string> </resources> Sorted by Name: Alphabetically
48. Issue with Resource Scope public static Issue ISSUE = Issue.create( "StringsSortedByName", "Strings should be ordered by name.", "To keep consistency, strings should be ordered by name.", Category.CORRECTNESS, 6, Severity.ERROR, new Implementation( StringsSortedByNameResourceDetector.class, Scope.RESOURCE_FILE_SCOPE ) );
49. Detector with Xml Scanner public class StringsSortedByNameResourceDetector extends Detector implements Detector.XmlScanner { public static Issue ISSUE = {...} @Override public boolean appliesTo(ResourceFolderType folderType) { return folderType == ResourceFolderType.VALUES; } @Override public Collection<String> getApplicableElements() { return Arrays.asList(SdkConstants.TAG_PLURALS, SdkConstants.TAG_STRING, SdkConstants.TAG_STRING_ARRAY); } @Override public void visitElement(XmlContext context, Element element) {...} }
50. visitElement @Override public void visitElement(XmlContext context, Element element) { Node node = element.getNextSibling(); while (node != null && node.getNodeType() != Node.ELEMENT_NODE) { node = node.getNextSibling(); } if (node == null) { // No next element to compare return; } Element nextElement = (Element) node; String name = element.getAttribute("name"); String nextName = nextElement.getAttribute("name"); if (name.compareTo(nextName) > 0) { context.report(ISSUE, context.getLocation(element), "Resource element name (" + nextName + ") must be sorted after (" + name + ")."); } }
51. Tips
52. Source Code Best documents you can use @Beta public abstract class Detector { /** Specialized interface for detectors that scan Java source file parse trees */ public interface JavaScanner { /** * Create a parse tree visitor to process the parse tree. All * {@link JavaScanner} detectors must provide a visitor, unless they * either return true from {@link #appliesToResourceRefs()} or return * non null from {@link #getApplicableMethodNames()} .... * @param context the {@link Context} for the file being analyzed * @return a visitor, or null. */ @Nullable AstVisitor createJavaVisitor(@NonNull JavaContext context);
53. Source Code Some methods should be overridden to work properly public interface ClassScanner { void checkClass(@NonNull ClassContext context, @NonNull ClassNode classNode); int[] getApplicableAsmNodeTypes(); void checkInstruction(@NonNull ClassContext context, @NonNull ClassNode classNode, @NonNull MethodNode method, @NonNull AbstractInsnNode instruction); List<String> getApplicableCallNames(); List<String> getApplicableCallOwners(); void checkCall(@NonNull ClassContext context, @NonNull ClassNode classNode, @NonNull MethodNode method, @NonNull MethodInsnNode call); }
54. Use Debugger The best way to check the value and to know how to access
55. Severity.ERROR People tend to ignore warnings public static Issue ISSUE = Issue.create( "JacksonIgnoreProperties", // ID "Should use @JsonIgnoreProperties(ignoreUnknown = true)", // Brief Description "By default, @JsonIgnoreProperties(ignoreUnknown = false)..." + // Explanation Category.CORRECTNESS, // Category 6, // Priority Severity.ERROR, // Severity new Implementation( JacksonIgnorePropertiesDetector.class, // Detector Scope.Java_FILE_SCOPE // Scope ) );
56. Setup & Tests
57. Github Repository https:/ /github.com/SangsooNam/CustomAndroidLint
58. lint.jar jar { archiveName 'lint.jar' manifest { attributes("Lint-Registry": "io.github.sangsoonam.lint.CustomIssueRegistry") } } Build a jar file having lint registry attribute
59. CopyLintJar configurations { customLint } task copyLintJar(type: Copy) { from (configurations.customLint) into "$buildDir/lint" } project.afterEvaluate { it.tasks.compileLint.dependsOn copyLintJar } dependencies { customLint project(path:':customlint', configuration: 'archives') } Copied lint.jar is used while ./gradlew lint
60. Sample Module For Testing Where can you get class files for testing? project.afterEvaluate { tasks.compileTestJava.dependsOn ':customlint:sample:assemble' }
61. Conclusion
62. Android Custom Lint Static Code Analysis Tool Detector (JavaScanner) Issue (Java File Scope) Issue Registry Detector (ClassScanner) Issue (Class File Scope) Detector (XmlScanner) Issue (Resource File Scope)
64. Brief Description Location
65. Brief Description Location Explanation
66. Questions Sangsoo Nam sangsoo@spotify.com Join the band! www.spotify.com/jobs
67. Let’s avoid pitfalls together