Skylint: check for usage of deprecated functions

This only covers functions deprecated in the same file. It does not yet recognize load()ed functions that are deprecated in another file.

RELNOTES: none
PiperOrigin-RevId: 171841455

7 files changed, 333 insertions, 18 deletions

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
new file mode 100644
index 0000000000..923f6360d6
--- /dev/null
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java
@@ -0,0 +1,91 @@
+// 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.syntax.BuildFileAST;
+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.Statement;
+import com.google.devtools.build.lib.syntax.StringLiteral;
+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 java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/** Checks for usage of deprecated functions. */
+public class DeprecationChecker extends AstVisitorWithNameResolution {
+  private final List<Issue> issues = new ArrayList<>();
+  /** Maps a global function name to its deprecation warning, if any. */
+  private final Map<String, String> functionToDeprecationWarning = new HashMap<>();
+
+  public static List<Issue> check(BuildFileAST ast) {
+    DeprecationChecker checker = new DeprecationChecker();
+    checker.visit(ast);
+    return checker.issues;
+  }
+
+  @Override
+  public void visit(BuildFileAST ast) {
+    for (Statement stmt : ast.getStatements()) {
+      if (stmt instanceof FunctionDefStatement) {
+        checkForDeprecation((FunctionDefStatement) stmt);
+      }
+    }
+    super.visit(ast);
+  }
+
+  private void checkForDeprecation(FunctionDefStatement funDef) {
+    StringLiteral docstringLiteral = DocstringUtils.extractDocstring(funDef.getStatements());
+    if (docstringLiteral == null) {
+      return;
+    }
+    DocstringInfo docstring = DocstringUtils.parseDocstring(docstringLiteral, new ArrayList<>());
+    if (!docstring.deprecated.isEmpty()) {
+      functionToDeprecationWarning.put(funDef.getIdentifier().getName(), docstring.deprecated);
+    }
+  }
+
+  @Override
+  public void visit(FunctionDefStatement node) {
+    // Don't issue deprecation warnings inside of deprecated functions:
+    if (!functionToDeprecationWarning.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
+        && functionToDeprecationWarning.containsKey(info.name)
+        && info.kind != Kind.LOCAL) {
+      String deprecationMessage = functionToDeprecationWarning.get(info.name);
+      issues.add(
+          new Issue(
+              "usage of '" + info.name + "' is deprecated: " + deprecationMessage,
+              ident.getLocation()));
+    }
+  }
+}
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
index c5fe40dd9f..48ba98acf5 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringChecker.java
@@ -53,7 +53,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       issues.add(new Issue("file has no module docstring", range));
     } else {
       List<DocstringParseError> errors = new ArrayList<>();
-      parseDocstring(moduleDocstring, errors);
+      DocstringUtils.parseDocstring(moduleDocstring, errors);
       for (DocstringParseError error : errors) {
         issues.add(docstringParseErrorToIssue(moduleDocstring, error));
       }
@@ -83,7 +83,7 @@ public class DocstringChecker extends SyntaxTreeVisitor {
       return;
     }
     List<DocstringParseError> errors = new ArrayList<>();
-    DocstringInfo info = parseDocstring(functionDocstring, errors);
+    DocstringInfo info = DocstringUtils.parseDocstring(functionDocstring, errors);
     for (DocstringParseError error : errors) {
       issues.add(docstringParseErrorToIssue(functionDocstring, error));
     }
@@ -93,12 +93,6 @@ public class DocstringChecker extends SyntaxTreeVisitor {
     }
   }
 
-  private static DocstringInfo parseDocstring(
-      StringLiteral functionDocstring, List<DocstringParseError> errors) {
-    int indentation = functionDocstring.getLocation().getStartLineAndColumn().getColumn() - 1;
-    return DocstringUtils.parseDocstring(functionDocstring.getValue(), indentation, errors);
-  }
-
   private static void checkMultilineFunctionDocstring(
       FunctionDefStatement functionDef,
       StringLiteral docstringLiteral,
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
index 22c5597ee7..7e41d1e818 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DocstringUtils.java
@@ -32,6 +32,7 @@ import javax.annotation.Nullable;
 public final class DocstringUtils {
   private DocstringUtils() {}
 
+  /** Takes a function body and returns the docstring literal, if present. */
   @Nullable
   static StringLiteral extractDocstring(List<Statement> statements) {
     if (statements.isEmpty()) {
@@ -47,6 +48,12 @@ public final class DocstringUtils {
     return null;
   }
 
+  /** Parses a docstring from a string literal and appends any new errors to the given list. */
+  static DocstringInfo parseDocstring(StringLiteral docstring, List<DocstringParseError> errors) {
+    int indentation = docstring.getLocation().getStartLineAndColumn().getColumn() - 1;
+    return parseDocstring(docstring.getValue(), indentation, errors);
+  }
+
   /**
    * Parses a docstring.
    *
@@ -90,16 +97,27 @@ public final class DocstringUtils {
   }
 
   static class DocstringInfo {
+    /** The one-line summary at the start of the docstring. */
     final String summary;
+    /** Documentation of function parameters from the 'Args:' section. */
     final List<ParameterDoc> parameters;
+    /** Documentation of the return value from the 'Returns:' section, or empty if there is none. */
     final String returns;
+    /** Deprecation warning from the 'Deprecated:' section, or empty if there is none. */
+    final String deprecated;
+    /** Rest of the docstring that is not part of any of the special sections above. */
     final String longDescription;
 
     public DocstringInfo(
-        String summary, List<ParameterDoc> parameters, String returns, String longDescription) {
+        String summary,
+        List<ParameterDoc> parameters,
+        String returns,
+        String deprecated,
+        String longDescription) {
       this.summary = summary;
       this.parameters = ImmutableList.copyOf(parameters);
       this.returns = returns;
+      this.deprecated = deprecated;
       this.longDescription = longDescription;
     }
 
@@ -214,8 +232,9 @@ public final class DocstringUtils {
 
     DocstringInfo parse() {
       String summary = line;
+      String nonStandardDeprecation = checkForNonStandardDeprecation(line);
       if (!nextLine()) {
-        return new DocstringInfo(summary, Collections.emptyList(), "", "");
+        return new DocstringInfo(summary, Collections.emptyList(), "", nonStandardDeprecation, "");
       }
       if (!line.isEmpty()) {
         error("the one-line summary should be followed by a blank line");
@@ -225,6 +244,7 @@ public final class DocstringUtils {
       List<String> longDescriptionLines = new ArrayList<>();
       List<ParameterDoc> params = new ArrayList<>();
       String returns = "";
+      String deprecated = "";
       while (!eof()) {
         switch (line) {
           case "Args:":
@@ -232,7 +252,7 @@ public final class DocstringUtils {
               error("section should be preceded by a blank line");
             }
             if (!params.isEmpty()) {
-              error("parameters were already documented before");
+              error("parameters were already documented above");
             }
             if (!returns.isEmpty()) {
               error("parameters should be documented before the return value");
@@ -244,16 +264,40 @@ public final class DocstringUtils {
               error("section should be preceded by a blank line");
             }
             if (!returns.isEmpty()) {
-              error("return value was already documented before");
+              error("return value was already documented above");
             }
             returns = parseSectionAfterHeading();
             break;
+          case "Deprecated:":
+            if (!blankLineBefore) {
+              error("section should be preceded by a blank line");
+            }
+            if (!deprecated.isEmpty()) {
+              error("deprecation message was already documented above");
+            }
+            deprecated = parseSectionAfterHeading();
+            break;
           default:
+            if (deprecated.isEmpty() && nonStandardDeprecation.isEmpty()) {
+              nonStandardDeprecation = checkForNonStandardDeprecation(line);
+            }
             longDescriptionLines.add(line);
             nextLine();
         }
       }
-      return new DocstringInfo(summary, params, returns, String.join("\n", longDescriptionLines));
+      if (deprecated.isEmpty()) {
+        deprecated = nonStandardDeprecation;
+      }
+      return new DocstringInfo(
+          summary, params, returns, deprecated, String.join("\n", longDescriptionLines));
+    }
+
+    private String checkForNonStandardDeprecation(String line) {
+      if (line.toLowerCase().startsWith("deprecated:") || line.contains("DEPRECATED")) {
+        error("use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section");
+        return line;
+      }
+      return "";
     }
 
     private static final Pattern paramLineMatcher =
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 20cebe7796..0296b3d293 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
@@ -38,6 +38,7 @@ public class Linter {
       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)
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
new file mode 100644
index 0000000000..5d55eb7a57
--- /dev/null
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DeprecationCheckerTest.java
@@ -0,0 +1,127 @@
+// 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 com.google.devtools.build.lib.syntax.BuildFileAST;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+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) {
+    String content = String.join("\n", lines);
+    BuildFileAST ast =
+        BuildFileAST.parseString(
+            event -> {
+              throw new IllegalArgumentException(event.getMessage());
+            },
+            content);
+    return DeprecationChecker.check(ast);
+  }
+
+  @Test
+  public void functionDeprecatedInSameFile() {
+    String errorMessages =
+        findIssues(
+                "def f():",
+                "  g()",
+                "  h()",
+                "def g():",
+                "  '''Foo.",
+                "  ",
+                "  Deprecated:",
+                "    reason'''",
+                "def h():",
+                "  '''Bar.",
+                "  ",
+                "  This function is DEPRECATED for some reason.",
+                "  The deprecation should really be documented in a 'Deprecated:' section",
+                "  but the linter should recognize this kind of deprecation as well'''")
+            .toString();
+    Truth.assertThat(errorMessages).contains("2:3: usage of 'g' is deprecated: reason");
+    Truth.assertThat(errorMessages)
+        .contains("3:3: usage of 'h' is deprecated: This function is DEPRECATED for some reason.");
+  }
+
+  @Test
+  public void deprecatedFunctionAliasing() {
+    String errorMessages =
+        findIssues(
+                "def f():",
+                "  h = g",
+                "  h()",
+                "def g():",
+                "  '''Foo.",
+                "  ",
+                "  Deprecated:",
+                "    reason'''")
+            .toString();
+    Truth.assertThat(errorMessages).contains("2:7: usage of 'g' is deprecated: reason");
+  }
+
+  @Test
+  public void functionNotDeprecated() {
+    Truth.assertThat(
+            findIssues(
+                "def f():",
+                "  g()",
+                "def g():",
+                "  '''This is a good function.",
+                "",
+                "  It is emphatically not deprecated.'''"))
+        .isEmpty();
+  }
+
+  @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();
+  }
+
+  @Test
+  public void shadowingWorks() {
+    Truth.assertThat(
+            findIssues(
+                "def f():",
+                "  bad = good",
+                "  bad()",
+                "def good(): pass",
+                "def bad():",
+                "  '''This is a deprecated function.",
+                "",
+                "  Deprecated:",
+                "    reason'''"))
+        .isEmpty();
+  }
+}
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
index 7a29ecd9bc..4524c551d4 100644
--- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
+++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringUtilsTest.java
@@ -156,6 +156,51 @@ public class DocstringUtilsTest {
   }
 
   @Test
+  public void docstringDeprecated() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring(
+            "summary\n"
+                + "\n"
+                + "  Deprecated:\n"
+                + "    line 1\n"
+                + "\n"
+                + "    line 2\n"
+                + "\n"
+                + "  remaining description",
+            2,
+            errors);
+    Truth.assertThat(info.summary).isEqualTo("summary");
+    Truth.assertThat(info.deprecated).isEqualTo("line 1\n\nline 2");
+    Truth.assertThat(info.longDescription).isEqualTo("remaining description");
+    Truth.assertThat(errors).isEmpty();
+  }
+
+  @Test
+  public void docstringDeprecatedTheWrongWay() throws Exception {
+    List<DocstringParseError> errors = new ArrayList<>();
+    DocstringInfo info =
+        DocstringUtils.parseDocstring("summary\n" + "\n" + "  Deprecated: foo\n", 2, errors);
+    Truth.assertThat(info.summary).isEqualTo("summary");
+    Truth.assertThat(info.deprecated).isEqualTo("Deprecated: foo");
+    Truth.assertThat(info.longDescription).isEqualTo("Deprecated: foo");
+    Truth.assertThat(errors).hasSize(1);
+    Truth.assertThat(errors.get(0).toString())
+        .isEqualTo(
+            "3: use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section");
+
+    errors = new ArrayList<>();
+    info = DocstringUtils.parseDocstring("summary\n" + "\n" + "  This is DEPRECATED.\n", 2, errors);
+    Truth.assertThat(info.summary).isEqualTo("summary");
+    Truth.assertThat(info.deprecated).isEqualTo("This is DEPRECATED.");
+    Truth.assertThat(info.longDescription).isEqualTo("This is DEPRECATED.");
+    Truth.assertThat(errors).hasSize(1);
+    Truth.assertThat(errors.get(0).toString())
+        .isEqualTo(
+            "3: use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section");
+  }
+
+  @Test
   public void docstringParameters() throws Exception {
     List<DocstringParseError> errors = new ArrayList<>();
     DocstringInfo info =
@@ -262,12 +307,20 @@ public class DocstringUtilsTest {
                 + "  Returns:\n"
                 + "    bar\n"
                 + "\n"
+                + "  Deprecated:\n"
+                + "    foo\n"
+                + "\n"
+                + "  Deprecated:\n"
+                + "    bar\n"
+                + "\n"
                 + "  description",
             2,
             errors);
     Truth.assertThat(info.parameters).hasSize(2);
-    Truth.assertThat(errors.toString()).contains("6: parameters were already documented before");
-    Truth.assertThat(errors.toString()).contains("12: return value was already documented before");
+    Truth.assertThat(errors.toString()).contains("6: parameters were already documented above");
+    Truth.assertThat(errors.toString()).contains("12: return value was already documented above");
+    Truth.assertThat(errors.toString())
+        .contains("18: deprecation message was already documented above");
   }
 
   @Test
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 98c4bfc8d5..148aa4845e 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
@@ -29,11 +29,15 @@ public class LinterTest {
     final String file =
         String.join(
             "\n",
-            "_unusedVar = function() + {}",
-            "load(':foo.bzl', 'bar')",
             "def function():",
+            "  '''A function.",
+            "  ",
+            "  Deprecated:",
+            "    Reason'''",
             "  return",
-            "  'unreachable and unused string literal'");
+            "  'unreachable and unused string literal'",
+            "_unusedVar = function() + {}",
+            "load(':foo.bzl', 'bar')");
     final String errorMessages =
         new Linter()
             .setFileContentsReader(p -> file.getBytes(StandardCharsets.ISO_8859_1))
@@ -41,6 +45,7 @@ public class LinterTest {
             .toString();
     Truth.assertThat(errorMessages).contains("'+' operator is deprecated"); // bad operation checker
     Truth.assertThat(errorMessages).contains("unreachable statement"); // control flow checker
+    Truth.assertThat(errorMessages).contains("'function' is deprecated"); // deprecation checker
     Truth.assertThat(errorMessages).contains("has no module docstring"); // docstring checker
     Truth.assertThat(errorMessages)
         .contains("load statement should be at the top"); // load statement checker