From 28fb95060cd607de3c8bde550908831705639ebf Mon Sep 17 00:00:00 2001 From: lharker Date: Thu, 16 Aug 2018 10:22:42 -0700 Subject: [PATCH] Fix some spurious warnings on destructuring in CheckGlobalNames ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=209004728 --- .../javascript/jscomp/CheckGlobalNames.java | 56 ++++++++++------ .../javascript/jscomp/GlobalNamespace.java | 12 +++- .../jscomp/CheckGlobalNamesTest.java | 67 ++++++++++++++++++- 3 files changed, 109 insertions(+), 26 deletions(-) diff --git a/src/com/google/javascript/jscomp/CheckGlobalNames.java b/src/com/google/javascript/jscomp/CheckGlobalNames.java index fff24e7371b..cbc5b9b7725 100644 --- a/src/com/google/javascript/jscomp/CheckGlobalNames.java +++ b/src/com/google/javascript/jscomp/CheckGlobalNames.java @@ -248,43 +248,33 @@ private boolean propertyMustBeInitializedByFullName(Name name) { return false; } - boolean parentIsAliased = false; - if (name.parent.aliasingGets > 0) { - for (Ref ref : name.parent.getRefs()) { - if (ref.type == Ref.Type.ALIASING_GET) { - Node aliaser = ref.getNode().getParent(); - - // We don't need to worry about known aliased, because - // they're already covered by the getIndirectlyDeclaredProperties - // call at the top. - boolean isKnownAlias = - aliaser.isCall() && - (convention.getClassesDefinedByCall(aliaser) != null || - convention.getSingletonGetterClassName(aliaser) != null); - if (!isKnownAlias) { - parentIsAliased = true; - } - } - } - } - - if (parentIsAliased) { + if (isNameUnsafelyAliased(name.parent)) { + // e.g. if we have `const ns = {}; escape(ns); alert(ns.a.b);` + // we don't expect ns.a.b to be defined somewhere because `ns` has escaped return false; } if (objectPrototypeProps.contains(name.getBaseName())) { + // checks for things on Object.prototype, e.g. a call to + // something.hasOwnProperty('a'); return false; } if (name.parent.type == Name.Type.OBJECTLIT) { + // if this is a property on an object literal, always expect an initialization somewhere return true; } if (name.parent.type == Name.Type.CLASS) { + // only warn on class properties if there is no superclass, because we don't handle + // class side inheritance here very well yet. return !hasSuperclass(name.parent); } - return name.parent.type == Name.Type.FUNCTION && name.parent.isDeclaredType() + // warn on remaining names if they are on a constructor and are not a Function.prototype + // property (e.g. f.call(1);) + return name.parent.type == Name.Type.FUNCTION + && name.parent.isDeclaredType() && !functionPrototypeProps.contains(name.getBaseName()); } @@ -297,4 +287,26 @@ private boolean hasSuperclass(Name es6Class) { return !superclass.isEmpty(); } + + private boolean isNameUnsafelyAliased(Name name) { + if (name.aliasingGets > 0) { + for (Ref ref : name.getRefs()) { + if (ref.type == Ref.Type.ALIASING_GET) { + Node aliaser = ref.getNode().getParent(); + + // We don't need to worry about known aliased, because + // they're already covered by the getIndirectlyDeclaredProperties + // call at the top. + boolean isKnownAlias = + aliaser.isCall() + && (convention.getClassesDefinedByCall(aliaser) != null + || convention.getSingletonGetterClassName(aliaser) != null); + if (!isKnownAlias) { + return true; + } + } + } + } + return name.parent != null && isNameUnsafelyAliased(name.parent); + } } diff --git a/src/com/google/javascript/jscomp/GlobalNamespace.java b/src/com/google/javascript/jscomp/GlobalNamespace.java index be50d29fa9f..dcf03cec495 100644 --- a/src/com/google/javascript/jscomp/GlobalNamespace.java +++ b/src/com/google/javascript/jscomp/GlobalNamespace.java @@ -402,10 +402,16 @@ public void collect(JSModule module, Scope scope, Node n) { type = Name.Type.CLASS; } break; + case STRING_KEY: case ARRAY_PATTERN: - // Specific case to handle inlining with array destructuring - isSet = true; - type = Name.Type.OTHER; + case DEFAULT_VALUE: + case COMPUTED_PROP: + case REST: + // This may be a set. + if (NodeUtil.isLhsByDestructuring(n)) { + isSet = true; + type = Name.Type.OTHER; + } break; default: if (NodeUtil.isAssignmentOp(parent) && parent.getFirstChild() == n) { diff --git a/test/com/google/javascript/jscomp/CheckGlobalNamesTest.java b/test/com/google/javascript/jscomp/CheckGlobalNamesTest.java index a338bb836db..795e5b99cd1 100644 --- a/test/com/google/javascript/jscomp/CheckGlobalNamesTest.java +++ b/test/com/google/javascript/jscomp/CheckGlobalNamesTest.java @@ -415,7 +415,8 @@ public void testCustomObjectPrototypeProperties() { } public void testFunctionPrototypeProperties() { - testSame("var x = {}; var y = x.hasOwnProperty('z');"); + // don't warn for "Foo.call" + testSame("/** @constructor */ function Foo() {}; Foo.call({});"); } public void testIndirectlyDeclaredProperties() { @@ -489,4 +490,68 @@ public void testEs6NonSubclass_stillWarnsForMissingProperty() { // We still do warn for undefined properties on an ES6 class with no superclass testWarning("class Child {} Child.f();", UNDEFINED_NAME_WARNING); } + + public void testObjectDestructuringAlias() { + testSame( + lines( + "var ns = {};", + "/** @enum */", + "ns.AdjustMode = {SELECT: 0, MOVE: 1};", + "const {AdjustMode} = ns;", + "alert(AdjustMode.SELECT);")); + } + + public void testObjectDestructuringAlias_computedProperty() { + testSame( + lines( + "var ns = {};", + "/** @enum */", + "ns.AdjustMode = {SELECT: 0, MOVE: 1};", + "const {['AdjustMode']: AdjustMode} = ns;", + "alert(AdjustMode.SELECT);")); + } + + public void testObjectDestructuringAlias_defaultValue() { + testSame( + lines( + "var ns = {};", + "/** @enum */", + "ns.AdjustMode = {SELECT: 0, MOVE: 1};", + "const {AdjustMode = {}} = ns;", + "alert(AdjustMode.SELECT);")); + } + + public void testArrayDestructuring() { + // Currently, we never issue warnings for property access on an object created through + // destructuring. + testSame(lines("const [ns] = [{foo: 3}];", "alert(ns.foo);", "alert(ns.bar);")); // undefined + } + + public void testArrayDestructuring_rest() { + testSame( + lines( + "const [...ns] = [{foo: 3}];", // + "alert(ns[0].foo);", + "alert(ns[0].bar);")); + } + + public void testPropertyCreationOnDestructuringAlias() { + testSame( + lines( + "var ns = {};", + "ns.AdjustMode = {SELECT: 0, MOVE: 1};", + "const {AdjustMode} = ns;", + "AdjustMode.OTHER = 2;", + "alert(ns.AdjustMode.OTHER);")); + } + + public void testNamespaceAliasPreventsWarning() { + // When a variable is aliased, we back off warning for potentially missing properties on that + // variable. + testSame( + lines( + "var a = {};", + "var alias = a;", + "alert(a.b.c.d);")); // This will cause a runtime error but not a compiler warning. + } }