Skip to content

Commit

Permalink
Minor refactorings and dead code removal in AmbiguateProperties
Browse files Browse the repository at this point in the history 
This doesn't change any behavior. All the added unit tests pass with
or without this change, but cover useful code paths that were not previously
unit tested in AmbiguateProperties (but were integration tested)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=221714530
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Nov 17, 2018
1 parent 5fa22ee commit db49787
Show file tree
Hide file tree
Showing 2 changed files with 209 additions and 145 deletions.
244 changes: 118 additions & 126 deletions src/com/google/javascript/jscomp/AmbiguateProperties.java
View file
Expand Up @@ -79,9 +79,9 @@ class AmbiguateProperties implements CompilerPass {
private final AbstractCompiler compiler; private final AbstractCompiler compiler;


private final List<Node> stringNodesToRename = new ArrayList<>(); private final List<Node> stringNodesToRename = new ArrayList<>();
// Can't use these as property names. // Can't use these to start property names.
private final char[] reservedFirstCharacters; private final char[] reservedFirstCharacters;
// Can't use these as property names. // Can't use these at all in property names.
private final char[] reservedNonFirstCharacters; private final char[] reservedNonFirstCharacters;


/** Map from property name to Property object */ /** Map from property name to Property object */
Expand Down Expand Up @@ -120,12 +120,6 @@ public int compare(Property p1, Property p2) {
/** A set of types that invalidate properties from ambiguation. */ /** A set of types that invalidate properties from ambiguation. */
private final InvalidatingTypes invalidatingTypes; private final InvalidatingTypes invalidatingTypes;


/**
* Prefix of properties to skip renaming. These should be renamed in the
* RenameProperties pass.
*/
static final String SKIP_PREFIX = "JSAbstractCompiler";

AmbiguateProperties( AmbiguateProperties(
AbstractCompiler compiler, AbstractCompiler compiler,
char[] reservedFirstCharacters, char[] reservedFirstCharacters,
Expand Down Expand Up @@ -247,7 +241,7 @@ public void process(Node externs, Node root) {


private BitSet getRelatedTypesOnNonUnion(JSType type) { private BitSet getRelatedTypesOnNonUnion(JSType type) {
// All of the types we encounter should have been added to the // All of the types we encounter should have been added to the
// relatedBitsets via computeRelatedTypes. // relatedBitsets via computeRelatedTypesForNonUnionType.
if (relatedBitsets.containsKey(type)) { if (relatedBitsets.containsKey(type)) {
return relatedBitsets.get(type); return relatedBitsets.get(type);
} else { } else {
Expand All @@ -257,39 +251,31 @@ private BitSet getRelatedTypesOnNonUnion(JSType type) {
} }


/** /**
* Adds subtypes - and implementors, in the case of interfaces - of the type * Adds subtypes - and implementors, in the case of interfaces - of the type to its JSTypeBitSet
* to its JSTypeBitSet of related types. Union types are decomposed into their * of related types.
* alternative types.
* *
* <p>The 'is related to' relationship is best understood graphically. Draw an * <p>The 'is related to' relationship is best understood graphically. Draw an arrow from each
* arrow from each instance type to the prototype of each of its * instance type to the prototype of each of its subclass. Draw an arrow from each prototype to
* subclass. Draw an arrow from each prototype to its instance type. Draw an * its instance type. Draw an arrow from each interface to its implementors. A type is related to
* arrow from each interface to its implementors. A type is related to another * another if there is a directed path in the graph from the type to other. Thus, the 'is related
* if there is a directed path in the graph from the type to other. Thus, the * to' relationship is reflexive and transitive.
* 'is related to' relationship is reflexive and transitive.
* *
* <p>Example with Foo extends Bar which extends Baz and Bar implements I: * <p>Example with Foo extends Bar which extends Baz and Bar implements I:
*
* <pre>{@code * <pre>{@code
* Foo -> Bar.prototype -> Bar -> Baz.prototype -> Baz * Foo -> Bar.prototype -> Bar -> Baz.prototype -> Baz
* ^ * ^
* | * |
* I * I
* }</pre> * }</pre>
* *
* <p>Note that we don't need to correctly handle the relationships between * <p>Note that we don't need to correctly handle the relationships between functions, because the
* functions, because the function type is invalidating (i.e. its properties * function type is invalidating (i.e. its properties won't be ambiguated).
* won't be ambiguated).
*/ */
private void computeRelatedTypes(JSType type) { private void computeRelatedTypesForNonUnionType(JSType type) {
if (type.isUnionType()) { // This method could be expanded to handle union types if necessary, but currently no union
type = type.restrictByNotNullOrUndefined(); // types are ever passed as input so the method doesn't have logic for union types
if (type.isUnionType()) { checkState(!type.isUnionType(), type);
for (JSType alt : type.getUnionMembers()) {
computeRelatedTypes(alt);
}
return;
}
}


if (relatedBitsets.containsKey(type)) { if (relatedBitsets.containsKey(type)) {
// We only need to generate the bit set once. // We only need to generate the bit set once.
Expand Down Expand Up @@ -327,7 +313,7 @@ private void addRelatedInstance(FunctionType constructor, JSTypeBitSet related)
"Constructor %s without instance type.", constructor); "Constructor %s without instance type.", constructor);
ObjectType instanceType = constructor.getInstanceType(); ObjectType instanceType = constructor.getInstanceType();
related.set(getIntForType(instanceType.getImplicitPrototype())); related.set(getIntForType(instanceType.getImplicitPrototype()));
computeRelatedTypes(instanceType); computeRelatedTypesForNonUnionType(instanceType);
related.or(relatedBitsets.get(instanceType)); related.or(relatedBitsets.get(instanceType));
} }


Expand Down Expand Up @@ -437,94 +423,116 @@ private class ProcessProperties extends AbstractPostOrderCallback {
@Override @Override
public void visit(NodeTraversal t, Node n, Node parent) { public void visit(NodeTraversal t, Node n, Node parent) {
switch (n.getToken()) { switch (n.getToken()) {
case GETPROP: { case GETPROP:
Node propNode = n.getSecondChild(); processGetProp(n);
JSType type = getJSType(n.getFirstChild());
maybeMarkCandidate(propNode, type);
return; return;
}
case CALL: {
Node target = n.getFirstChild();
if (!target.isQualifiedName()) {
return;
}


String renameFunctionName = target.getOriginalQualifiedName(); case CALL:
if (renameFunctionName != null processCall(n);
&& compiler.getCodingConvention().isPropertyRenameFunction(renameFunctionName)) {
int childCount = n.getChildCount();
if (childCount != 2 && childCount != 3) {
reportInvalidRenameFunction(n, renameFunctionName, WRONG_ARGUMENT_COUNT);
return;
}

Node propName = n.getSecondChild();
if (!propName.isString()) {
reportInvalidRenameFunction(n, renameFunctionName, WANT_STRING_LITERAL);
return;
}

if (propName.getString().contains(".")) {
reportInvalidRenameFunction(n, renameFunctionName, DO_NOT_WANT_PATH);
return;
}

maybeMarkCandidate(propName, getJSType(n.getSecondChild()));
} else if (NodeUtil.isObjectDefinePropertiesDefinition(n)) {
Node typeObj = n.getSecondChild();
JSType type = getJSType(typeObj);
Node objectLiteral = typeObj.getNext();

if (!objectLiteral.isObjectLit()) {
return;
}

for (Node key : objectLiteral.children()) {
if (key.isQuotedString()) {
quotedNames.add(key.getString());
} else {
maybeMarkCandidate(key, type);
}
}
}
return; return;
}
case OBJECTLIT:
// Object.defineProperties literals are handled at the CALL node.
if (n.getParent().isCall()
&& NodeUtil.isObjectDefinePropertiesDefinition(n.getParent())) {
return;
}


// The children of an OBJECTLIT node are keys, where the values case OBJECTLIT:
// are the children of the keys. processObjectLit(n);
JSType type = getJSType(n);
for (Node key = n.getFirstChild(); key != null; key = key.getNext()) {
// We only want keys that were unquoted.
// Keys are STRING, GET, SET
if (key.isQuotedString()) {
// Ensure that we never rename some other property in a way
// that could conflict with this quoted key.
quotedNames.add(key.getString());
} else {
maybeMarkCandidate(key, type);
}
}
return; return;

case GETELEM: case GETELEM:
// If this is a quoted property access (e.g. x['myprop']), we need to processGetElem(n);
// ensure that we never rename some other property in a way that
// could conflict with this quoted name.
Node child = n.getLastChild();
if (child.isString()) {
quotedNames.add(child.getString());
}
return; return;

default: default:
// Nothing to do. // Nothing to do.
} }
} }


private void processGetProp(Node getProp) {
Node propNode = getProp.getSecondChild();
JSType type = getJSType(getProp.getFirstChild());
maybeMarkCandidate(propNode, type);
}

private void processCall(Node call) {
Node target = call.getFirstChild();
if (!target.isQualifiedName()) {
return;
}

String renameFunctionName = target.getOriginalQualifiedName();
if (renameFunctionName != null
&& compiler.getCodingConvention().isPropertyRenameFunction(renameFunctionName)) {
int childCount = call.getChildCount();
if (childCount != 2 && childCount != 3) {
reportInvalidRenameFunction(call, renameFunctionName, WRONG_ARGUMENT_COUNT);
return;
}

Node propName = call.getSecondChild();
if (!propName.isString()) {
reportInvalidRenameFunction(call, renameFunctionName, WANT_STRING_LITERAL);
return;
}

if (propName.getString().contains(".")) {
reportInvalidRenameFunction(call, renameFunctionName, DO_NOT_WANT_PATH);
return;
}

// Skip ambiguation for properties in renaming calls
// NOTE (lharker@) - I'm not sure if this behavior is necessary, or if we could safely
// ambiguate the property as long as we also updated the property renaming call
Property p = getProperty(propName.getString());
p.skipAmbiguating = true;
} else if (NodeUtil.isObjectDefinePropertiesDefinition(call)) {
Node typeObj = call.getSecondChild();
JSType type = getJSType(typeObj);
Node objectLiteral = typeObj.getNext();

if (!objectLiteral.isObjectLit()) {
return;
}

for (Node key : objectLiteral.children()) {
if (key.isQuotedString()) {
quotedNames.add(key.getString());
} else {
maybeMarkCandidate(key, type);
}
}
}
}

private void processObjectLit(Node objectLit) {
// Object.defineProperties literals are handled at the CALL node.
if (objectLit.getParent().isCall()
&& NodeUtil.isObjectDefinePropertiesDefinition(objectLit.getParent())) {
return;
}

// The children of an OBJECTLIT node are keys, where the values
// are the children of the keys.
JSType type = getJSType(objectLit);
for (Node key = objectLit.getFirstChild(); key != null; key = key.getNext()) {
// We only want keys that were unquoted.
// Keys are STRING, GET, SET
if (key.isQuotedString()) {
// Ensure that we never rename some other property in a way
// that could conflict with this quoted key.
quotedNames.add(key.getString());
} else {
maybeMarkCandidate(key, type);
}
}
}

private void processGetElem(Node n) {
// If this is a quoted property access (e.g. x['myprop']), we need to
// ensure that we never rename some other property in a way that
// could conflict with this quoted name.
Node child = n.getLastChild();
if (child.isString()) {
quotedNames.add(child.getString());
}
}

/** /**
* If a property node is eligible for renaming, stashes a reference to it * If a property node is eligible for renaming, stashes a reference to it
* and increments the property name's access count. * and increments the property name's access count.
Expand Down Expand Up @@ -560,16 +568,9 @@ private Property getProperty(String name) {
* present. * present.
*/ */
private JSType getJSType(Node n) { private JSType getJSType(Node n) {
if (n == null) {
return compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE);
}

JSType type = n.getJSType(); JSType type = n.getJSType();
if (type == null) { if (type == null) {
// TODO(user): This branch indicates a compiler bug, not worthy of // TODO(bradfordcsmith): This branch indicates a compiler bug. It should throw an exception.
// halting the compilation but we should log this and analyze to track
// down why it happens. This is not critical and will be resolved over
// time as the type checker is extended.
return compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE); return compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE);
} else { } else {
return type; return type;
Expand All @@ -586,11 +587,6 @@ private class Property {


Property(String name) { Property(String name) {
this.oldName = name; this.oldName = name;

// Properties with this suffix are handled in RenameProperties.
if (name.startsWith(SKIP_PREFIX)) {
skipAmbiguating = true;
}
} }


/** Add this type to this property, calculating */ /** Add this type to this property, calculating */
Expand Down Expand Up @@ -619,7 +615,7 @@ private void addNonUnionType(JSType newType) {
return; return;
} }
if (!relatedTypes.get(getIntForType(newType))) { if (!relatedTypes.get(getIntForType(newType))) {
computeRelatedTypes(newType); computeRelatedTypesForNonUnionType(newType);
relatedTypes.or(getRelatedTypesOnNonUnion(newType)); relatedTypes.or(getRelatedTypesOnNonUnion(newType));
} }
} }
Expand All @@ -633,10 +629,6 @@ private JSTypeBitSet(int size) {
super(size); super(size);
} }


private JSTypeBitSet() {
super();
}

/** /**
* Pretty-printing, for diagnostic purposes. * Pretty-printing, for diagnostic purposes.
*/ */
Expand Down

0 comments on commit db49787

Please sign in to comment.