Preserve `pass` statements in the Skylark AST. RELNOTES: none PiperOrigin-RevId: 173125138
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java index 4842374..9a1ed7a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -206,6 +206,8 @@ case LOAD: execLoad((LoadStatement) st); break; + case PASS: + break; case RETURN: execReturn((ReturnStatement) st); break;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java index e59f329..ac0902a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -1120,8 +1120,7 @@ // small_stmt | 'pass' private void parseSmallStatementOrPass(List<Statement> list) { if (token.kind == TokenKind.PASS) { - // Skip the token, don't add it to the list. - // It has no existence in the AST. + list.add(setLocation(new PassStatement(), token.left, token.right)); expect(TokenKind.PASS); } else { list.add(parseSmallStatement());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/PassStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/PassStatement.java new file mode 100644 index 0000000..e035100 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/syntax/PassStatement.java
@@ -0,0 +1,37 @@ +// 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.build.lib.syntax; + +import java.io.IOException; + +/** Syntax node for a `pass` statement. */ +public class PassStatement extends Statement { + + @Override + public void prettyPrint(Appendable buffer, int indentLevel) throws IOException { + printIndent(buffer, indentLevel); + buffer.append("pass\n"); + } + + @Override + public void accept(SyntaxTreeVisitor visitor) { + visitor.visit(this); + } + + @Override + public Kind kind() { + return Kind.PASS; + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java index 1f08bbb..43363fc 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
@@ -33,6 +33,7 @@ FUNCTION_DEF, IF, LOAD, + PASS, RETURN, }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java index c7c0eb0..22bc8b7 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
@@ -141,6 +141,8 @@ visitBlock(node.getStatements()); } + public void visit(PassStatement node) {} + public void visit(ReturnStatement node) { if (node.getReturnExpression() != null) { visit(node.getReturnExpression());
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 858e0ca..a19fc83 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
@@ -982,7 +982,8 @@ @Test public void testPass() throws Exception { List<Statement> statements = parseFileForSkylark("pass\n"); - assertThat(statements).isEmpty(); + assertThat(statements).hasSize(1); + assertThat(statements.get(0)).isInstanceOf(PassStatement.class); } @Test @@ -993,7 +994,7 @@ assertThat(statements).hasSize(1); FunctionDefStatement stmt = (FunctionDefStatement) statements.get(0); - assertThat(stmt.getStatements()).isEmpty(); + assertThat(stmt.getStatements().get(0)).isInstanceOf(PassStatement.class); } @Test
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 66b36a2..3869bfa 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
@@ -83,7 +83,7 @@ Location start = Location.from(node.getLocation().getStartLineAndColumn()); Location end; if (node.getStatements().isEmpty()) { - // The function body can be empty because the parser discards `pass` statements: + // empty statement suites cannot come from the parser yet we should handle this gracefully: end = Location.from(node.getLocation().getEndLineAndColumn()); } else { LineAndColumn lac = node.getStatements().get(0).getLocation().getStartLineAndColumn();
diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java index 876ce24..9abcfc1 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/DocstringCheckerTests.java
@@ -16,6 +16,7 @@ import com.google.common.truth.Truth; import com.google.devtools.build.lib.syntax.BuildFileAST; +import com.google.devtools.build.lib.syntax.FunctionDefStatement; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,11 +45,12 @@ .contains("1:1-2:1: file has no module docstring [missing-docstring]"); Truth.assertThat(errorMessage) .contains("2:1-3:2: function 'function' has no docstring [missing-docstring]"); - // The following function has zero statements since the parser throws `pass` statements away. - // Hence we have to check this case to make sure the end location is set correctly. - errorMessage = findIssues("def function():", " pass # no function docstring").toString(); - Truth.assertThat(errorMessage) - .contains("1:1-2:30: function 'function' has no docstring [missing-docstring]"); + // This function has zero statements. We want to make sure the end location is set correctly: + BuildFileAST ast = BuildFileAST.parseString(event -> {}, "def foo():"); + FunctionDefStatement funDefWithEmptySuite = (FunctionDefStatement) ast.getStatements().get(0); + Truth.assertThat(funDefWithEmptySuite.getStatements()).isEmpty(); + Truth.assertThat(DocstringChecker.check(ast).toString()) + .contains("1:1-1:10: function 'foo' has no docstring [missing-docstring]"); } @Test