Detect deprecated operations on dict() variables in Skylint RELNOTES: None. PiperOrigin-RevId: 184667932
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 index 0fdc0ee..65922b5 100644 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/BadOperationChecker.java
@@ -16,7 +16,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import com.google.common.collect.Sets; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.syntax.ASTNode; import com.google.devtools.build.lib.syntax.AssignmentStatement; import com.google.devtools.build.lib.syntax.AugmentedAssignmentStatement; @@ -31,7 +31,8 @@ import com.google.devtools.skylark.skylint.Environment.NameInfo; import java.util.ArrayList; import java.util.List; -import java.util.Set; +import java.util.Map; +import javax.annotation.Nullable; /** Checks for operations that are deprecated */ public class BadOperationChecker extends AstVisitorWithNameResolution { @@ -41,12 +42,22 @@ private final List<Issue> issues = new ArrayList<>(); + enum NodeType { + DEPSET, + DICT + } + /** - * Set of variables that (we assume) are depsets. - * We consider x a depset variable if it appears in a statement of form `x = expr`, where - * expr is either a call to `depset()` or else is itself a depset variable. + * An inferred map of variables mapped to their type. + * We consider x an 'inferred-type' variable if it appears in a statement of form `x = expr`, + * where expr is either a trivially-typed constructor (e.g. 'depset()') or is itself an + * inferred-type variable. + * + * In principle, it's possible that a variable is reassigned to a different type and that + * this map is therefore inaccurate. In practice, however, that's a fairly uncommon and + * error-prone pattern. */ - private final Set<Integer> depsetVariables = Sets.newHashSet(); + private final Map<Integer, NodeType> inferredTypeVariables = Maps.newHashMap(); private BadOperationChecker() {} @@ -56,44 +67,54 @@ return checker.issues; } - /** Use heuristic to guess if a node is an expression of type depset. */ - private boolean isDepset(ASTNode node) { + /** + * If the given node is an inferred-type {@link NodeType}, return that type. + * Otherwise return null. + */ + @Nullable + private NodeType getInferredTypeOrNull(ASTNode node) { if (node instanceof Identifier) { NameInfo name = env.resolveName(((Identifier) node).getName()); - return name != null && depsetVariables.contains(name.id); + return name != null ? inferredTypeVariables.get(name.id) : null; } if (node instanceof FuncallExpression) { Expression function = ((FuncallExpression) node).getFunction(); - return function instanceof Identifier && ((Identifier) function).getName().equals("depset"); + if (function instanceof Identifier) { + if (((Identifier) function).getName().equals("depset")) { + return NodeType.DEPSET; + } else if (((Identifier) function).getName().equals("dict")) { + return NodeType.DICT; + } + } } - - return false; + if (node instanceof DictionaryLiteral || node instanceof DictComprehension) { + return NodeType.DICT; + } + return null; } @Override public void visit(BinaryOperatorExpression node) { super.visit(node); + NodeType lhsNodeType = getInferredTypeOrNull(node.getLhs()); + NodeType rhsNodeType = getInferredTypeOrNull(node.getRhs()); + if (node.getOperator() == Operator.PLUS) { - if (node.getLhs() instanceof DictionaryLiteral - || node.getLhs() instanceof DictComprehension - || node.getRhs() instanceof DictionaryLiteral - || node.getRhs() instanceof DictComprehension) { + if (lhsNodeType == NodeType.DICT || rhsNodeType == NodeType.DICT) { issues.add( Issue.create( DEPRECATED_PLUS_DICT_CATEGORY, "'+' operator is deprecated and should not be used on dictionaries", node.getLocation())); } - - if (isDepset(node.getLhs()) || isDepset(node.getRhs())) { + if (lhsNodeType == NodeType.DEPSET || rhsNodeType == NodeType.DEPSET) { issues.add( Issue.create( DEPRECATED_PLUS_DEPSET_CATEGORY, "'+' operator is deprecated and should not be used on depsets", node.getLocation())); } - } else if (node.getOperator() == Operator.PIPE) { issues.add( Issue.create( @@ -108,21 +129,21 @@ @Override public void visit(AugmentedAssignmentStatement node) { super.visit(node); - if (node.getExpression() instanceof DictionaryLiteral - || node.getExpression() instanceof DictComprehension) { + Identifier ident = Iterables.getOnlyElement(node.getLValue().boundIdentifiers()); + NodeType identType = getInferredTypeOrNull(ident); + NodeType expressionType = getInferredTypeOrNull(node.getExpression()); + if (identType == NodeType.DICT || expressionType == NodeType.DICT) { issues.add( Issue.create( DEPRECATED_PLUS_DICT_CATEGORY, "'+=' operator is deprecated and should not be used on dictionaries", node.getLocation())); } - - Identifier ident = Iterables.getOnlyElement(node.getLValue().boundIdentifiers()); - if (isDepset(ident) || isDepset(node.getExpression())) { + if (identType == NodeType.DEPSET || expressionType == NodeType.DEPSET) { issues.add( Issue.create( DEPRECATED_PLUS_DEPSET_CATEGORY, - "'+' operator is deprecated and should not be used on depsets", + "'+=' operator is deprecated and should not be used on depsets", node.getLocation())); } } @@ -135,8 +156,10 @@ return; } Identifier ident = Iterables.getOnlyElement(lvalues); - if (isDepset(node.getExpression())) { - depsetVariables.add(env.resolveName(ident.getName()).id); + NodeType inferredType = getInferredTypeOrNull(node.getExpression()); + + if (inferredType != null) { + inferredTypeVariables.put(env.resolveName(ident.getName()).id, inferredType); } } }
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 index 2e31121..834b0e0 100644 --- a/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java +++ b/src/tools/skylark/javatests/com/google/devtools/skylark/skylint/BadOperationCheckerTest.java
@@ -89,13 +89,49 @@ .contains( "2:1-2:9: '+' operator is deprecated"); + Truth.assertThat(findIssues("foo = depset()", "bar = foo", "bar + baz").toString()) + .contains( + "3:1-3:9: '+' operator is deprecated"); + Truth.assertThat(findIssues("foo = depset()", "foo += bar").toString()) .contains( - "2:1-2:10: '+' operator is deprecated"); + "2:1-2:10: '+=' operator is deprecated"); Truth.assertThat(findIssues("foo += depset()").toString()) .contains( - "1:1-1:15: '+' operator is deprecated"); + "1:1-1:15: '+=' operator is deprecated"); + } + + @Test + public void dictPlusOperator() { + Truth.assertThat(findIssues("foo + dict()").toString()) + .contains( + "1:1-1:12: '+' operator is deprecated and should not be used on dictionaries " + + "[deprecated-plus-dict]"); + + Truth.assertThat(findIssues("foo = dict()", "foo + bar").toString()) + .contains( + "2:1-2:9: '+' operator is deprecated"); + + Truth.assertThat(findIssues("foo = dict()", "bar = foo", "bar + baz").toString()) + .contains( + "3:1-3:9: '+' operator is deprecated"); + + Truth.assertThat(findIssues("foo = dict()", "foo += bar").toString()) + .contains( + "2:1-2:10: '+=' operator is deprecated"); + + Truth.assertThat(findIssues("foo += dict()").toString()) + .contains( + "1:1-1:13: '+=' operator is deprecated"); + + Truth.assertThat(findIssues("foo += { 5:3 }").toString()) + .contains( + "1:1-1:14: '+=' operator is deprecated"); + + Truth.assertThat(findIssues("foo = { 5:3 }", "bar = foo", "bar + baz").toString()) + .contains( + "3:1-3:9: '+' operator is deprecated"); } @Test