Skip to content

Commit

Permalink
Simplify MakeDeclaredNamesUnique by taking advantage of scope creatio…
Browse files Browse the repository at this point in the history 
…n that already happens automatically. This fixes some unit tests which were previously disabled, removes some unnecessary renaming, and allows us to remove code that was essentially duplicated between MakeDeclaredNamesUnique and Es6SyntacticScopeCreator.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=168622855
  • Loading branch information
@lauraharker
tbreisacher authored and lauraharker committed Sep 14, 2017
1 parent 6387d2a commit 0f83d45
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 195 deletions.
14 changes: 5 additions & 9 deletions src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java
View file
Expand Up @@ -16,7 +16,6 @@

package com.google.javascript.jscomp;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;

import com.google.javascript.rhino.InputId;
Expand Down Expand Up @@ -100,6 +99,9 @@ static class ScopeScanner {
private final Scope scope;
private final AbstractCompiler compiler;
private final RedeclarationHandler redeclarationHandler;

// Will be null, when a detached node is traversed.
@Nullable
private InputId inputId;
private final Set<Node> changeRootSet;

Expand All @@ -124,10 +126,6 @@ void populate() {
inputId = NodeUtil.getInputId(n);
switch (n.getToken()) {
case FUNCTION: {
// TODO(johnlenz): inputId maybe null if the FUNCTION node is detached
// from the AST.
// Is it meaningful to build a scope for detached FUNCTION node?

final Node fnNameNode = n.getFirstChild();
final Node args = fnNameNode.getNext();

Expand Down Expand Up @@ -267,7 +265,6 @@ private void scanVars(Node n, @Nullable Scope hoistScope, @Nullable Scope blockS
return;
}
inputId = n.getInputId();
checkNotNull(inputId);
break;

case MODULE_BODY:
Expand Down Expand Up @@ -312,9 +309,8 @@ private void declareVar(Scope s, Node n) {
String name = n.getString();
// Because of how we scan the variables, it is possible to encounter
// the same var declared name node twice. Bail out in this case.
// TODO(johnlenz): hash lookups are not free and
// building scopes are already expensive
// restructure the scope building to avoid this check.
// TODO(johnlenz): Hash lookups are not free and building scopes are already expensive.
// Restructure the scope building to avoid this check.
Var v = s.getOwnSlot(name);
if (v != null && v.getNode() == n) {
return;
Expand Down
14 changes: 5 additions & 9 deletions src/com/google/javascript/jscomp/FunctionInjector.java
View file
Expand Up @@ -199,8 +199,7 @@ CanInlineResult canInlineReferenceToFunction(
// last until explicitly cleared.
if (containsFunctions) {
if (!assumeMinimumCapture && !ref.scope.isGlobal()) {
// TODO(johnlenz): Allow inlining into any scope without local names or
// inner functions.
// TODO(johnlenz): Allow inlining into any scope without local names or inner functions.
return CanInlineResult.NO;
} else if (NodeUtil.isWithinLoop(callNode)) {
// An inner closure maybe relying on a local value holding a value for a
Expand Down Expand Up @@ -486,8 +485,7 @@ void maybePrepareCall(Reference ref) {
* canInlineReferenceAsStatementBlock into the call site, replacing the
* parent expression.
*/
private Node inlineFunction(
Reference ref, Node fnNode, String fnName) {
private Node inlineFunction(Reference ref, Node fnNode, String fnName) {
Node callNode = ref.callNode;
Node parent = callNode.getParent();
Node grandParent = parent.getParent();
Expand Down Expand Up @@ -530,8 +528,7 @@ private Node inlineFunction(
throw new IllegalStateException("Unexpected call site type.");
}

FunctionToBlockMutator mutator = new FunctionToBlockMutator(
compiler, this.safeNameIdSupplier);
FunctionToBlockMutator mutator = new FunctionToBlockMutator(compiler, this.safeNameIdSupplier);

boolean isCallInLoop = NodeUtil.isWithinLoop(callNode);
Node newBlock = mutator.mutate(
Expand Down Expand Up @@ -931,9 +928,8 @@ private static int inlineCostDelta(
final int perReturnResultOverhead = 3; // "XX="
final int perAliasOverhead = 3; // "XX="

// TODO(johnlenz): Counting the number of returns is relatively expensive
// this information should be determined during the traversal and
// cached.
// TODO(johnlenz): Counting the number of returns is relatively expensive.
// This information should be determined during the traversal and cached.
int returnCount = NodeUtil.getNodeTypeReferenceCount(
block, Token.RETURN, new NodeUtil.MatchShallowStatement());
int resultCount = (returnCount > 0) ? returnCount - 1 : 0;
Expand Down
11 changes: 8 additions & 3 deletions src/com/google/javascript/jscomp/FunctionToBlockMutator.java
View file
Expand Up @@ -22,6 +22,7 @@
import static com.google.javascript.jscomp.FunctionArgumentInjector.THIS_MARKER;

import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableList;
import com.google.javascript.jscomp.MakeDeclaredNamesUnique.InlineRenamer;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
Expand Down Expand Up @@ -110,6 +111,8 @@ private Node mutateInternal(
boolean isCallInLoop,
boolean renameLocals) {
Node newFnNode = fnNode.cloneTree();
// Wrap the clone in a script, in a root, so that makeLocalNamesUnique sees a coherent tree.
IR.root(IR.script(newFnNode));

if (renameLocals) {
// Now that parameter names have been replaced, make sure all the local
Expand Down Expand Up @@ -243,13 +246,15 @@ private static void fixUninitializedVarDeclarations(Node n, Node containingBlock
private void makeLocalNamesUnique(Node fnNode, boolean isCallInLoop) {
Supplier<String> idSupplier = compiler.getUniqueNameIdSupplier();
// Make variable names unique to this instance.
NodeTraversal.traverseEs6(
NodeTraversal.traverseEs6ScopeRoots(
compiler,
fnNode,
null,
ImmutableList.of(fnNode),
new MakeDeclaredNamesUnique(
new InlineRenamer(
compiler.getCodingConvention(), idSupplier, "inline_", isCallInLoop, true, null),
false));
false),
true);
// Make label names unique to this instance.
new RenameLabels(compiler, new LabelNameSupplier(idSupplier), false, false)
.process(null, fnNode);
Expand Down
114 changes: 8 additions & 106 deletions src/com/google/javascript/jscomp/MakeDeclaredNamesUnique.java
View file
Expand Up @@ -43,10 +43,8 @@
* Find all Functions, VARs, and Exception names and make them
* unique. Specifically, it will not modify object properties.
* @author johnlenz@google.com (John Lenz)
* TODO(johnlenz): Try to merge this with the ScopeCreator.
* TODO(moz): Handle more ES6 features, such as default parameters.
*/
class MakeDeclaredNamesUnique implements NodeTraversal.ScopedCallback {
class MakeDeclaredNamesUnique extends NodeTraversal.AbstractScopedCallback {

// Arguments is special cased to handle cases where a local name shadows
// the arguments declaration.
Expand All @@ -55,8 +53,6 @@ class MakeDeclaredNamesUnique implements NodeTraversal.ScopedCallback {
// There is one renamer on the stack for each scope. This was added before any support for ES6 was
// in place, so it was necessary to maintain this separate stack, rather than just using the
// NodeTraversal's stack of scopes, in order to handle catch blocks and function names correctly.
// TODO(tbreisacher): Now that this class always uses the Es6SyntacticScopeCreator it may be
// possible to greatly simplify the way the renamerStack field is used.
private final Deque<Renamer> renamerStack = new ArrayDeque<>();
private final Renamer rootRenamer;
private final boolean markChanges;
Expand Down Expand Up @@ -85,17 +81,12 @@ public void enterScope(NodeTraversal t) {
"MakeDeclaredNamesUnique requires an ES6-compatible scope creator. %s is not compatible.",
t.getScopeCreator());
Node declarationRoot = t.getScopeRoot();
// Function bodies are handled along with PARAM_LIST
if (NodeUtil.isFunctionBlock(declarationRoot)) {
return;
}

Renamer renamer;
if (renamerStack.isEmpty()) {
// If the contextual renamer is being used, the starting context can not
// be a function.
checkState(!declarationRoot.isFunction() || !(rootRenamer instanceof ContextualRenamer));
checkState(t.inGlobalScope());
renamer = rootRenamer;
} else {
boolean hoist = !declarationRoot.isFunction() && !NodeUtil.createsBlockScope(declarationRoot);
Expand All @@ -104,63 +95,16 @@ public void enterScope(NodeTraversal t) {

renamerStack.push(renamer);

if (!declarationRoot.isFunction()) {
// Add the block declarations
findDeclaredNames(t, declarationRoot);
}
findDeclaredNames(t, declarationRoot);
}

@Override
public void exitScope(NodeTraversal t) {
// ES6 function blocks are handled along with PARAM_LIST
if (NodeUtil.isFunctionBlock(t.getScopeRoot())) {
return;
}
if (!t.inGlobalScope()) {
renamerStack.pop();
}
}

@Override
public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
switch (n.getToken()) {
case FUNCTION: {
// Add recursive function name, if needed.
// NOTE: "enterScope" is called after we need to pick up this name.
Renamer renamer = renamerStack.peek().createForChildScope(n, false);

// If needed, add the function recursive name.
String name = n.getFirstChild().getString();
if (!name.isEmpty() && parent != null && !NodeUtil.isFunctionDeclaration(n)) {
renamer.addDeclaredName(name, false);
}

renamerStack.push(renamer);
break;
}

case PARAM_LIST: {
Renamer renamer = renamerStack.peek().createForChildScope(n, true);

// Add the function parameters
for (Node lhs : NodeUtil.getLhsNodesOfDeclaration(n)) {
renamer.addDeclaredName(lhs.getString(), true);
}

Node functionBody = n.getNext();
renamerStack.push(renamer);

findDeclaredNames(t, functionBody);
break;
}

default:
break;
}

return true;
}

@Override
public void visit(NodeTraversal t, Node n, Node parent) {
switch (n.getToken()) {
Expand All @@ -178,19 +122,6 @@ public void visit(NodeTraversal t, Node n, Node parent) {
break;
}

case FUNCTION:
// Remove the function body scope
renamerStack.pop();
// Remove function recursive name (if any).
renamerStack.pop();
break;

case PARAM_LIST:
// Note: The parameters and function body variables live in the
// same scope, we introduce the scope when in the "shouldTraverse"
// visit of PARAM_LIST, but remove it when when we exit the function above.
break;

default:
break;
}
Expand Down Expand Up @@ -220,6 +151,7 @@ private void visitName(NodeTraversal t, Node n, Node parent) {
}
}
}

/**
* Walks the stack of name maps and finds the replacement name for the
* current scope.
Expand All @@ -239,36 +171,10 @@ private String getReplacementName(String oldName) {
* the {@code Renamer} that is at the top of the {@code renamerStack}.
*/
private void findDeclaredNames(NodeTraversal t, Node n) {
findDeclaredNamesHelper(t, n, false);
}

private void findDeclaredNamesHelper(NodeTraversal t, Node n, boolean recursive) {
Renamer renamer = renamerStack.peek();
Node parent = n.getParent();

// Do a shallow traversal: Don't traverse into the function param list or body; just its name.
if (recursive && parent.isFunction() && n != parent.getFirstChild()) {
return;
}
if (NodeUtil.isBlockScopedDeclaration(n)) {
if (t.getScopeRoot() == NodeUtil.getEnclosingScopeRoot(n)) {
renamer.addDeclaredName(n.getString(), false);
// For functions, findDeclaredNames is called from enterScope when entering the function
// scope, rather than when entering the function body scope, so we need to check for that
// case as well.
} else if (t.getScopeRoot().isFunction()
&& NodeUtil.getEnclosingScopeRoot(n) == NodeUtil.getFunctionBody(t.getScopeRoot())) {
renamer.addDeclaredName(n.getString(), false);
}
} else if (n.isName() && n.getParent().isVar()) {
renamer.addDeclaredName(n.getString(), true);
} else if (NodeUtil.isFunctionDeclaration(n)) {
Node nameNode = n.getFirstChild();
renamer.addDeclaredName(nameNode.getString(), false);
}
checkState(NodeUtil.createsScope(n) || n.isScript(), n);

for (Node c = n.getFirstChild(); c != null; c = c.getNext()) {
findDeclaredNamesHelper(t, c, true);
for (Var v : t.getScope().getVarIterable()) {
renamerStack.peek().addDeclaredName(v.getName(), false);
}
}

Expand Down Expand Up @@ -517,10 +423,7 @@ public String toString() {
/** Constructor for child scopes. */
private ContextualRenamer(
Node scopeRoot, Multiset<String> nameUsage, boolean hoistingTargetScope, Renamer parent) {
// Currently we are passing the PARAM_LIST node as the "root" of the function body scope.
// TODO(tbreisacher): Pass the function body itself, so that "scopeRoot" will always be an
// actual scope root.
checkState(NodeUtil.createsScope(scopeRoot) || scopeRoot.isParamList(), scopeRoot);
checkState(NodeUtil.createsScope(scopeRoot), scopeRoot);

if (scopeRoot.isFunction()) {
checkState(!hoistingTargetScope, scopeRoot);
Expand Down Expand Up @@ -673,8 +576,7 @@ private String getUniqueName(String name) {

// By using the same separator the id will be stripped if it isn't
// needed when variable renaming is turned off.
return name + ContextualRenamer.UNIQUE_ID_SEPARATOR
+ idPrefix + uniqueIdSupplier.get();
return name + ContextualRenamer.UNIQUE_ID_SEPARATOR + idPrefix + uniqueIdSupplier.get();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/Scope.java
View file
Expand Up @@ -46,7 +46,7 @@ public class Scope implements StaticScope, Serializable {
* Creates a Scope given the parent Scope and the root node of the current scope.
*
* @param parent The parent Scope. Cannot be null.
* @param rootNode The root node of the curent scope. Cannot be null.
* @param rootNode The root node of the current scope. Cannot be null.
*/
Scope(Scope parent, Node rootNode) {
checkNotNull(parent);
Expand Down

0 comments on commit 0f83d45

Please sign in to comment.