Skip to content

Commit

Permalink
Stop using JSDoc to name transpiled destructuring parameters
Browse files Browse the repository at this point in the history 
This was a remnant of when destructuring transpilation happened before
typechecking.

Also deletes some unnecessary runtime library injection.

Fixes #3175

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=226386677
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Dec 21, 2018
1 parent 2a27ebb commit cd556c2
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 73 deletions.
55 changes: 15 additions & 40 deletions src/com/google/javascript/jscomp/Es6RewriteDestructuring.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.javascript.rhino.JSDocInfoBuilder;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.TokenStream;
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.JSTypeNative;
import java.util.ArrayDeque;
Expand Down Expand Up @@ -259,8 +258,7 @@ private void pullDestructuringOutOfParams(Node paramList, Node function) {
newParam =
nameOrPattern.isName()
? nameOrPattern
: astFactory.createName(
getTempParameterName(function, i), nameOrPattern.getJSType());
: astFactory.createName(getTempVariableName(), nameOrPattern.getJSType());
Node lhs = nameOrPattern.cloneTree();
Node rhs = defaultValueHook(newParam.cloneTree(), defaultValue);
Node newStatement =
Expand All @@ -279,13 +277,12 @@ private void pullDestructuringOutOfParams(Node paramList, Node function) {
compiler.reportChangeToChangeScope(function);
} else if (param.isDestructuringPattern()) {
insertSpot =
replacePatternParamWithTempVar(
function, insertSpot, param, getTempParameterName(function, i));
replacePatternParamWithTempVar(function, insertSpot, param, getTempVariableName());
compiler.reportChangeToChangeScope(function);
} else if (param.isRest() && param.getFirstChild().isDestructuringPattern()) {
insertSpot =
replacePatternParamWithTempVar(
function, insertSpot, param.getFirstChild(), getTempParameterName(function, i));
function, insertSpot, param.getFirstChild(), getTempVariableName());
compiler.reportChangeToChangeScope(function);
}
}
Expand Down Expand Up @@ -315,27 +312,9 @@ private Node replacePatternParamWithTempVar(
return newDecl;
}

/**
* Find or create the best name to use for a parameter we need to rewrite.
*
* <ol>
* <li> Use the JS Doc function parameter name at the given index, if possible.
* <li> Otherwise, build one of our own.
* </ol>
* @param function
* @param parameterIndex
* @return name to use for the given parameter
*/
private String getTempParameterName(Node function, int parameterIndex) {
String tempVarName;
JSDocInfo fnJSDoc = NodeUtil.getBestJSDocInfo(function);
if (fnJSDoc != null && fnJSDoc.getParameterNameAt(parameterIndex) != null) {
tempVarName = fnJSDoc.getParameterNameAt(parameterIndex);
} else {
tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
}
checkState(TokenStream.isJSIdentifier(tempVarName));
return tempVarName;
/** Creates a new unique name to use for a pattern we need to rewrite. */
private String getTempVariableName() {
return DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
}

private void visitPattern(NodeTraversal t, Node pattern, Node parent) {
Expand Down Expand Up @@ -391,7 +370,7 @@ private void replacePattern(
private void replaceObjectPattern(
NodeTraversal t, Node objectPattern, Node rhs, Node parent, Node nodeToDetach) {
final Scope scope = t.getScope(); // get scope here, AST may be in temporary invalid state later
String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String tempVarName = getTempVariableName();
final JSType tempVarType = objectPattern.getJSType();

String restTempVarName = null;
Expand All @@ -400,7 +379,7 @@ private void replaceObjectPattern(
ArrayList<Node> propsToDeleteForRest = null;
if (objectPattern.hasChildren() && objectPattern.getLastChild().isRest()) {
propsToDeleteForRest = new ArrayList<>();
restTempVarName = DESTRUCTURING_TEMP_VAR + destructuringVarCounter++;
restTempVarName = getTempVariableName();
} else if (rewriteMode == ObjectDestructuringRewriteMode.REWRITE_OBJECT_REST) {
// We are configured to only break object pattern rest, but this destructure has none.
if (!this.patternNestingStack.peekLast().hasNestedObjectRest) {
Expand Down Expand Up @@ -469,7 +448,7 @@ private void replaceObjectPattern(
}
if (propsToDeleteForRest != null) {
// A "...rest" variable is present and result of computation must be cached
String exprEvalTempVarName = DESTRUCTURING_TEMP_VAR + destructuringVarCounter++;
String exprEvalTempVarName = getTempVariableName();
Node exprEvalTempVarModel =
astFactory.createName(exprEvalTempVarName, propExpr.getJSType()); // clone this node
Node exprEvalDecl = IR.var(exprEvalTempVarModel.cloneNode(), propExpr);
Expand All @@ -484,7 +463,7 @@ private void replaceObjectPattern(
astFactory.createGetElem(astFactory.createName(tempVarName, tempVarType), propExpr);

// var tempVarName2 = tempVarName1[propExpr]
String intermediateTempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String intermediateTempVarName = getTempVariableName();
Node intermediateDecl =
IR.var(astFactory.createName(intermediateTempVarName, getelem.getJSType()), getelem);
intermediateDecl.useSourceInfoIfMissingFromForTree(child);
Expand Down Expand Up @@ -608,9 +587,7 @@ private void replaceArrayPattern(
}
}

String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
// TODO(b/77597706): delete the runtime injection after moving this pass post-typechecking
Es6ToEs3Util.preloadEs6RuntimeFunction(compiler, "makeIterator");
String tempVarName = getTempVariableName();
Node makeIteratorCall = astFactory.createJSCompMakeIteratorCall(rhs.detach(), t.getScope());
Node tempVarModel = astFactory.createName(tempVarName, makeIteratorCall.getJSType());
Node tempDecl = IR.var(tempVarModel.cloneNode(), makeIteratorCall);
Expand All @@ -637,7 +614,7 @@ private void replaceArrayPattern(
// var temp0 = $jscomp.makeIterator(rhs);
// var temp1 = temp.next().value
// x = (temp1 === undefined) ? defaultValue : temp1;
String nextVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String nextVarName = getTempVariableName();
// `temp.next().value`
Node nextCallDotValue =
astFactory.createGetProp(
Expand All @@ -661,8 +638,6 @@ private void replaceArrayPattern(
// var temp = $jscomp.makeIterator(rhs);
// x = $jscomp.arrayFromIterator(temp);
newLHS = child.getFirstChild().detach();
// TODO(b/77597706): delete the runtime injection after moving this pass post-typechecking
Es6ToEs3Util.preloadEs6RuntimeFunction(compiler, "arrayFromIterator");
newRHS =
astFactory.createJscompArrayFromIteratorCall(tempVarModel.cloneNode(), t.getScope());
} else {
Expand Down Expand Up @@ -704,7 +679,7 @@ private void replaceArrayPattern(
* temp0.x; var b = temp0.y; return temp0; })
*/
private void wrapAssignmentInCallToArrow(NodeTraversal t, Node assignment) {
String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String tempVarName = getTempVariableName();
Node tempVarModel = astFactory.createName(tempVarName, assignment.getJSType());
Node rhs = assignment.getLastChild().detach();
// let temp0 = rhs;
Expand Down Expand Up @@ -739,7 +714,7 @@ private void wrapAssignmentInCallToArrow(NodeTraversal t, Node assignment) {

private void visitDestructuringPatternInEnhancedFor(Node pattern) {
checkArgument(pattern.isDestructuringPattern());
String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String tempVarName = getTempVariableName();
if (NodeUtil.isEnhancedFor(pattern.getParent())) {
// for ([a, b, c] of arr) {
Node forNode = pattern.getParent();
Expand Down Expand Up @@ -781,7 +756,7 @@ private void visitDestructuringPatternInEnhancedFor(Node pattern) {
}

private void visitDestructuringPatternInCatch(NodeTraversal t, Node pattern) {
String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
String tempVarName = getTempVariableName();
Node catchBlock = pattern.getNext();

pattern.replaceWith(astFactory.createName(tempVarName, pattern.getJSType()));
Expand Down
76 changes: 43 additions & 33 deletions test/com/google/javascript/jscomp/Es6RewriteDestructuringTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,6 @@ public void testObjectDestructuringFunction() {
" var y = $jscomp$destructuring$var2.next().value;",
" var z = $jscomp$destructuring$var2.next().value;",
"}"));
// TODO(b/77597706): inject this runtime library in Es6InjectRuntimeLibraries, so it will happen
// before typechecking.
assertThat(((NoninjectingCompiler) getLastCompiler()).injected)
.containsExactly("es6/util/makeiterator");

test(
"function f({key: x = 5}) {}",
Expand Down Expand Up @@ -286,6 +282,29 @@ public void testObjectDestructuringFunction() {
"}"));
}

@Test
public void testObjectDestructuringFunctionBadJsdoc() {
// see https://github.com/google/closure-compiler/issues/3175
setExpectParseWarningsThisTest(); // intentionally pass bad JSDoc
ignoreWarnings(TypeCheck.INEXISTENT_PROPERTY);
test(
lines(
"/**",
" * @param {{foo: string[]}} obj",
" * @param {string} id",
" */",
"function f({foo}, id) {}"),
lines(
"/**",
" * @param {{foo: string[]}} obj",
" * @param {string} id",
" */",
"function f($jscomp$destructuring$var0, id) {",
" var $jscomp$destructuring$var1 = $jscomp$destructuring$var0;",
" var foo = $jscomp$destructuring$var1.foo;",
"}"));
}

@Test
public void testObjectDestructuringFunctionJsDoc() {
test(
Expand Down Expand Up @@ -420,8 +439,6 @@ public void testArrayDestructuringRest() {
"var $jscomp$destructuring$var0 = $jscomp.makeIterator(f());",
"let one = $jscomp$destructuring$var0.next().value;",
"let others = $jscomp.arrayFromIterator($jscomp$destructuring$var0);"));
assertThat(((NoninjectingCompiler) getLastCompiler()).injected)
.containsExactly("es6/util/arrayfromiterator", "es6/util/makeiterator");

test(
"function f([first, ...rest]) {}",
Expand Down Expand Up @@ -637,46 +654,40 @@ public void testTypeCheck() {
"/** @param {{x: number}} obj */ function f({x}) {}",
lines(
"/** @param {{x: number}} obj */",
"function f(obj) {",
" var $jscomp$destructuring$var0 = obj;",
" var x = $jscomp$destructuring$var0.x;",
"function f($jscomp$destructuring$var0) {",
" var $jscomp$destructuring$var1 = $jscomp$destructuring$var0;",
" var x = $jscomp$destructuring$var1.x;",
"}"));

test(
srcs(lines("/** @param {{x: number}} obj */", "function f({x}) {}", "f({ x: 'str'});")),
warning(TypeValidator.TYPE_MISMATCH_WARNING));

test(
lines("/** @param {{x: number}} obj */", "var f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"var f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"var f = function(obj) {",
" var $jscomp$destructuring$var0 = obj;",
" var x = $jscomp$destructuring$var0.x;",
"var f = function($jscomp$destructuring$var0) {",
" var $jscomp$destructuring$var1 = $jscomp$destructuring$var0;",
" var x = $jscomp$destructuring$var1.x;",
"}"));

test(
lines("/** @param {{x: number}} obj */", "f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"f = function(obj) {",
" var $jscomp$destructuring$var0 = obj;",
" var x = $jscomp$destructuring$var0.x;",
"f = function($jscomp$destructuring$var0) {",
" var $jscomp$destructuring$var1 = $jscomp$destructuring$var0;",
" var x = $jscomp$destructuring$var1.x;",
"}"));

test(
lines("/** @param {{x: number}} obj */", "ns.f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"ns.f = function({x}) {}"),
lines(
"/** @param {{x: number}} obj */",
"ns.f = function(obj) {",
" var $jscomp$destructuring$var0 = obj;",
" var x = $jscomp$destructuring$var0.x;",
"ns.f = function($jscomp$destructuring$var0) {",
" var $jscomp$destructuring$var1 = $jscomp$destructuring$var0;",
" var x = $jscomp$destructuring$var1.x;",
"}"));

test(
Expand All @@ -689,14 +700,13 @@ public void testTypeCheck() {
"};"));

test(
lines("/** @param {{x: number}=} obj */", "ns.f = function({x} = {x: 0}) {};"),
lines(
"/** @param {{x: number}=} obj */",
"ns.f = function({x} = {x: 0}) {};"),
lines(
"/** @param {{x: number}=} obj */",
"ns.f = function(obj) {",
" var $jscomp$destructuring$var0 = obj===undefined ? {x:0} : obj;",
" var x = $jscomp$destructuring$var0.x",
"ns.f = function($jscomp$destructuring$var0) {",
" var $jscomp$destructuring$var1 = ",
" $jscomp$destructuring$var0===undefined ? {x:0} : $jscomp$destructuring$var0;",
" var x = $jscomp$destructuring$var1.x",
"};"));
}

Expand Down

0 comments on commit cd556c2

Please sign in to comment.