Skylint: also check for deprecations in dependencies.

RELNOTES: none
PiperOrigin-RevId: 173658526

6 files changed, 750 insertions, 68 deletions

diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java
new file mode 100644
index 0000000000..8f2abb8bf9
--- /dev/null
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DependencyAnalyzer.java
@@ -0,0 +1,216 @@
+// 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.ImmutableList;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
+import com.google.devtools.build.lib.syntax.LoadStatement;
+import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.skylark.skylint.Linter.FileFacade;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Helps collect information about direct and transitive dependencies of a Skylark file.
+ *
+ * @param <T> the type of information associated with a file
+ */
+public class DependencyAnalyzer<T> {
+  private final FileFacade fileFacade;
+  private final Map<Path, T> pathToInfo = new LinkedHashMap<>();
+  private Map<Path, Path> cachedWorkspaceRoot = new LinkedHashMap<>();
+  private Map<Path, Path> cachedPackageRoot = new LinkedHashMap<>();
+  private DependencyCollector<T> collector;
+  private Set<Path> visited = new LinkedHashSet<>();
+
+  private static final ImmutableList<String> BUILD_FILES = ImmutableList.of("BUILD", "BUILD.bazel");
+  private static final ImmutableList<String> WORKSPACE_FILE = ImmutableList.of("WORKSPACE");
+
+  /**
+   * Creates an instance of DependencyAnalyzer that can be reused for multiple Skylark files.
+   *
+   * @param fileFacade interface to access file contents
+   * @param collector extracts the desired information from a Skylark file
+   */
+  public DependencyAnalyzer(FileFacade fileFacade, DependencyCollector<T> collector) {
+    this.fileFacade = fileFacade;
+    this.collector = collector;
+  }
+
+  /**
+   * Collects information about the given file and its direct and transitive dependencies.
+   *
+   * <p>The information is cached between calls to this method, so it won't reanalyze the same file
+   * twice. This applies even if a file is loaded via different path labels that correspond to the
+   * same canonical path.
+   *
+   * @param path the path of the file to be analyzed
+   * @return the information about that file, or null if it can't be read
+   */
+  @Nullable
+  public T collectTransitiveInfo(Path path) {
+    path = path.toAbsolutePath();
+    if (visited.contains(path)) {
+      return pathToInfo.get(path);
+    }
+    visited.add(path);
+    BuildFileAST ast;
+    try {
+      ast = fileFacade.readAst(path);
+    } catch (IOException e) {
+      return null;
+    }
+    T info = collector.initInfo(path);
+    for (Statement stmt : ast.getStatements()) {
+      if (stmt instanceof LoadStatement) {
+        String label = ((LoadStatement) stmt).getImport().getValue();
+        Path dep = labelToPath(label, path);
+        if (dep == null) {
+          continue;
+        }
+        T depInfo = collectTransitiveInfo(dep);
+        if (depInfo == null) {
+          continue; // may happen if there's an illegal dependency cycle
+        }
+        info = collector.loadDependency(info, (LoadStatement) stmt, dep, depInfo);
+      }
+    }
+    info = collector.collectInfo(path, ast, info);
+    pathToInfo.put(path, info);
+    return info;
+  }
+
+  @Nullable
+  private Path findAncestorDirectoryContainingAnyOf(Path path, Iterable<String> fileNames) {
+    Path dir = path.toAbsolutePath();
+    while ((dir = dir.getParent()) != null) {
+      for (String fileName : fileNames) {
+        if (fileFacade.fileExists(dir.resolve(fileName))) {
+          return dir;
+        }
+      }
+    }
+    return null;
+  }
+
+  /**
+   * Resolves the label of a load statement to a path.
+   *
+   * @param label the import of a load statement
+   * @param currentPath the path of the file containing the load statement
+   * @return the path corresponding to the label or null if it can't be resolved
+   */
+  @Nullable
+  private Path labelToPath(String label, Path currentPath) {
+    if (label.startsWith("@")) {
+      // TODO(skylark-team): analyze such dependencies as well
+      return null;
+    } else if (label.startsWith("//")) {
+      Path workspaceRoot = getWorkspaceRoot(currentPath);
+      if (workspaceRoot == null) {
+        return null;
+      }
+      label = label.substring(label.startsWith("//:") ? 3 : 2);
+      return workspaceRoot.resolve(label.replace(':', '/'));
+    } else if (label.startsWith(":")) {
+      Path packageRoot = getPackageRoot(currentPath);
+      if (packageRoot == null) {
+        return null;
+      }
+      return packageRoot.resolve(label.substring(1));
+    } else {
+      // otherwise just treat it as a though it started with "//"
+      Path workspaceRoot = getWorkspaceRoot(currentPath);
+      if (workspaceRoot == null) {
+        return null;
+      }
+      return workspaceRoot.resolve(label.replace(':', '/'));
+    }
+  }
+
+  @Nullable
+  private Path getPackageRoot(Path path) {
+    if (!cachedPackageRoot.containsKey(path)) {
+      cachedPackageRoot.put(path, findAncestorDirectoryContainingAnyOf(path, BUILD_FILES));
+    }
+    return cachedPackageRoot.get(path);
+  }
+
+  @Nullable
+  private Path getWorkspaceRoot(Path path) {
+    if (!cachedWorkspaceRoot.containsKey(path)) {
+      cachedWorkspaceRoot.put(path, findAncestorDirectoryContainingAnyOf(path, WORKSPACE_FILE));
+    }
+    return cachedWorkspaceRoot.get(path);
+  }
+
+  /**
+   * Encapsulates how to produce information about a Skylark file, given its contents and the
+   * information about its transitive dependencies.
+   *
+   * <p>Each of the methods returns new or updated instances of T associated with the file.
+   *
+   * <p>When analyzing a file, the methods will be invoked in the following order:
+   *
+   * <ol>
+   *   <li>{@link DependencyCollector#initInfo} to return an initial info object
+   *   <li>{@link DependencyCollector#loadDependency} is iteratively called for each direct
+   *       dependency to get an updated info object that accounts for the info from this dependency,
+   *       until all direct dependencies have been processed
+   *   <li>{@link DependencyCollector#collectInfo} is called to get a final updated info object that
+   *       accounts for the content of the current file.
+   * </ol>
+   *
+   * @param <T> the type of information being collected
+   */
+  public interface DependencyCollector<T> {
+
+    /**
+     * Used to initialize the dependency information when starting analysis of a file.
+     *
+     * @param path the path of the current file
+     * @return the initial information about the current file
+     */
+    T initInfo(Path path);
+
+    /**
+     * Incorporates the information about a dependency in the information about the file.
+     *
+     * <p>This method is called for every load() statement in the current file.
+     *
+     * @param currentFileInfo info about the current file so far, may be modified
+     * @param stmt the load statement being processed
+     * @param loadedPath the path of the dependency that is load()ed
+     * @param loadedFileInfo info about the dependency file that is load()ed, must not be modified
+     * @return the updated information about the current file
+     */
+    T loadDependency(T currentFileInfo, LoadStatement stmt, Path loadedPath, T loadedFileInfo);
+
+    /**
+     * Collect information about the current file after the load statements have been processed.
+     *
+     * @param path the path of the current file
+     * @param ast the ast of the current file
+     * @param info the information about the current file so far, may be modified
+     * @return the updated information about the current file
+     */
+    T collectInfo(Path path, BuildFileAST ast, T info);
+  }
+}
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
index fc4f435447..9fb7ba171d 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
@@ -19,11 +19,14 @@ import com.google.devtools.build.lib.syntax.FunctionDefStatement;
 import com.google.devtools.build.lib.syntax.Identifier;
 import com.google.devtools.build.lib.syntax.LoadStatement;
 import com.google.devtools.build.lib.syntax.StringLiteral;
+import com.google.devtools.skylark.skylint.DependencyAnalyzer.DependencyCollector;
 import com.google.devtools.skylark.skylint.DocstringUtils.DocstringInfo;
 import com.google.devtools.skylark.skylint.Environment.NameInfo;
 import com.google.devtools.skylark.skylint.Environment.NameInfo.Kind;
+import com.google.devtools.skylark.skylint.Linter.FileFacade;
+import java.nio.file.Path;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -33,55 +36,123 @@ public class DeprecationChecker extends AstVisitorWithNameResolution {
   private static final String DEPRECATED_SYMBOL_CATEGORY = "deprecated-symbol";
 
   private final List<Issue> issues = new ArrayList<>();
-  /** Maps a global function name to its deprecation warning, if any. */
-  private final Map<String, String> symbolToDeprecationWarning = new HashMap<>();
+  /** Maps a global function name to its deprecation information, if any. */
+  private final Map<String, DeprecatedSymbol> symbolToDeprecation;
+  /** Path of the file that is checked. */
+  private final Path path;
 
-  public static List<Issue> check(BuildFileAST ast) {
-    DeprecationChecker checker = new DeprecationChecker();
-    checker.visit(ast);
-    return checker.issues;
+  private DeprecationChecker(Path path, Map<String, DeprecatedSymbol> symbolToDeprecation) {
+    this.path = path;
+    this.symbolToDeprecation = symbolToDeprecation;
   }
 
-  @Override
-  public void visit(BuildFileAST ast) {
-    Map<String, StringLiteral> docstrings = DocstringUtils.collectDocstringLiterals(ast);
-    for (Entry<String, StringLiteral> entry : docstrings.entrySet()) {
-      String symbol = entry.getKey();
-      StringLiteral docstring = entry.getValue();
-      DocstringInfo info = DocstringUtils.parseDocstring(docstring, new ArrayList<>());
-      if (!info.deprecated.isEmpty()) {
-        symbolToDeprecationWarning.put(symbol, info.deprecated);
-      }
-    }
-    super.visit(ast);
+  public static List<Issue> check(Path path, BuildFileAST ast, FileFacade fileFacade) {
+    Map<String, DeprecatedSymbol> symbolToDeprecationWarning =
+        DeprecationCollector.getDeprecations(path, fileFacade);
+    DeprecationChecker checker = new DeprecationChecker(path, symbolToDeprecationWarning);
+    checker.visit(ast);
+    return checker.issues;
   }
 
   @Override
   public void visit(FunctionDefStatement node) {
     // Don't issue deprecation warnings inside of deprecated functions:
-    if (!symbolToDeprecationWarning.containsKey(node.getIdentifier().getName())) {
+    if (!symbolToDeprecation.containsKey(node.getIdentifier().getName())) {
       super.visit(node);
     }
   }
 
   @Override
-  public void visit(LoadStatement stmt) {
-    super.visit(stmt);
-    // TODO(skylark-team): analyze dependencies for deprecations.
-  }
-
-  @Override
   void use(Identifier ident) {
     NameInfo info = env.resolveName(ident.getName());
-    if (info != null
-        && symbolToDeprecationWarning.containsKey(info.name)
-        && info.kind != Kind.LOCAL) {
-      String deprecationMessage = symbolToDeprecationWarning.get(info.name);
-      issues.add(
-          Issue.create(
-              DEPRECATED_SYMBOL_CATEGORY,
-              "usage of '" + info.name + "' is deprecated: " + deprecationMessage,
-              ident.getLocation()));
+    if (info == null) {
+      return;
+    }
+    DeprecatedSymbol deprecation = symbolToDeprecation.get(info.name);
+    if (deprecation != null && info.kind != Kind.LOCAL && info.kind != Kind.PARAMETER) {
+      String originInfo = "";
+      if (deprecation.origin != path) {
+        originInfo = "(imported from " + deprecation.origin;
+        if (!deprecation.originalName.equals(info.name)) {
+          originInfo += ", named '" + deprecation.originalName + "' there";
+        }
+        originInfo += ") ";
+      }
+      String message =
+          "usage of '"
+              + info.name
+              + "' "
+              + originInfo
+              + "is deprecated: "
+              + deprecation.deprecationMessage;
+      issues.add(Issue.create(DEPRECATED_SYMBOL_CATEGORY, message, ident.getLocation()));
+    }
+  }
+
+  /** Holds information about a deprecated symbol. */
+  private static class DeprecatedSymbol {
+    final Path origin;
+    final String originalName;
+    final String deprecationMessage;
+
+    public DeprecatedSymbol(Path origin, String originalName, String deprecationMessage) {
+      this.origin = origin;
+      this.originalName = originalName;
+      this.deprecationMessage = deprecationMessage;
+    }
+  }
+
+  /** Collects information about deprecated symbols (including dependencies). */
+  private static class DeprecationCollector
+      implements DependencyCollector<Map<String, DeprecatedSymbol>> {
+
+    /**
+     * Returns deprecation information (including dependencies) about symbols in the given file.
+     *
+     * @param path the path of the file to collect deprecation from
+     * @param fileFacade to access files
+     * @return a map: symbol name -> deprecation info
+     */
+    public static Map<String, DeprecatedSymbol> getDeprecations(Path path, FileFacade fileFacade) {
+      return new DependencyAnalyzer<>(fileFacade, new DeprecationCollector())
+          .collectTransitiveInfo(path);
+    }
+
+    @Override
+    public Map<String, DeprecatedSymbol> initInfo(Path path) {
+      return new LinkedHashMap<>();
+    }
+
+    @Override
+    public Map<String, DeprecatedSymbol> loadDependency(
+        Map<String, DeprecatedSymbol> currentFileInfo,
+        LoadStatement stmt,
+        Path loadedPath,
+        Map<String, DeprecatedSymbol> loadedFileInfo) {
+      for (Entry<Identifier, String> entry : stmt.getSymbolMap().entrySet()) {
+        String originalName = entry.getValue();
+        String alias = entry.getKey().getName();
+        DeprecatedSymbol originalDeprecation = loadedFileInfo.get(originalName);
+        if (originalDeprecation != null) {
+          currentFileInfo.put(alias, originalDeprecation);
+        }
+      }
+      return currentFileInfo;
+    }
+
+    @Override
+    public Map<String, DeprecatedSymbol> collectInfo(
+        Path path, BuildFileAST ast, Map<String, DeprecatedSymbol> deprecationInfos) {
+      Map<String, StringLiteral> docstrings = DocstringUtils.collectDocstringLiterals(ast);
+      for (Entry<String, StringLiteral> entry : docstrings.entrySet()) {
+        String symbol = entry.getKey();
+        StringLiteral docstring = entry.getValue();
+        DocstringInfo info = DocstringUtils.parseDocstring(docstring, new ArrayList<>());
+        if (!info.deprecated.isEmpty()) {
+          deprecationInfos.put(symbol, new DeprecatedSymbol(path, symbol, info.deprecated));
+        }
+      }
+      return deprecationInfos;
     }
   }
 }
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
index d403ecf771..a97936d6ee 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/Linter.java
@@ -20,6 +20,7 @@ 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.NoSuchFileException;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
@@ -34,26 +35,43 @@ import java.util.Set;
  */
 public class Linter {
   private static final String PARSE_ERROR_CATEGORY = "parse-error";
-  /** Map of all checks and their names. */
+  /** Map of all single-file checks and their names. */
   private static final ImmutableMap<String, Check> nameToCheck =
       ImmutableMap.<String, Check>builder()
           .put("bad-operation", BadOperationChecker::check)
           .put("control-flow", ControlFlowChecker::check)
-          .put("deprecation", DeprecationChecker::check)
           .put("docstring", DocstringChecker::check)
           .put("load", LoadStatementChecker::check)
           .put("naming", NamingConventionsChecker::check)
           .put("no-effect", StatementWithoutEffectChecker::check)
           .put("usage", UsageChecker::check)
           .build();
+  /** Map of all multi-file checks and their names. */
+  private static final ImmutableMap<String, MultiFileCheck> nameToMultiFileCheck =
+      ImmutableMap.<String, MultiFileCheck>builder()
+          .put("deprecation", DeprecationChecker::check)
+          .build();
 
   /** Function to read files (can be changed for testing). */
-  private FileContentsReader fileReader = Files::readAllBytes;
+  private FileFacade fileFacade = DEFAULT_FILE_FACADE;
+
+  private static final FileFacade DEFAULT_FILE_FACADE =
+      new FileFacade() {
+        @Override
+        public boolean fileExists(Path path) {
+          return Files.exists(path);
+        }
+
+        @Override
+        public byte[] readBytes(Path path) throws IOException {
+          return Files.readAllBytes(path);
+        }
+      };
 
   private final Set<String> disabledChecks = new LinkedHashSet<>();
 
-  public Linter setFileContentsReader(FileContentsReader reader) {
-    this.fileReader = reader;
+  public Linter setFileContentsReader(FileFacade reader) {
+    this.fileFacade = reader;
     return this;
   }
 
@@ -72,7 +90,8 @@ public class Linter {
    * @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);
+    path = path.toAbsolutePath();
+    String content = new String(fileFacade.readBytes(path), StandardCharsets.ISO_8859_1);
     List<Issue> issues = new ArrayList<>();
     BuildFileAST ast =
         BuildFileAST.parseString(
@@ -89,6 +108,12 @@ public class Linter {
       }
       issues.addAll(entry.getValue().check(ast));
     }
+    for (Entry<String, MultiFileCheck> entry : nameToMultiFileCheck.entrySet()) {
+      if (disabledChecks.contains(entry.getKey())) {
+        continue;
+      }
+      issues.addAll(entry.getValue().check(path, ast, fileFacade));
+    }
     issues.sort(Issue::compareLocation);
     return issues;
   }
@@ -99,8 +124,41 @@ public class Linter {
    * <p>This is useful because we can use a fake for testing.
    */
   @FunctionalInterface
-  public interface FileContentsReader {
-    byte[] read(Path path) throws IOException;
+  public interface FileFacade {
+
+    /**
+     * Reads a file path to bytes.
+     *
+     * <p>This operation may be repeated for the same file.
+     */
+    byte[] readBytes(Path path) throws IOException;
+
+    /**
+     * Reads a file and parses it to an AST.
+     *
+     * <p>The default implementation silently ignores syntax errors.
+     */
+    default BuildFileAST readAst(Path path) throws IOException {
+      String contents = new String(readBytes(path), StandardCharsets.ISO_8859_1);
+      return BuildFileAST.parseString(event -> {}, contents);
+    }
+
+    /**
+     * Checks whether a given file exists.
+     *
+     * <p>The default implementation invokes readBytes and returns false if {@link
+     * NoSuchFileException} is thrown, true otherwise.
+     */
+    default boolean fileExists(Path path) {
+      try {
+        readBytes(path);
+      } catch (NoSuchFileException e) {
+        return false;
+      } catch (IOException e) {
+        // This method shouldn't throw.
+      }
+      return true;
+    }
   }
 
   /** Allows to invoke a check. */
@@ -108,4 +166,10 @@ public class Linter {
   public interface Check {
     List<Issue> check(BuildFileAST ast);
   }
+
+  /** Allows to invoke a check. */
+  @FunctionalInterface
+  public interface MultiFileCheck {
+    List<Issue> check(Path path, BuildFileAST ast, FileFacade fileFacade);
+  }
 }
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