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

4 files changed, 143 insertions, 13 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)