From feb8050e553712f4d689d9e84e624abc0f4c2fa8 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Wed, 11 Jul 2018 05:32:24 -0700 Subject: [Skylint] Fix a crash on analyzing augmented assignment to IndexExpression. Fixes #5534. Skylint was operating under assumption that all lvalues have a single bound identifier, which is not true for `IndexExpression`s like ``` d["foo"] += "bar" ``` As a result, Skylint would crash. This change makes it handle cases without bound identifiers gracefully. Ideally `IndexExpression` assigments should be analyzed too, but it's a more involved change. Closes #5535. PiperOrigin-RevId: 204109060 --- .../com/google/devtools/skylark/skylint/BadOperationChecker.java | 8 +++++++- .../google/devtools/skylark/skylint/BadOperationCheckerTest.java | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-) (limited to 'src/tools') 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 09df8ff7fe..cb9460ad01 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 @@ -137,7 +137,13 @@ public class BadOperationChecker extends AstVisitorWithNameResolution { @Override public void visit(AugmentedAssignmentStatement node) { super.visit(node); - Identifier ident = Iterables.getOnlyElement(node.getLValue().boundIdentifiers()); + ImmutableSet lvalues = node.getLValue().boundIdentifiers(); + if (lvalues.size() != 1) { + // assignment visitor does not track information about assignments to IndexExpressions like + // kwargs["name"] += "foo" so nothing to do here until that changes + return; + } + Identifier ident = Iterables.getOnlyElement(lvalues); NodeType identType = getInferredTypeOrNull(ident); NodeType expressionType = getInferredTypeOrNull(node.getExpression()); if (identType == NodeType.DICT || expressionType == NodeType.DICT) { 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 27c1063147..20cd662de7 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 @@ -152,4 +152,9 @@ public class BadOperationCheckerTest { .contains("1:1-1:5: '/' operator is deprecated"); Truth.assertThat(findIssues("5 // 2")).isEmpty(); } + + @Test + public void augmentedAssignmentOperator() { + Truth.assertThat(findIssues("kwargs['name'] += 'foo'")).isEmpty(); + } } -- cgit v1.2.3