Skip to content

Commit

Permalink
Change PeepholeRemoveDeadCode to stop asserting that the AST is norma…
Browse files Browse the repository at this point in the history
…lized.

The pass runs both before and after denormalize. So it’s a bug that the pass unconditionally asserts that the AST is normalized.

This CL deletes the check. Alternatively, even just guarding the check with `isASTNormalized` would works - http://sponge2/c65113df-4f96-4148-9400-6f551562349c

```
        if (!isASTNormalized()) {
          // the pass is running after denormalize
          return subtree;
        }
        throw checkNormalization(false, "WHILE");
```

PiperOrigin-RevId: 561471939
  • Loading branch information
rishipal authored and copybara-github committed Aug 30, 2023
1 parent 334e0b6 commit 4fbcc71
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 8 deletions.
12 changes: 4 additions & 8 deletions src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ Node optimizeSubtree(Node subtree) {
case IF:
return tryFoldIf(subtree);
case WHILE:
throw checkNormalization(false, "WHILE");
// This pass gets run both before and after denormalize. Hence, the AST could potentially
// contain WHILE (denormalized).
// TODO: Ideally, we should optimize this case instead of returning
return subtree;
case FOR:
{
Node condition = NodeUtil.getConditionExpression(subtree);
Expand Down Expand Up @@ -787,7 +790,6 @@ Node tryOptimizeBlock(Node n) {
for (Node c = n.getFirstChild(); c != null; ) {
Node next = c.getNext(); // save c.next, since 'c' may be removed
if (!isUnremovableNode(c) && !mayHaveSideEffects(c)) {
checkNormalization(!NodeUtil.isFunctionDeclaration(n), "function declaration");
// TODO(johnlenz): determine what this is actually removing. Candidates
// include: EMPTY nodes, control structures without children
// (removing infinite loops), empty try blocks. What else?
Expand Down Expand Up @@ -1377,10 +1379,4 @@ private Node tryRemoveOptionalCall(Node optionalCall) {
this.reportChangeToEnclosingScope(result);
return result;
}

private static @Nullable IllegalStateException checkNormalization(
boolean condition, String feature) {
checkState(condition, "Unexpected %s. AST should be normalized.", feature);
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ public void process(Node externs, Node root) {
@Before
public void setUp() throws Exception {
super.setUp();
// This pass doesn't need the AST to be normalized as it runs both before and after
// `denormalize`. Since it runs PureFunctionsIdentifier pass which expects the AST to be
// normalized, we're doing `enableNormalize` for these tests.
enableNormalize();
// TODO(bradfordcsmith): Stop normalizing the expected output or document why it is necessary.
enableNormalizeExpectedOutput();
Expand Down

0 comments on commit 4fbcc71

Please sign in to comment.