bazel syntax: delete StarlarkFunction.getStatements Clients cannot assume the representation of code is syntax trees; soon, it will be bytecode. Details: - added StarlarkFunction.getDocumentation method. - remove dead code from DocstringUtils collectDocstringLiterals, getNameAndDocstring, getAssignedVariableName, extractDocstring - DocstringUtils.parseDocstring now computes the indentation using a heuristic. - removed assertion from StarlarkFunctionCodecTest - rephrased assertion in PackageSerializationTest - remove function syntax from some obscure log messages PiperOrigin-RevId: 290084502
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java index bd53793..b9bf93b 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java
@@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.events.Location; +import javax.annotation.Nullable; /** A StarlarkFunction is the function value created by a Starlark {@code def} statement. */ public final class StarlarkFunction extends BaseFunction { @@ -69,11 +70,21 @@ return name; } - /** @deprecated Do not assume function values are represented as syntax trees. */ - // TODO(adonovan): the only non-test use is to obtain the function's doc string. Add API for that. - @Deprecated - public ImmutableList<Statement> getStatements() { - return statements; + /** Returns the value denoted by the function's doc string literal, or null if absent. */ + @Nullable + public String getDocumentation() { + if (statements.isEmpty()) { + return null; + } + Statement first = statements.get(0); + if (!(first instanceof ExpressionStatement)) { + return null; + } + Expression expr = ((ExpressionStatement) first).getExpression(); + if (!(expr instanceof StringLiteral)) { + return null; + } + return ((StringLiteral) expr).getValue(); } public Module getModule() {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java index bee563d..d38f85d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
@@ -23,16 +23,18 @@ /** Syntax node for a string literal. */ public final class StringLiteral extends Expression { - String value; - public String getValue() { - return value; - } + private final String value; StringLiteral(String value) { this.value = value; } + /** Returns the value denoted by the string literal */ + public String getValue() { + return value; + } + @Override public void accept(NodeVisitor visitor) { visitor.visit(this);
diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java index 06fe769..02ed1e0 100644 --- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java +++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -58,6 +58,8 @@ import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; +import com.google.devtools.build.lib.syntax.Expression; +import com.google.devtools.build.lib.syntax.ExpressionStatement; import com.google.devtools.build.lib.syntax.LoadStatement; import com.google.devtools.build.lib.syntax.Module; import com.google.devtools.build.lib.syntax.Mutability; @@ -122,7 +124,6 @@ import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo; import com.google.devtools.common.options.OptionsParser; -import com.google.devtools.skylark.common.DocstringUtils; import java.io.BufferedOutputStream; import java.io.FileOutputStream; import java.io.IOException; @@ -395,10 +396,12 @@ private static String getModuleDoc(StarlarkFile buildFileAST) { ImmutableList<Statement> fileStatements = buildFileAST.getStatements(); if (!fileStatements.isEmpty()) { - Statement moduleComment = fileStatements.get(0); - StringLiteral moduleDocLiteral = DocstringUtils.getStringLiteral(moduleComment); - if (moduleDocLiteral != null) { - return moduleDocLiteral.getValue(); + Statement stmt = fileStatements.get(0); + if (stmt instanceof ExpressionStatement) { + Expression expr = ((ExpressionStatement) stmt).getExpression(); + if (expr instanceof StringLiteral) { + return ((StringLiteral) expr).getValue(); + } } } return "";
diff --git a/src/main/java/com/google/devtools/build/skydoc/rendering/FunctionUtil.java b/src/main/java/com/google/devtools/build/skydoc/rendering/FunctionUtil.java index efe1a8e..712a6ca 100644 --- a/src/main/java/com/google/devtools/build/skydoc/rendering/FunctionUtil.java +++ b/src/main/java/com/google/devtools/build/skydoc/rendering/FunctionUtil.java
@@ -21,7 +21,6 @@ import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.syntax.Printer.BasePrinter; import com.google.devtools.build.lib.syntax.StarlarkFunction; -import com.google.devtools.build.lib.syntax.StringLiteral; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.FunctionParamInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.StarlarkFunctionInfo; import com.google.devtools.skylark.common.DocstringUtils; @@ -41,24 +40,21 @@ * @param functionName the name of the function in the target scope. (Note this is not necessarily * the original exported function name; the function may have been renamed in the target * Starlark file's scope) - * @param userDefinedFunction the raw function object + * @param fn the function object * @throws com.google.devtools.build.skydoc.rendering.DocstringParseException if the function's * docstring is malformed */ - public static StarlarkFunctionInfo fromNameAndFunction( - String functionName, StarlarkFunction userDefinedFunction) throws DocstringParseException { + public static StarlarkFunctionInfo fromNameAndFunction(String functionName, StarlarkFunction fn) + throws DocstringParseException { String functionDescription = ""; Map<String, String> paramNameToDocMap = Maps.newLinkedHashMap(); - StringLiteral docStringLiteral = - DocstringUtils.extractDocstring(userDefinedFunction.getStatements()); - - if (docStringLiteral != null) { + String doc = fn.getDocumentation(); + if (doc != null) { List<DocstringParseError> parseErrors = Lists.newArrayList(); - DocstringInfo docstringInfo = DocstringUtils.parseDocstring(docStringLiteral, parseErrors); + DocstringInfo docstringInfo = DocstringUtils.parseDocstring(doc, parseErrors); if (!parseErrors.isEmpty()) { - throw new DocstringParseException( - functionName, userDefinedFunction.getLocation(), parseErrors); + throw new DocstringParseException(functionName, fn.getLocation(), parseErrors); } functionDescription += docstringInfo.getSummary(); if (!docstringInfo.getSummary().isEmpty() && !docstringInfo.getLongDescription().isEmpty()) { @@ -69,7 +65,7 @@ paramNameToDocMap.put(paramDoc.getParameterName(), paramDoc.getDescription()); } } - List<FunctionParamInfo> paramsInfo = parameterInfos(userDefinedFunction, paramNameToDocMap); + List<FunctionParamInfo> paramsInfo = parameterInfos(fn, paramNameToDocMap); return StarlarkFunctionInfo.newBuilder() .setFunctionName(functionName) .setDocString(functionDescription)
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java index b97ac8d..096247b 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -40,7 +40,6 @@ assertThat(stmt).isNotNull(); assertThat(stmt.getName()).isEqualTo("func"); assertThat(stmt.getSignature().numMandatoryPositionals()).isEqualTo(3); - assertThat(stmt.getStatements()).hasSize(2); } @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java index 6a6decf..37df98f 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -273,7 +273,7 @@ @Test public void testStringLiteralOptimizationValue() throws Exception { StringLiteral l = (StringLiteral) parseExpression("'abc' + 'def'"); - assertThat(l.value).isEqualTo("abcdef"); + assertThat(l.getValue()).isEqualTo("abcdef"); } @Test
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/common/DocstringUtils.java b/src/tools/skylark/java/com/google/devtools/skylark/common/DocstringUtils.java index af7daea..f57dabb 100644 --- a/src/tools/skylark/java/com/google/devtools/skylark/common/DocstringUtils.java +++ b/src/tools/skylark/java/com/google/devtools/skylark/common/DocstringUtils.java
@@ -15,123 +15,20 @@ package com.google.devtools.skylark.common; import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.syntax.AssignmentStatement; -import com.google.devtools.build.lib.syntax.DefStatement; -import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.ExpressionStatement; -import com.google.devtools.build.lib.syntax.Identifier; -import com.google.devtools.build.lib.syntax.StarlarkFile; -import com.google.devtools.build.lib.syntax.Statement; -import com.google.devtools.build.lib.syntax.StringLiteral; import com.google.devtools.skylark.common.LocationRange.Location; -import java.util.AbstractMap; -import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; -import javax.annotation.Nullable; /** Utilities to extract and parse docstrings. */ public final class DocstringUtils { - private DocstringUtils() {} - /** - * Collect all docstrings in the AST and store them in a map: name -> docstring. - * - * <p>Note that local variables can't have docstrings. - * - * @param ast the AST to traverse - * @return a map from identifier names to their docstring; if there is a file-level docstring, its - * key is "". - */ - public static ImmutableMap<String, StringLiteral> collectDocstringLiterals(StarlarkFile ast) { - ImmutableMap.Builder<String, StringLiteral> nameToDocstringLiteral = ImmutableMap.builder(); - Statement previousStatement = null; - for (Statement currentStatement : ast.getStatements()) { - Map.Entry<String, StringLiteral> entry = - getNameAndDocstring(previousStatement, currentStatement); - if (entry != null) { - nameToDocstringLiteral.put(entry); - } - previousStatement = currentStatement; - } - return nameToDocstringLiteral.build(); - } - - @Nullable - private static Map.Entry<String, StringLiteral> getNameAndDocstring( - @Nullable Statement previousStatement, Statement currentStatement) { - // function docstring: - if (currentStatement instanceof DefStatement) { - StringLiteral docstring = extractDocstring(((DefStatement) currentStatement).getStatements()); - if (docstring != null) { - return new AbstractMap.SimpleEntry<>( - ((DefStatement) currentStatement).getIdentifier().getName(), docstring); - } - } else { - StringLiteral docstring = getStringLiteral(currentStatement); - if (docstring != null) { - if (previousStatement == null) { - // file docstring: - return new SimpleEntry<>("", docstring); - } else { - // variable docstring: - String variable = getAssignedVariableName(previousStatement); - if (variable != null) { - return new SimpleEntry<>(variable, docstring); - } - } - } - } - return null; - } - - /** If the statement is an assignment to one variable, returns its name, or otherwise null. */ - @Nullable - public static String getAssignedVariableName(@Nullable Statement stmt) { - if (stmt instanceof AssignmentStatement) { - AssignmentStatement assign = (AssignmentStatement) stmt; - Expression lhs = assign.getLHS(); - if (lhs instanceof Identifier && !assign.isAugmented()) { - return ((Identifier) lhs).getName(); - } - } - return null; - } - - /** If the statement is a string literal, returns it, or otherwise null. */ - @Nullable - public static StringLiteral getStringLiteral(Statement stmt) { - if (stmt instanceof ExpressionStatement) { - Expression expr = ((ExpressionStatement) stmt).getExpression(); - if (expr instanceof StringLiteral) { - return (StringLiteral) expr; - } - } - return null; - } - - /** Takes a function body and returns the docstring literal, if present. */ - @Nullable - public static StringLiteral extractDocstring(List<Statement> statements) { - if (statements.isEmpty()) { - return null; - } - return getStringLiteral(statements.get(0)); - } - - /** Parses a docstring from a string literal and appends any new errors to the given list. */ - public static DocstringInfo parseDocstring( - StringLiteral docstring, List<DocstringParseError> errors) { - int indentation = docstring.getStartLocation().column() - 1; - return parseDocstring(docstring.getValue(), indentation, errors); - } + private DocstringUtils() {} // uninstantiable /** * Parses a docstring. @@ -163,13 +60,11 @@ * }</pre> * * @param docstring a docstring of the format described above - * @param indentation the indentation level (number of spaces) of the docstring * @param parseErrors a list to which parsing error messages are written * @return the parsed docstring information */ - public static DocstringInfo parseDocstring( - String docstring, int indentation, List<DocstringParseError> parseErrors) { - DocstringParser parser = new DocstringParser(docstring, indentation); + public static DocstringInfo parseDocstring(String doc, List<DocstringParseError> parseErrors) { + DocstringParser parser = new DocstringParser(doc); DocstringInfo result = parser.parse(); parseErrors.addAll(parser.errors); return result; @@ -289,7 +184,7 @@ /** Current line number within the docstring. */ private int lineNumber = 0; /** - * The indentation of the doctring literal in the source file. + * The indentation of the docstring literal in the source file. * * <p>Every line except the first one must be indented by at least that many spaces. */ @@ -311,8 +206,31 @@ /** Errors that occurred so far. */ private final List<DocstringParseError> errors = new ArrayList<>(); - DocstringParser(String docstring, int indentation) { + private DocstringParser(String docstring) { this.docstring = docstring; + + // Infer the indentation level: + // the smallest amount of leading whitespace + // common to all non-blank lines except the first. + int indentation = Integer.MAX_VALUE; + boolean first = true; + for (String line : Splitter.on("\n").split(docstring)) { + // ignore first line + if (first) { + first = false; + continue; + } + // count leading spaces + int i; + for (i = 0; i < line.length() && line.charAt(i) == ' '; i++) {} + if (i != line.length()) { + indentation = Math.min(indentation, i); + } + } + if (indentation == Integer.MAX_VALUE) { + indentation = 0; + } + nextLine(); // the indentation is only relevant for the following lines, not the first one: this.baselineIndentation = indentation;