Skylint: add main linter class and clean up BUILD files

RELNOTES: none
PiperOrigin-RevId: 167846717

src/tools/skylark/java/com/google/devtools/skylark/skylint/BUILD

@@ -3,20 +3,21 @@
 
 java_binary(
     name = "Skylint",
-    srcs = glob(["**/*.java"]),
+    srcs = [],
     main_class = "com.google.devtools.skylark.skylint.Skylint",
-    deps = [
-        ":skylint-lib",
-        "//src/main/java/com/google/devtools/build/lib:packages",
+    runtime_deps = [
+        ":skylint_lib",
     ],
 )
 
 java_library(
-    name = "skylint-lib",
+    name = "skylint_lib",
     srcs = glob(["**/*.java"]),
     visibility = ["//src/tools/skylark/javatests/com/google/devtools/skylark/skylint:__pkg__"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:packages",
+        "//src/main/java/com/google/devtools/build/lib:skylarkinterface",
+        "//third_party:guava",
     ],
 )
 

src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java

@@ -0,0 +1,89 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.skylark.skylint;
+
+import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Main class of the linter library.
+ *
+ * <p>Most users of the linter library should only need to use this class.
+ */
+public class Linter {
+
+  /** List of all checkers that the linter runs. */
+  private static final Checker[] checkers = {
+    ControlFlowChecker::check,
+    DocstringChecker::check,
+    NamingConventionsChecker::check,
+    StatementWithoutEffectChecker::check,
+    UsageChecker::check
+  };
+
+  /** Function to read files (can be changed for testing). */
+  private FileContentsReader fileReader = Files::readAllBytes;
+
+  public Linter setFileContentsReader(FileContentsReader reader) {
+    this.fileReader = reader;
+    return this;
+  }
+
+  /**
+   * Runs all checkers on the given file.
+   *
+   * @param path path of the file
+   * @return list of issues found in that file
+   */
+  public List<Issue> lint(Path path) throws IOException {
+    String content = new String(fileReader.read(path), StandardCharsets.ISO_8859_1);
+    List<Issue> issues = new ArrayList<>();
+    BuildFileAST ast =
+        BuildFileAST.parseString(
+            event -> {
+              if (event.getKind() == EventKind.ERROR || event.getKind() == EventKind.WARNING) {
+                issues.add(new Issue(event.getMessage(), event.getLocation()));
+              }
+            },
+            content);
+    for (Checker checker : checkers) {
+      issues.addAll(checker.check(ast));
+    }
+    issues.sort(Issue::compare);
+    return issues;
+  }
+
+  /**
+   * Interface with a function that reads a file.
+   *
+   * <p>This is useful because we can use a fake for testing.
+   */
+  @FunctionalInterface
+  public interface FileContentsReader {
+    byte[] read(Path path) throws IOException;
+  }
+
+  /** A checker analyzes an AST and produces a list of issues. */
+  @FunctionalInterface
+  public interface Checker {
+    List<Issue> check(BuildFileAST ast);
+  }
+}

src/tools/skylark/java/com/google/devtools/skylark/skylint/Skylint.java

@@ -14,33 +14,16 @@
 
 package com.google.devtools.skylark.skylint;
 
-import com.google.devtools.build.lib.syntax.BuildFileAST;
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.ArrayList;
 import java.util.List;
 
 /** The main class for the skylint binary. */
 public class Skylint {
   public static void main(String[] args) throws IOException {
     Path path = Paths.get(args[0]).toAbsolutePath();
-    String content = new String(Files.readAllBytes(path), StandardCharsets.ISO_8859_1);
-    BuildFileAST ast =
-        BuildFileAST.parseString(
-            event -> {
-              System.err.println(event);
-            },
-            content);
-    List<Issue> issues = new ArrayList<>();
-    issues.addAll(NamingConventionsChecker.check(ast));
-    issues.addAll(ControlFlowChecker.check(ast));
-    issues.addAll(StatementWithoutEffectChecker.check(ast));
-    issues.addAll(UsageChecker.check(ast));
-    issues.addAll(DocstringChecker.check(ast));
-    issues.sort(Issue::compare);
+    List<Issue> issues = new Linter().lint(path);
     if (!issues.isEmpty()) {
       System.out.println(path);
       for (Issue issue : issues) {

src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BUILD

@@ -7,7 +7,7 @@ java_test(
     deps = [
         "//src/main/java/com/google/devtools/build/lib:packages",
         "//src/test/java/com/google/devtools/build/lib:testutil",
-        "//src/tools/skylark/java/com/google/devtools/skylark/skylint:skylint-lib",
+        "//src/tools/skylark/java/com/google/devtools/skylark/skylint:skylint_lib",
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",

src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java

@@ -0,0 +1,49 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.skylark.skylint;
+
+import com.google.common.truth.Truth;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link com.google.devtools.skylark.skylint.Linter}. */
+@RunWith(JUnit4.class)
+public class LinterTest {
+  @Test
+  public void testAllCheckersAreRun() throws Exception {
+    final String file =
+        String.join(
+            "\n",
+            "_unusedVar = function()",
+            "def function():",
+            "  return",
+            "  'unreachable and unused string literal'");
+    final String errorMessages =
+        new Linter()
+            .setFileContentsReader(p -> file.getBytes(StandardCharsets.ISO_8859_1))
+            .lint(Paths.get("foo"))
+            .toString();
+    Truth.assertThat(errorMessages).contains("unreachable statement"); // control flow checker
+    Truth.assertThat(errorMessages).contains("has no module docstring"); // docstring checker
+    Truth.assertThat(errorMessages)
+        .contains("should be lower_snake_case"); // naming conventions checker
+    Truth.assertThat(errorMessages)
+        .contains("expression result not used"); // checker statements without effect
+    Truth.assertThat(errorMessages).contains("unused definition of"); // usage checker
+  }
+}