Skip to content

Commit

Permalink
InlineAndCollapseProperties fix for new TS 5.2 emit shape
Browse files Browse the repository at this point in the history
TS 5.2. emits code like this:

```
// Alias variable created without being initialized.
var alias;
var C = class {
  // references to alias in here, because TS doesn't like using
  // the class name.
};
alias = C;
C.staticProperty = someValue;
```

This code shape confused `InlineAndCollapseProperties`
causing it both to back off from some inlining (bigger output code)
and fail to back off similarly for collapsing (broken output code).

PiperOrigin-RevId: 564552797
  • Loading branch information
brad4d authored and copybara-github committed Sep 12, 2023
1 parent 35345da commit d4da7b3
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 31 deletions.
47 changes: 36 additions & 11 deletions src/com/google/javascript/jscomp/GlobalNamespace.java
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,7 @@ private boolean declarationHasFollowingObjectSpreadSibling(Node declaration) {
private class BuildGlobalNamespace extends NodeTraversal.AbstractPreOrderCallback {
private @Nullable Node curModuleRoot = null;
private @Nullable ModuleMetadata curMetadata = null;

/** Collect the references in pre-order. */
@Override
public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
Expand Down Expand Up @@ -1216,9 +1217,13 @@ final class Name implements StaticSlot {

// The children of this name. Must be null if there are no children.
@Nullable List<Name> props;
/** The first global assignment to a name. */

/** The declaration of a name. */
private @Nullable Ref declaration;

/** The first global assignment to a name. */
private @Nullable Ref initialization;

/**
* Keep track of which Nodes are Refs for this Name.
*
Expand Down Expand Up @@ -1311,6 +1316,10 @@ boolean usesHasOwnProperty() {
return declaration;
}

public @Nullable Ref getInitialization() {
return initialization;
}

boolean isFunction() {
return getBooleanProperty(NameProp.FUNCTION);
}
Expand Down Expand Up @@ -1455,6 +1464,12 @@ private void updateStateForAddedRef(Ref ref) {
if (declaration == null) {
declaration = ref;
}
if (initialization == null && !ref.isUninitializedDeclaration()) {
// Record the reference where the first value is actually assigned.
// Do not include the automatically-assigned `undefined` value case.
// e.g. `var name;`
initialization = ref;
}
if (firstDeclarationJSDocInfo == null) {
// JSDocInfo from the first SET_FROM_GLOBAL will be assumed to be canonical
// Note that this will not change if the first declaration is later removed
Expand Down Expand Up @@ -1725,7 +1740,8 @@ boolean isCollapsingExplicitlyDenied() {
*/
Inlinability calculateInlinability() {
// Only simple aliases with direct usage are inlineable.
if (inExterns() || globalSets != 1 || localSets != 0) {
// Exactly 2 global sets could be OK, if the first is the declaration with no initializer.
if (inExterns() || !hasOneRealGlobalSet() || localSets != 0) {
return Inlinability.DO_NOT_INLINE;
}

Expand Down Expand Up @@ -1761,6 +1777,10 @@ Inlinability calculateInlinability() {
return collapsibility;
}

private boolean hasOneRealGlobalSet() {
return globalSets == 1 || (globalSets == 2 && declaration.isUninitializedDeclaration());
}

boolean canCollapse() {
return canCollapseOrInline().canCollapse();
}
Expand Down Expand Up @@ -1914,13 +1934,13 @@ private void logDecision(Inlinability inlinability, String reason) {

private Inlinability getUnsafeInlinabilityBasedOnDeclaredType() {
if (this.getBooleanProperty(NameProp.CONSTRUCTOR_TYPE)) {
return Inlinability.INLINE_BUT_KEEP_DECLARATION_CLASS;
return Inlinability.INLINE_BUT_KEEP_DECLARATION_CLASS;
} else if (this.getBooleanProperty(NameProp.INTERFACE_TYPE)) {
return Inlinability.INLINE_BUT_KEEP_DECLARATION_INTERFACE;
return Inlinability.INLINE_BUT_KEEP_DECLARATION_INTERFACE;
} else if (this.getBooleanProperty(NameProp.ENUM_TYPE)) {
return Inlinability.INLINE_BUT_KEEP_DECLARATION_ENUM;
return Inlinability.INLINE_BUT_KEEP_DECLARATION_ENUM;
} else if (this.getBooleanProperty(NameProp.NOT_A_TYPE)) {
return Inlinability.DO_NOT_INLINE;
return Inlinability.DO_NOT_INLINE;
}
throw new IllegalStateException(
SimpleFormat.format("name missing declaredType value: %s", this));
Expand Down Expand Up @@ -2133,11 +2153,11 @@ private boolean valueImplicitlySupportsAliasing() {
return true;
}
if (getBooleanProperty(NameProp.FUNCTION)) {
// We want ES5 ctors/interfaces to behave consistently with ES6 because:
// - transpilation should not change behaviour
// - updating code shouldn't be hindered by behaviour changes
@Nullable JSDocInfo jsdoc = getJSDocInfo();
return jsdoc != null && jsdoc.isConstructorOrInterface();
// We want ES5 ctors/interfaces to behave consistently with ES6 because:
// - transpilation should not change behaviour
// - updating code shouldn't be hindered by behaviour changes
@Nullable JSDocInfo jsdoc = getJSDocInfo();
return jsdoc != null && jsdoc.isConstructorOrInterface();
}
return false;
}
Expand Down Expand Up @@ -2472,6 +2492,11 @@ boolean isSetFromGlobal() {
return this.type == Type.SET_FROM_GLOBAL || this.type == Type.GET_AND_SET_FROM_GLOBAL;
}

/** True for `var name;` and similar cases. */
boolean isUninitializedDeclaration() {
return node.isName() && NodeUtil.isNameDeclaration(node.getParent()) && !node.hasChildren();
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -835,8 +835,8 @@ private void inlineGlobalAliasIfPossible(Name name, Ref alias, GlobalNamespace n

if (aliasInlinability.shouldRemoveDeclaration()) {
// Rewrite the initialization of the alias.
Ref aliasDeclaration = aliasingName.getDeclaration();
if (aliasDeclaration.isTwin()) {
Ref aliasInitialization = aliasingName.getInitialization();
if (aliasInitialization.isTwin()) {
// This is in a nested assign.
// Replace
// a.b = aliasing.name = aliased.name
Expand All @@ -846,7 +846,7 @@ private void inlineGlobalAliasIfPossible(Name name, Ref alias, GlobalNamespace n
Node aliasGrandparent = aliasParent.getParent();
aliasParent.replaceWith(alias.getNode().detach());
// Remove the ref to 'aliasing.name' entirely
aliasingName.removeRef(aliasDeclaration);
aliasingName.removeRef(aliasInitialization);
// Force GlobalNamespace to revisit the new reference to 'aliased.name' and update its
// internal state.
newNodes.add(new AstChange(alias.scope, alias.getNode()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ public void testTs52OutputChange() {
" method2() {",
" return alias.staticPropOnAlias;",
" }",
"}",
"};",
"alias = module$exports$C;",
"(() => {",
" alias.staticPropOnAlias = 1",
" alias.staticPropOnAlias = 1;",
"})();",
"module$exports$C.staticPropOnC = 2;",
"")),
Expand All @@ -96,19 +96,18 @@ public void testTs52OutputChange() {
"var alias;",
"var module$exports$C = class {",
" method1() {",
" return alias.staticPropOnC;",
" return module$exports$C$staticPropOnC;",
" }",
" method2() {",
" return alias.staticPropOnAlias;",
" return module$exports$C$staticPropOnAlias;",
" }",
"}",
"alias = module$exports$C;",
"};",
"var module$exports$C$staticPropOnAlias;",
"alias = null;",
"(() => {",
" alias.staticPropOnAlias = 1",
" module$exports$C$staticPropOnAlias = 1;",
"})();",
// TODO : b/299055739 - bad collapse
"var module$exports$C$staticPropOnC = 2;",
"",
"")));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,12 @@ public void logicalAssignmentPropertyReferenceNotTranspiledOutput1() {
options,
lines(
"const foo = {}", //
"foo.x &&= 'something';"),
lines("let a; (a = {}).a && (a.a = 'something')"));
"foo.x &&= 'something';",
"alert(foo.x);"),
lines(
"var a;", //
"a &&= 'something';",
"alert(a);"));
}

@Test
Expand Down Expand Up @@ -281,10 +285,12 @@ public void logicalAsssignmentsPropertyReferenceCastType_supportedOnlyWithoutTra
lines(
"const obj = {};", //
"obj.baa = true;",
"/** @type {?} */ (obj.baa) &&= 5"),
"/** @type {?} */ (obj.baa) &&= 5;",
"alert(obj.baa);"),
lines(
"const a = {a:!0}", //
"a.a && (a.a = 5)"));
"var a = !0;", //
"a = 5;",
"alert(a);"));
}

@Test
Expand Down Expand Up @@ -322,10 +328,12 @@ public void logicalAsssignmentsPropertyReferenceCastType_supportedWithTranspilat
lines(
"const obj = {};", //
"obj.baa = true;",
"/** @type {?} */ (obj.baa) &&= 5"),
"/** @type {?} */ (obj.baa) &&= 5",
"alert(obj.baa);"),
lines(
"const a = {a:!0}", //
"a.a && (a.a = 5)"));
"var a = !0;", //
"a = 5;",
"alert(a);"));
}

@Test
Expand Down

0 comments on commit d4da7b3

Please sign in to comment.