Skylint: also check for deprecations in dependencies.

RELNOTES: none
PiperOrigin-RevId: 173658526

3 files changed, 356 insertions, 25 deletions

diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java
new file mode 100644
index 0000000000..674c1241a7
--- /dev/null
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DependencyAnalyzerTest.java
@@ -0,0 +1,237 @@
+// 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.collect.ImmutableMap;
+import com.google.common.truth.Truth;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.build.lib.syntax.FunctionDefStatement;
+import com.google.devtools.build.lib.syntax.LoadStatement;
+import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.skylark.skylint.DependencyAnalyzer.DependencyCollector;
+import com.google.devtools.skylark.skylint.Linter.FileFacade;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.NoSuchFileException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link com.google.devtools.skylark.skylint.DependencyAnalyzer} */
+@RunWith(JUnit4.class)
+public class DependencyAnalyzerTest {
+  private static final DependencyCollector<List<String>> functionCollector =
+      new DependencyCollector<List<String>>() {
+        @Override
+        public List<String> initInfo(Path path) {
+          return new ArrayList<>();
+        }
+
+        @Override
+        public List<String> loadDependency(
+            List<String> currentFileInfo,
+            LoadStatement stmt,
+            Path loadedPath,
+            List<String> loadedFileInfo) {
+          for (String name : loadedFileInfo) {
+            currentFileInfo.add(loadedPath + " - " + name);
+          }
+          return currentFileInfo;
+        }
+
+        @Override
+        public List<String> collectInfo(Path path, BuildFileAST ast, List<String> info) {
+          for (Statement stmt : ast.getStatements()) {
+            if (stmt instanceof FunctionDefStatement) {
+              info.add(((FunctionDefStatement) stmt).getIdentifier().getName());
+            }
+          }
+          return info;
+        }
+      };
+
+  public static FileFacade toFileFacade(Map<String, String> files) {
+    return path -> {
+      String contents = files.get(path.toString());
+      if (contents == null) {
+        throw new NoSuchFileException(path.toString());
+      }
+      return contents.getBytes(StandardCharsets.ISO_8859_1);
+    };
+  }
+
+  private static List<String> getFunctionNames(Map<String, String> files, String path) {
+    return new DependencyAnalyzer<>(toFileFacade(files), functionCollector)
+        .collectTransitiveInfo(Paths.get(path));
+  }
+
+  @Test
+  public void externalRepositoryDependency() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load('@some-repo//foo:bar.bzl', 'baz')")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty();
+  }
+
+  @Test
+  public void nonexistentDependency() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':does_not_exist.bzl', 'baz')")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty();
+  }
+
+  @Test
+  public void samePackageDependency() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':bar.bzl', 'baz')\nload('//:bar.bzl', 'baz')")
+            .put("/bar.bzl", "def baz(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Arrays.asList("/bar.bzl - baz", "/bar.bzl - baz"));
+  }
+
+  @Test
+  public void samePackageDependencyWithoutBuildFile() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/test.bzl", "load(':bar.bzl', 'baz')")
+            .put("/bar.bzl", "def baz(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl")).isEmpty();
+  }
+
+  @Test
+  public void subpackageDependency() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load('subpackage:foo.bzl', 'foo')")
+            .put("/subpackage/BUILD", "")
+            .put("/subpackage/foo.bzl", "def foo(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Collections.singletonList("/subpackage/foo.bzl - foo"));
+  }
+
+  @Test
+  public void dependencyInSiblingPackage() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/pkg1/BUILD", "")
+            .put("/pkg1/test.bzl", "load('//pkg2:bar.bzl')")
+            .put("/pkg2/BUILD", "")
+            .put("/pkg2/bar.bzl", "def bar(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/pkg1/test.bzl"))
+        .isEqualTo(Collections.singletonList("/pkg2/bar.bzl - bar"));
+  }
+
+  @Test
+  public void dependencyInSiblingPackageWithBuildDotBazelFile() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/pkg1/BUILD.bazel", "")
+            .put("/pkg1/test.bzl", "load('//pkg2:bar.bzl')")
+            .put("/pkg2/BUILD.bazel", "")
+            .put("/pkg2/bar.bzl", "def bar(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/pkg1/test.bzl"))
+        .isEqualTo(Collections.singletonList("/pkg2/bar.bzl - bar"));
+  }
+
+  @Test
+  public void dependencyLabelWithoutPrefix() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/test/test.bzl", "load('bar.bzl', 'baz')")
+            .put("/bar.bzl", "def baz(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test/test.bzl"))
+        .isEqualTo(Collections.singletonList("/bar.bzl - baz"));
+  }
+
+  @Test
+  public void transitiveDependencies() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':foo.bzl', 'foo')")
+            .put("/foo.bzl", "load(':bar.bzl', foo = 'bar')")
+            .put("/bar.bzl", "def bar(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Collections.singletonList("/foo.bzl - /bar.bzl - bar"));
+  }
+
+  @Test
+  public void diamondDependencies() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':foo.bzl', 'foo')\nload(':bar.bzl', 'bar')")
+            .put("/foo.bzl", "load(':base.bzl', foo = 'base')")
+            .put("/bar.bzl", "load(':base.bzl', bar = 'base')")
+            .put("/base.bzl", "def base(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Arrays.asList("/foo.bzl - /base.bzl - base", "/bar.bzl - /base.bzl - base"));
+  }
+
+  @Test
+  public void cyclicDependenciesAreHandledGracefully() throws Exception {
+    Map<String, String> files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':test.bzl', 'foo')\ndef test(): pass")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Collections.singletonList("test"));
+
+    files =
+        ImmutableMap.<String, String>builder()
+            .put("/WORKSPACE", "")
+            .put("/BUILD", "")
+            .put("/test.bzl", "load(':foo.bzl', 'baz')\ndef test(): pass")
+            .put("/foo.bzl", "load(':bar.bzl', 'baz')")
+            .put("/bar.bzl", "load(':foo.bzl', 'baz')")
+            .build();
+    Truth.assertThat(getFunctionNames(files, "/test.bzl"))
+        .isEqualTo(Collections.singletonList("test"));
+  }
+}
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java
index 9fddcd5a46..fffc5bd480 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java
@@ -14,9 +14,16 @@
 
 package com.google.devtools.skylark.skylint;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.truth.Truth;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.skylark.skylint.Linter.FileFacade;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.List;
+import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -24,7 +31,7 @@ import org.junit.runners.JUnit4;
 /** Tests the lint done by {@link DeprecationChecker}. */
 @RunWith(JUnit4.class)
 public class DeprecationCheckerTest {
-  private static List<Issue> findIssues(String... lines) {
+  private static List<Issue> findIssuesIgnoringDependencies(String... lines) {
     String content = String.join("\n", lines);
     BuildFileAST ast =
         BuildFileAST.parseString(
@@ -32,13 +39,14 @@ public class DeprecationCheckerTest {
               throw new IllegalArgumentException(event.getMessage());
             },
             content);
-    return DeprecationChecker.check(ast);
+    return DeprecationChecker.check(
+        Paths.get("/fake"), ast, _path -> content.getBytes(StandardCharsets.ISO_8859_1));
   }
 
   @Test
   public void symbolDeprecatedInSameFile() {
     String errorMessages =
-        findIssues(
+        findIssuesIgnoringDependencies(
                 "def f():",
                 "  g()",
                 "  h()",
@@ -74,7 +82,7 @@ public class DeprecationCheckerTest {
   @Test
   public void deprecatedFunctionAliasing() {
     String errorMessages =
-        findIssues(
+        findIssuesIgnoringDependencies(
                 "def f():",
                 "  h = g",
                 "  h()",
@@ -91,7 +99,7 @@ public class DeprecationCheckerTest {
   @Test
   public void functionNotDeprecated() {
     Truth.assertThat(
-            findIssues(
+            findIssuesIgnoringDependencies(
                 "def f():",
                 "  g()",
                 "def g():",
@@ -104,28 +112,27 @@ public class DeprecationCheckerTest {
   @Test
   public void noWarningsInsideDeprecatedFunctions() {
     Truth.assertThat(
-        findIssues(
-            "def f():",
-            "  '''A deprecated function calling another deprecated function -> no warning",
-            "",
-            "  Deprecated:",
-            "    This function is deprecated.",
-            "  '''",
-            "  g()",
-            "",
-            "def g():",
-            "  '''Another deprecated function",
-            "",
-            "  Deprecated:",
-            "    This function is deprecated.'''"
-        )
-    ).isEmpty();
+            findIssuesIgnoringDependencies(
+                "def f():",
+                "  '''A deprecated function calling another deprecated function -> no warning",
+                "",
+                "  Deprecated:",
+                "    This function is deprecated.",
+                "  '''",
+                "  g()",
+                "",
+                "def g():",
+                "  '''Another deprecated function",
+                "",
+                "  Deprecated:",
+                "    This function is deprecated.'''"))
+        .isEmpty();
   }
 
   @Test
   public void shadowingWorks() {
     Truth.assertThat(
-            findIssues(
+            findIssuesIgnoringDependencies(
                 "def f():",
                 "  bad = good",
                 "  bad()",
@@ -137,4 +144,83 @@ public class DeprecationCheckerTest {
                 "    reason'''"))
         .isEmpty();
   }
+
+  private static final Map<String, String> files =
+      ImmutableMap.<String, String>builder()
+          .put("/WORKSPACE", "")
+          .put("/pkg1/BUILD", "")
+          .put(
+              "/pkg1/foo.bzl",
+              String.join(
+                  "\n",
+                  "load('//pkg2:bar.bzl', 'baz', foo = 'bar') # test aliasing",
+                  "load(':qux.bzl', 'qux') # test package label",
+                  "foo()",
+                  "qux()"))
+          .put(
+              "/pkg1/qux.bzl",
+              String.join(
+                  "\n",
+                  "load(':does_not_exist.bzl', 'foo') # test error handling",
+                  "def qux():",
+                  "  '''qux",
+                  "",
+                  "  Deprecated:",
+                  "    qux is deprecated'''",
+                  "return"))
+          .put("/pkg2/BUILD", "")
+          .put(
+              "/pkg2/bar.bzl",
+              String.join(
+                  "\n",
+                  "load('//pkg2/pkg3:baz.bzl', bar = 'baz') # test aliasing",
+                  "bar()",
+                  "def baz():",
+                  "  '''baz",
+                  "",
+                  "  Deprecated:",
+                  "    baz is deprecated",
+                  "  '''"))
+          .put("/pkg2/pkg3/BUILD", "")
+          .put(
+              "/pkg2/pkg3/baz.bzl",
+              String.join(
+                  "\n",
+                  "def baz():",
+                  "  '''baz",
+                  "",
+                  "  Deprecated:",
+                  "    baz is deprecated",
+                  "  '''"))
+          .build();
+
+  private static final FileFacade testFileFacade = DependencyAnalyzerTest.toFileFacade(files);
+
+  private static List<Issue> findIssuesIncludingDependencies(String pathString) throws IOException {
+    Path path = Paths.get(pathString);
+    return DeprecationChecker.check(path, testFileFacade.readAst(path), testFileFacade);
+  }
+
+  @Test
+  public void deprecatedFunctionsInDirectDependency() throws Exception {
+    String errorMessages = findIssuesIncludingDependencies("/pkg1/foo.bzl").toString();
+    Truth.assertThat(errorMessages)
+        .contains(
+            "4:1-4:3: usage of 'qux' (imported from /pkg1/qux.bzl)"
+                + " is deprecated: qux is deprecated");
+    errorMessages = findIssuesIncludingDependencies("/pkg2/bar.bzl").toString();
+    Truth.assertThat(errorMessages)
+        .contains(
+            "2:1-2:3: usage of 'bar' (imported from /pkg2/pkg3/baz.bzl, named 'baz' there)"
+                + " is deprecated: baz is deprecated");
+  }
+
+  @Test
+  public void deprecatedFunctionsInTransitiveDependencies() throws Exception {
+    String errorMessages = findIssuesIncludingDependencies("/pkg1/foo.bzl").toString();
+    Truth.assertThat(errorMessages)
+        .contains(
+            "3:1-3:3: usage of 'foo' (imported from /pkg2/pkg3/baz.bzl, named 'baz' there)"
+                + " is deprecated: baz is deprecated");
+  }
 }
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java
index 96ff5d47f5..d496ef71b2 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/LinterTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.skylark.skylint;
 
 import com.google.common.truth.Truth;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Paths;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -37,11 +38,18 @@ public class LinterTest {
             "  return",
             "  'unreachable and unused string literal'",
             "_unusedVar = function() + {}",
-            "load(':foo.bzl', 'bar')");
+            "load(':dep.bzl', 'bar')");
     final String errorMessages =
         new Linter()
-            .setFileContentsReader(p -> file.getBytes(StandardCharsets.ISO_8859_1))
-            .lint(Paths.get("foo"))
+            .setFileContentsReader(
+                p -> {
+                  if ("/foo.bzl".equals(p.toString())) {
+                    return file.getBytes(StandardCharsets.ISO_8859_1);
+                  } else {
+                    throw new NoSuchFileException(p.toString());
+                  }
+                })
+            .lint(Paths.get("/foo.bzl"))
             .toString();
     Truth.assertThat(errorMessages).contains("'+' operator is deprecated"); // bad operation checker
     Truth.assertThat(errorMessages).contains("unreachable statement"); // control flow checker