From da4146069c62af63fc9208352edbc03a25145c66 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Mon, 7 May 2018 10:57:18 -0700 Subject: [PATCH] Fix incorrect scoping of catch variables. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=195687456 --- .../javascript/jscomp/TypedScopeCreator.java | 1 + .../jscomp/TypeCheckNoTranspileTest.java | 10 +++ .../javascript/jscomp/TypeValidatorTest.java | 7 ++ .../jscomp/TypedScopeCreatorTest.java | 83 +++++++++++++++++-- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 7d071b369b4..bc18c4156da 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1462,6 +1462,7 @@ private TypedScope getLValueRootScope(Node n) { case CLASS: case FUNCTION: case PARAM_LIST: + case CATCH: return currentScope; default: TypedVar var = currentScope.getVar(root.getString()); diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index e624ed9bb2e..3925484ca03 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -49,6 +49,16 @@ protected CompilerOptions getDefaultOptions() { return options; } + public void testDuplicateCatchVarName() { + // Make sure that catch variables with the same name are considered to be distinct variables + // rather than causing a redeclaration error. + testTypes( + lines( + "try { throw 1; } catch (/** @type {number} */ err) {}", + "try { throw 'error'; } catch (/** @type {string} */ err) {}", + "")); + } + public void testTypedefFieldInLoopLocal() { testTypes( lines( diff --git a/test/com/google/javascript/jscomp/TypeValidatorTest.java b/test/com/google/javascript/jscomp/TypeValidatorTest.java index da7e21df5aa..b7b4f11744d 100644 --- a/test/com/google/javascript/jscomp/TypeValidatorTest.java +++ b/test/com/google/javascript/jscomp/TypeValidatorTest.java @@ -451,6 +451,13 @@ public void testDuplicateSuppression() { "ns2.x = 'a';"), TypeValidator.DUP_VAR_DECLARATION_TYPE_MISMATCH); + // catch variables in different catch blocks are not duplicate declarations + testSame( + lines( + "try { throw 1; } catch (/** @type {number} */ err) {}", + "try { throw 1; } catch (/** @type {number} */ err) {}" + )); + // duplicates suppressed on 1st declaration. testSame( lines( diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 0efa425f029..e2964426f69 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -16,7 +16,10 @@ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static com.google.javascript.jscomp.ScopeSubject.assertScope; import static com.google.javascript.jscomp.TypedScopeCreator.CTOR_INITIALIZER; import static com.google.javascript.jscomp.TypedScopeCreator.IFACE_INITIALIZER; @@ -42,7 +45,9 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * Tests for {@link TypedScopeCreator} and {@link TypeInference}. Admittedly, @@ -56,6 +61,26 @@ public final class TypedScopeCreatorTest extends CompilerTestCase { private TypedScope lastLocalScope; private TypedScope lastFunctionScope; + /** + * Maps a label name to information about the labeled statement. + * + *

This map is recreated each time parseAndRunTypeInference() is executed. + * TODO(bradfordcsmith): This map and LabeledStatement are also in TypeInferenceTest. + * It would be good to unify them. + */ + private Map labeledStatementMap; + + /** Stores information about a labeled statement and allows making assertions on it. */ + static class LabeledStatement { + final Node statementNode; + final TypedScope enclosingScope; + + LabeledStatement(Node statementNode, TypedScope enclosingScope) { + this.statementNode = checkNotNull(statementNode); + this.enclosingScope = checkNotNull(enclosingScope); + } + } + @Override protected void setUp() throws Exception { super.setUp(); @@ -70,20 +95,40 @@ protected int getNumRepetitions() { private class ScopeFinder extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { - TypedScope s = t.getTypedScope(); - if (s.isGlobal()) { - globalScope = s; - } else if (s.isBlockScope()) { - lastLocalScope = s; - } else if (s.isFunctionScope()) { - lastFunctionScope = s; + TypedScope scope = t.getTypedScope(); + if (scope.isGlobal()) { + globalScope = scope; + } else if (scope.isBlockScope()) { + // TODO(bradfordcsmith): use labels to find scopes instead of lastLocalScope + lastLocalScope = scope; + } else if (scope.isFunctionScope()) { + lastFunctionScope = scope; + } + if (parent != null && parent.isLabel() && !n.isLabelName()) { + // First child of a LABEL is a LABEL_NAME, n is the second child. + Node labelNameNode = checkNotNull(n.getPrevious(), n); + checkState(labelNameNode.isLabelName(), labelNameNode); + String labelName = labelNameNode.getString(); + assertWithMessage("Duplicate label name: %s", labelName) + .that(labeledStatementMap) + .doesNotContainKey(labelName); + labeledStatementMap.put(labelName, new LabeledStatement(n, scope)); } } } + private LabeledStatement getLabeledStatement(String label) { + assertWithMessage("No statement found for label: %s", label) + .that(labeledStatementMap) + .containsKey(label); + return labeledStatementMap.get(label); + } + @Override protected CompilerPass getProcessor(final Compiler compiler) { registry = compiler.getTypeRegistry(); + // Create a fresh statement map for each test case. + labeledStatementMap = new HashMap<>(); return new CompilerPass() { @Override public void process(Node externs, Node root) { @@ -2294,6 +2339,30 @@ public void testDeclaredCatchExpression2() { assertEquals("string", lastLocalScope.getVar("e").getType().toString()); } + public void testDuplicateCatchVariableNames() { + testSame( + lines( + "try {}", // preserve newlines + "catch (err) {", + " FIRST_CATCH: err;", + "}", + "try {}", + "catch (err) {", + " SECOND_CATCH: err;", + "}", + "")); + TypedScope firstCatchScope = getLabeledStatement("FIRST_CATCH").enclosingScope; + assertScope(firstCatchScope).declares("err").directly(); + TypedVar firstErrVar = firstCatchScope.getVar("err"); + + TypedScope secondCatchScope = getLabeledStatement("SECOND_CATCH").enclosingScope; + assertScope(firstCatchScope).declares("err").directly(); + assertThat(firstCatchScope).isNotSameAs(secondCatchScope); + + TypedVar secondErrVar = secondCatchScope.getVar("err"); + assertThat(firstErrVar).isNotSameAs(secondErrVar); + } + public void testGenerator1() { testSame("function *gen() { yield 1; } var g = gen();"); assertEquals("function(): Generator", findNameType("gen", globalScope).toString());