diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java index f929626e5e1..d610fb66c1f 100644 --- a/src/com/google/javascript/jscomp/AmbiguateProperties.java +++ b/src/com/google/javascript/jscomp/AmbiguateProperties.java @@ -79,9 +79,9 @@ class AmbiguateProperties implements CompilerPass { private final AbstractCompiler compiler; private final List stringNodesToRename = new ArrayList<>(); - // Can't use these as property names. + // Can't use these to start property names. private final char[] reservedFirstCharacters; - // Can't use these as property names. + // Can't use these at all in property names. private final char[] reservedNonFirstCharacters; /** Map from property name to Property object */ @@ -120,12 +120,6 @@ public int compare(Property p1, Property p2) { /** A set of types that invalidate properties from ambiguation. */ 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( AbstractCompiler compiler, char[] reservedFirstCharacters, @@ -247,7 +241,7 @@ public void process(Node externs, Node root) { private BitSet getRelatedTypesOnNonUnion(JSType type) { // All of the types we encounter should have been added to the - // relatedBitsets via computeRelatedTypes. + // relatedBitsets via computeRelatedTypesForNonUnionType. if (relatedBitsets.containsKey(type)) { return relatedBitsets.get(type); } else { @@ -257,18 +251,17 @@ private BitSet getRelatedTypesOnNonUnion(JSType type) { } /** - * Adds subtypes - and implementors, in the case of interfaces - of the type - * to its JSTypeBitSet of related types. Union types are decomposed into their - * alternative types. + * Adds subtypes - and implementors, in the case of interfaces - of the type to its JSTypeBitSet + * of related types. * - *

The 'is related to' relationship is best understood graphically. Draw an - * arrow from each instance type to the prototype of each of its - * subclass. Draw an arrow from each prototype to its instance type. Draw an - * arrow from each interface to its implementors. A type is related to another - * if there is a directed path in the graph from the type to other. Thus, the - * 'is related to' relationship is reflexive and transitive. + *

The 'is related to' relationship is best understood graphically. Draw an arrow from each + * instance type to the prototype of each of its subclass. Draw an arrow from each prototype to + * its instance type. Draw an arrow from each interface to its implementors. A type is related to + * another if there is a directed path in the graph from the type to other. Thus, the 'is related + * to' relationship is reflexive and transitive. * *

Example with Foo extends Bar which extends Baz and Bar implements I: + * * 

{@code
    * Foo -> Bar.prototype -> Bar -> Baz.prototype -> Baz
    *                          ^
@@ -276,20 +269,13 @@ private BitSet getRelatedTypesOnNonUnion(JSType type) {
    *                          I
    * }
* - *

Note that we don't need to correctly handle the relationships between - * functions, because the function type is invalidating (i.e. its properties - * won't be ambiguated). + *

Note that we don't need to correctly handle the relationships between functions, because the + * function type is invalidating (i.e. its properties won't be ambiguated). */ - private void computeRelatedTypes(JSType type) { - if (type.isUnionType()) { - type = type.restrictByNotNullOrUndefined(); - if (type.isUnionType()) { - for (JSType alt : type.getUnionMembers()) { - computeRelatedTypes(alt); - } - return; - } - } + private void computeRelatedTypesForNonUnionType(JSType type) { + // This method could be expanded to handle union types if necessary, but currently no union + // types are ever passed as input so the method doesn't have logic for union types + checkState(!type.isUnionType(), type); if (relatedBitsets.containsKey(type)) { // We only need to generate the bit set once. @@ -327,7 +313,7 @@ private void addRelatedInstance(FunctionType constructor, JSTypeBitSet related) "Constructor %s without instance type.", constructor); ObjectType instanceType = constructor.getInstanceType(); related.set(getIntForType(instanceType.getImplicitPrototype())); - computeRelatedTypes(instanceType); + computeRelatedTypesForNonUnionType(instanceType); related.or(relatedBitsets.get(instanceType)); } @@ -437,94 +423,116 @@ private class ProcessProperties extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { switch (n.getToken()) { - case GETPROP: { - Node propNode = n.getSecondChild(); - JSType type = getJSType(n.getFirstChild()); - maybeMarkCandidate(propNode, type); + case GETPROP: + processGetProp(n); return; - } - case CALL: { - Node target = n.getFirstChild(); - if (!target.isQualifiedName()) { - return; - } - String renameFunctionName = target.getOriginalQualifiedName(); - if (renameFunctionName != null - && 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); - } - } - } + case CALL: + processCall(n); 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 - // are the children of the keys. - 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); - } - } + case OBJECTLIT: + processObjectLit(n); return; + case GETELEM: - // 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()); - } + processGetElem(n); return; + default: // 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 * and increments the property name's access count. @@ -560,16 +568,9 @@ private Property getProperty(String name) { * present. */ private JSType getJSType(Node n) { - if (n == null) { - return compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE); - } - JSType type = n.getJSType(); if (type == null) { - // TODO(user): This branch indicates a compiler bug, not worthy of - // 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. + // TODO(bradfordcsmith): This branch indicates a compiler bug. It should throw an exception. return compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE); } else { return type; @@ -586,11 +587,6 @@ private class Property { Property(String 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 */ @@ -619,7 +615,7 @@ private void addNonUnionType(JSType newType) { return; } if (!relatedTypes.get(getIntForType(newType))) { - computeRelatedTypes(newType); + computeRelatedTypesForNonUnionType(newType); relatedTypes.or(getRelatedTypesOnNonUnion(newType)); } } @@ -633,10 +629,6 @@ private JSTypeBitSet(int size) { super(size); } - private JSTypeBitSet() { - super(); - } - /** * Pretty-printing, for diagnostic purposes. */ diff --git a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java index 9b2bb4ee4cf..87989f7b1d5 100644 --- a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.javascript.jscomp.DisambiguateProperties.Warnings; import com.google.javascript.rhino.Node; import java.util.HashMap; import java.util.Map; @@ -176,25 +177,56 @@ public void testTwoTypesTwoVar() { } @Test - public void testUnion() { - String js = lines( - "/** @constructor */ var Foo = function(){};", - "/** @constructor */ var Bar = function(){};", - "Foo.prototype.foodoo=0;", - "Bar.prototype.bardoo=0;", - "/** @type {Foo|Bar} */", - "var U = any();", - "U.joint;", - "U.joint"); - String output = lines( - "/** @constructor */ var Foo = function(){};", - "/** @constructor */ var Bar = function(){};", - "Foo.prototype.b=0;", - "Bar.prototype.b=0;", - "/** @type {Foo|Bar} */", - "var U = any();", - "U.a;", - "U.a"); + public void testUnion_withUnrelatedPropertyAccess() { + String js = + lines( + "/** @constructor */ var Foo = function(){};", + "/** @constructor */ var Bar = function(){};", + "Foo.prototype.foodoo=0;", + "Bar.prototype.bardoo=0;", + // variable exists that could be a Foo or a Bar + "/** @type {Foo|Bar} */", + "var U = any();", + // We don't actually access either foodoo or bardoo on it, + // though, so it's OK if they end up having the same name + "U.joint"); + String output = + lines( + "/** @constructor */ var Foo = function(){};", + "/** @constructor */ var Bar = function(){};", + "Foo.prototype.a=0;", + "Bar.prototype.a=0;", + "/** @type {Foo|Bar} */", + "var U = any();", + "U.b"); + test(js, output); + } + + @Test + public void testUnion_withRelatedPropertyAccess() { + String js = + lines( + "/** @constructor */ var Foo = function(){};", + "/** @constructor */ var Bar = function(){};", + "Foo.prototype.foodoo=0;", + "Bar.prototype.bardoo=0;", + // variable exists that could be a Foo or a Bar + "/** @type {Foo|Bar} */", + "var U = any();", + // both foodoo and bardoo are accessed through that variable, + // so they must have different names. + "U.foodoo;", + "U.bardoo"); + String output = + lines( + "/** @constructor */ var Foo = function(){};", + "/** @constructor */ var Bar = function(){};", + "Foo.prototype.b=0;", + "Bar.prototype.a=0;", + "/** @type {Foo|Bar} */", + "var U = any();", + "U.b;", + "U.a"); test(js, output); } @@ -1193,4 +1225,44 @@ public void testDontRenamePrototypeWithoutExterns() { test(externs(""), srcs(js), expected(output)); } + + @Test + public void testInvalidRenameFunction_withZeroArgs_causesWarning() { + testError("const p = JSCompiler_renameProperty()", Warnings.INVALID_RENAME_FUNCTION); + } + + @Test + public void testInvalidRenameFunction_withThreeArgs_causesWarning() { + testError("const p = JSCompiler_renameProperty('foo', 0, 1)", Warnings.INVALID_RENAME_FUNCTION); + } + + @Test + public void testInvalidRenameFunction_withNonStringArg_causesWarning() { + testError("const p = JSCompiler_renameProperty(0)", Warnings.INVALID_RENAME_FUNCTION); + } + + @Test + public void testInvalidRenameFunction_withPropertyRefInFirstArg_causesWarning() { + testError("const p = JSCompiler_renameProperty('a.b')", Warnings.INVALID_RENAME_FUNCTION); + } + + @Test + public void testJSCompiler_renameProperty_twoArgs_blocksAmbiguation() { + testSame( + lines( + "/** @constructor */", + "function Foo() {}", + "Foo.prototype.bar = 3;", + "const barName = JSCompiler_renameProperty('bar', Foo);")); + } + + @Test + public void testJSCompiler_renameProperty_oneArg_blocksAmbiguation() { + testSame( + lines( + "/** @constructor */", + "function Foo() {}", + "Foo.prototype.bar = 3;", + "const barName = JSCompiler_renameProperty('bar');")); + } }