diff options
author | fzaiser <fzaiser@google.com> | 2017-09-28 09:47:21 -0400 |
---|---|---|
committer | John Cater <jcater@google.com> | 2017-09-29 12:12:36 -0400 |
commit | c2b88a63a4f82a2bc7aefe18ab5e46746212b312 (patch) | |
tree | 26359a5b3eb3648a87620dfb5f831f2dfe2805ab /src/tools | |
parent | 471c0e1678d0471961f1dc467666991e4cce3846 (diff) |
Skylint: check for bad operations
So far, only checks for usages of '+' on dictionary literals and
comprehensions.
RELNOTES: none
PiperOrigin-RevId: 170336917
Diffstat (limited to 'src/tools')
4 files changed, 139 insertions, 1 deletions
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java new file mode 100644 index 0000000000..8b08bdff65 --- /dev/null +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java @@ -0,0 +1,66 @@ +// 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.AugmentedAssignmentStatement; +import com.google.devtools.build.lib.syntax.BinaryOperatorExpression; +import com.google.devtools.build.lib.syntax.BuildFileAST; +import com.google.devtools.build.lib.syntax.DictComprehension; +import com.google.devtools.build.lib.syntax.DictionaryLiteral; +import com.google.devtools.build.lib.syntax.Operator; +import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor; +import java.util.ArrayList; +import java.util.List; + +/** Checks for operations that are deprecated */ +public class BadOperationChecker extends SyntaxTreeVisitor { + private final List<Issue> issues = new ArrayList<>(); + + private BadOperationChecker() {} + + public static List<Issue> check(BuildFileAST ast) { + BadOperationChecker checker = new BadOperationChecker(); + checker.visit(ast); + return checker.issues; + } + + @Override + public void visit(BinaryOperatorExpression node) { + super.visit(node); + if (node.getOperator() == Operator.PLUS) { + if (node.getLhs() instanceof DictionaryLiteral + || node.getLhs() instanceof DictComprehension + || node.getRhs() instanceof DictionaryLiteral + || node.getRhs() instanceof DictComprehension) { + issues.add( + new Issue( + "'+' operator is deprecated and should not be used on dictionaries", + node.getLocation())); + } + } + } + + @Override + public void visit(AugmentedAssignmentStatement node) { + super.visit(node); + if (node.getExpression() instanceof DictionaryLiteral + || node.getExpression() instanceof DictComprehension) { + issues.add( + new Issue( + "'+' operator is deprecated and should not be used on dictionaries", + node.getLocation())); + } + } +} 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 0f9fa65865..3ec3853c78 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 @@ -36,6 +36,7 @@ public class Linter { /** Map of all 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("docstring", DocstringChecker::check) .put("load", LoadStatementChecker::check) diff --git a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java new file mode 100644 index 0000000000..a9a2a46ff1 --- /dev/null +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java @@ -0,0 +1,70 @@ +// 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; + +@RunWith(JUnit4.class) +public class BadOperationCheckerTest { + 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 BadOperationChecker.check(ast); + } + + @Test + public void dictionaryLiteralPlusOperator() { + Truth.assertThat(findIssues("{} + foo").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + Truth.assertThat(findIssues("foo + {}").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + Truth.assertThat(findIssues("foo += {}").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + } + + @Test + public void dictionaryComprehensionPlusOperator() { + Truth.assertThat(findIssues("{k:v for k,v in []} + foo").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + Truth.assertThat(findIssues("foo + {k:v for k,v in []}").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + Truth.assertThat(findIssues("foo += {k:v for k,v in []}").toString()) + .contains(":1:1: '+' operator is deprecated and should not be used on dictionaries"); + } + + @Test + public void dictionaryPlusOperatorNested() { + Truth.assertThat(findIssues("foo + ({} + bar)").toString()) + .contains(":1:7: '+' operator is deprecated and should not be used on dictionaries"); + Truth.assertThat(findIssues("foo + (bar + {})").toString()) + .contains(":1:7: '+' operator is deprecated and should not be used on dictionaries"); + } + + @Test + public void plusOperatorNoIssue() { + Truth.assertThat(findIssues("foo + bar")).isEmpty(); + Truth.assertThat(findIssues("foo += bar")).isEmpty(); + } +} 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 3bbade4733..98c4bfc8d5 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,7 +29,7 @@ public class LinterTest { final String file = String.join( "\n", - "_unusedVar = function()", + "_unusedVar = function() + {}", "load(':foo.bzl', 'bar')", "def function():", " return", @@ -39,6 +39,7 @@ public class LinterTest { .setFileContentsReader(p -> file.getBytes(StandardCharsets.ISO_8859_1)) .lint(Paths.get("foo")) .toString(); + Truth.assertThat(errorMessages).contains("'+' operator is deprecated"); // bad operation checker Truth.assertThat(errorMessages).contains("unreachable statement"); // control flow checker Truth.assertThat(errorMessages).contains("has no module docstring"); // docstring checker Truth.assertThat(errorMessages) |