Skip to content

Commit 96a8e2a

Browse files
brad4dcopybara-github
authored andcommitted
GlobalNamespace: handle properties defined in nested object literals correctly
A previous fix prevented incorrect collapsing of cases like this: ``` const X = typeof OtherThing != 'undefined' ? OtherThing : {prop: 1}; ``` However, it didn't handle nested object literals correctly. ``` const X = typeof OtherThing != 'undefined' ? OtherThing : {prop: {nested: 1}}; ``` This change expands the fix to the more general case. PiperOrigin-RevId: 501412585
1 parent bc9b6a2 commit 96a8e2a

File tree

3 files changed

+61
-14
lines changed

3 files changed

+61
-14
lines changed

src/com/google/javascript/jscomp/GlobalNamespace.java

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ static SourceKind fromScriptNode(Node n) {
137137
* @param root The root of the rest of the code to build a namespace for.
138138
*/
139139
GlobalNamespace(AbstractCompiler compiler, Node root) {
140-
this(/* decisionsLog = */ null, compiler, null, root);
140+
this(/* decisionsLog= */ null, compiler, null, root);
141141
}
142142

143143
/**
@@ -1173,25 +1173,25 @@ private enum DeclaredTypeKind {
11731173
*/
11741174
enum Inlinability {
11751175
INLINE_COMPLETELY(
1176-
/* shouldInlineUsages = */ true,
1176+
/* shouldInlineUsages= */ true,
11771177
/* shouldRemoveDeclaration= */ true,
1178-
/* canCollapse = */ true),
1178+
/* canCollapse= */ true),
11791179
INLINE_BUT_KEEP_DECLARATION_ENUM(
1180-
/* shouldInlineUsages = */ true,
1180+
/* shouldInlineUsages= */ true,
11811181
/* shouldRemoveDeclaration= */ false,
1182-
/* canCollapse = */ true),
1182+
/* canCollapse= */ true),
11831183
INLINE_BUT_KEEP_DECLARATION_INTERFACE(
1184-
/* shouldInlineUsages = */ true,
1184+
/* shouldInlineUsages= */ true,
11851185
/* shouldRemoveDeclaration= */ false,
1186-
/* canCollapse = */ true),
1186+
/* canCollapse= */ true),
11871187
INLINE_BUT_KEEP_DECLARATION_CLASS(
1188-
/* shouldInlineUsages = */ true,
1188+
/* shouldInlineUsages= */ true,
11891189
/* shouldRemoveDeclaration= */ false,
1190-
/* canCollapse = */ true),
1190+
/* canCollapse= */ true),
11911191
DO_NOT_INLINE(
1192-
/* shouldInlineUsages = */ false,
1192+
/* shouldInlineUsages= */ false,
11931193
/* shouldRemoveDeclaration= */ false,
1194-
/* canCollapse = */ false);
1194+
/* canCollapse= */ false);
11951195

11961196
private final boolean shouldInlineUsages;
11971197
private final boolean shouldRemoveDeclaration;
@@ -1771,8 +1771,10 @@ Inlinability canCollapseOrInline() {
17711771
logDecision(Inlinability.DO_NOT_INLINE, "obj lit property followed by spread");
17721772
return Inlinability.DO_NOT_INLINE;
17731773
}
1774-
Node grandparent = declaration.getGrandparent();
1775-
if (grandparent.isOr() || grandparent.isHook()) {
1774+
// We may be in a deeply nested object literal like, `{a: {b: {c: 1}}}`, so find the
1775+
// outermost object literal node in order to determine whether it is used conditionally.
1776+
final Node objectLitParent = getOutermostObjectLit(declaration).getParent();
1777+
if (objectLitParent.isOr() || objectLitParent.isHook()) {
17761778
// Case: `var x = y || {a: b}` or `var x = cond ? y : {a: b}`.
17771779
logDecision(Inlinability.DO_NOT_INLINE, "conditional definition");
17781780
return Inlinability.DO_NOT_INLINE;
@@ -2191,6 +2193,26 @@ boolean isModuleExport() {
21912193
}
21922194
}
21932195

2196+
/**
2197+
* Given something like the `'c'` STRING_KEY node in `x = {a: {b: {c: 0}}};`, return the Node for
2198+
* the outermost object literal.
2199+
*
2200+
* @param objLitKey Must be the child of an OBJECT_LIT node
2201+
* @return The first ancestor that is an OBJECT_LIT and whose grandparent is not an OBJECT_LIT
2202+
*/
2203+
private Node getOutermostObjectLit(Node objLitKey) {
2204+
Node outermostObjectLit = objLitKey.getParent();
2205+
checkState(outermostObjectLit.isObjectLit(), outermostObjectLit);
2206+
while (true) {
2207+
final Node objLitGrandparent = outermostObjectLit.getGrandparent();
2208+
if (objLitGrandparent != null && objLitGrandparent.isObjectLit()) {
2209+
outermostObjectLit = objLitGrandparent;
2210+
} else {
2211+
return outermostObjectLit;
2212+
}
2213+
}
2214+
}
2215+
21942216
/**
21952217
* True if the given Node is the GETPROP in a statement like `some.q.name;`
21962218
*
@@ -2400,7 +2422,7 @@ static ObjLitStringKeyAnalysis forObjLitAssignment(String nameString, NameType n
24002422
/** The object literal key does not represent a qualified name assignment. */
24012423
static ObjLitStringKeyAnalysis forNonReference() {
24022424
return new AutoValue_GlobalNamespace_ObjLitStringKeyAnalysis(
2403-
/* nameString = */ null, NameType.OTHER);
2425+
/* nameString= */ null, NameType.OTHER);
24042426
}
24052427
}
24062428
}

test/com/google/javascript/jscomp/CollapsePropertiesTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4420,6 +4420,11 @@ public void testTernaryExpression() {
44204420
// right branch, write to undeclared prop
44214421
test(
44224422
srcs("var a = p ? x : {b: 1}; a.c = 2;"), expected("var a = p ? x : {b: 1}; var a$c = 2;"));
4423+
4424+
// right branch, use nested prop, alias is inlined but prop is not collapsed.
4425+
test(
4426+
srcs(" var a = p ? x : {b: { insideB: 1 }}; var t = a.b.insideB; use( t);"),
4427+
expected("var a = p ? x : {b: { insideB: 1 }}; var t = null ; use(a.b.insideB);"));
44234428
}
44244429

44254430
@Test

test/com/google/javascript/jscomp/GlobalNamespaceTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1061,6 +1061,26 @@ public void testCannotCollapseAliasedObjectLitProperty() {
10611061
assertThat(fooProp.canCollapse()).isFalse();
10621062
}
10631063

1064+
@Test
1065+
public void testCannotCollapseConditionalObjectLitProperty() {
1066+
GlobalNamespace namespace = parse("var foo = x || {prop: 0}; use(foo.prop);");
1067+
1068+
Name fooProp = namespace.getSlot("foo.prop");
1069+
1070+
// We should not convert foo.prop -> foo$prop because use(foo) might read foo.prop
1071+
assertThat(fooProp.canCollapse()).isFalse();
1072+
}
1073+
1074+
@Test
1075+
public void testCannotCollapseConditionalObjectLitNestedProperty() {
1076+
GlobalNamespace namespace = parse("var foo = x || {prop: {nested: 0}}; use(foo.prop.nested);");
1077+
1078+
Name fooProp = namespace.getSlot("foo.prop.nested");
1079+
1080+
// We should not convert foo.prop -> foo$prop because use(foo) might read foo.prop
1081+
assertThat(fooProp.canCollapse()).isFalse();
1082+
}
1083+
10641084
@Test
10651085
public void testGitHubIssue3733() {
10661086
GlobalNamespace namespace =

0 commit comments

Comments
 (0)