Fix deserialization to put closureUnaware features on the main AST

This makes it consistent with the parser/IRFactory & lets us fix a TODO & re-enable ast validation in TranspileAndOptimizeClosureUnawareTest.

In the long run, maybe we should instead track closureUnaware features separately. But at the moment, the added complexity doesn't seem worth it.

PiperOrigin-RevId: 890576326

Author: lauraharker

src/com/google/javascript/jscomp/serialization/ScriptNodeDeserializer.java

@@ -100,14 +100,11 @@ private ScriptNodeDeserializer owner() {
      * Visits a single AST node and returns the corresponding Node.
      *
      * @param astNode The AST node to visit.
-     * @param contextStack The context stack to use for the node. If feature collection should not
-     *     occur, this should be null.
+     * @param contextStack The context stack to use for the node.
      * @param sourceFileTemplate A template node to use for the node.
      */
     private Node visit(
-        AstNode astNode,
-        @Nullable List<FeatureContext> contextStack,
-        @Nullable Node sourceFileTemplate) {
+        AstNode astNode, List<FeatureContext> contextStack, @Nullable Node sourceFileTemplate) {
       if (sourceFileTemplate == null || astNode.getSourceFile() != 0) {
         // 0 == 'not set'
         sourceFileTemplate =
@@ -141,12 +138,13 @@ private Node visit(
         AstNode serializedShadowChild = astNode.getChild(0);
         Node closureUnawareSubtreeTemplateNode = sourceFileTemplate.cloneTree();
         closureUnawareSubtreeTemplateNode.setIsInClosureUnawareSubtree(true);
-        // Unlike normal deserialization, we want to avoid recording features for code within the
-        // shadow because we don't run transpilation passes over it and later stages of the compiler
-        // attempt to validate that transpilation has successfully run over the entire AST and no
-        // features remain that won't work in the given language output level.
+        // Note: we intentionally pass `contextStack` to visit() in order to capture any
+        // language features present in the shadow AST, like const syntax, into the main AST
+        // featureset. We are doing this to align ScriptNodeDeserializer with the existing behavior
+        // in IRFactory. (In the future we could consider instead giving each shadow AST script
+        // its own separate featureset, but that would be a larger change.)
         Node shadowedCode =
-            this.visit(serializedShadowChild, null, closureUnawareSubtreeTemplateNode);
+            this.visit(serializedShadowChild, contextStack, closureUnawareSubtreeTemplateNode);
         this.owner().setOriginalNameIfPresent(serializedShadowChild, shadowedCode);
         // The shadowed code is only the "source" parts of the shadow structure, and does not
         // include the synthetic code that is needed for the compiler to consider it a valid
@@ -169,9 +167,7 @@ private Node visit(
         this.owner().setOriginalNameIfPresent(child, deserializedChild);
       }
 
-      if (contextStack != null) {
-        contextStack.removeLast();
-      }
+      contextStack.removeLast();
 
       return n;
     }

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

@@ -54,8 +54,6 @@ public void setup() {
     this.outputCount = 0;
     this.extraOptions = options -> {};
     this.mode = NestedCompilerRunner.Mode.TRANSPILE_AND_OPTIMIZE;
-    // TODO: b/494231728 - re-enable this.
-    disableAstValidation();
   }
 
   @Override

test/com/google/javascript/jscomp/serialization/SerializeAndDeserializeAstTest.java

@@ -36,6 +36,7 @@
 import com.google.javascript.jscomp.colors.ColorRegistry;
 import com.google.javascript.jscomp.colors.StandardColors;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet;
+import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature;
 import com.google.javascript.jscomp.serialization.TypedAstDeserializer.DeserializedAst;
 import com.google.javascript.jscomp.testing.CodeSubTree;
 import com.google.javascript.jscomp.testing.TestExternsBuilder;
@@ -851,6 +852,41 @@ public void visit(NodeTraversal t, Node n, Node parent) {
     NodeTraversal.traverse(result.compiler, shadowedContent, cb);
   }
 
+  @Test
+  public void shadowAsts_featuresSetOnMainAstScript() throws IOException {
+    Result result =
+        runWithoutExpected(
+            externs(new TestExternsBuilder().addClosureExterns().build()),
+            srcs(
+                """
+                /** @closureUnaware @fileoverview */
+                goog.module('test');
+
+                /** @closureUnaware */
+                (function() {
+                  const x = 5;
+                })();
+                """));
+
+    Node shadowHost =
+        CodeSubTree.findFirstNode(result.sourceRoot, (n) -> n.getClosureUnawareShadow() != null);
+    Node shadowScript = shadowHost.getClosureUnawareShadow().getOnlyChild();
+    Node mainScript = result.sourceRoot.getOnlyChild();
+    checkState(mainScript.isScript(), mainScript);
+    checkState(shadowScript.isScript(), shadowScript);
+
+    // The main AST script contains the "const" feature; the shadow AST featureset is null.
+    // NOTE: this behavior was selected for consistency with the parser/IRFactory and to address
+    // AstValidator breakages. However, we could consider instead setting the featureset on
+    // the shadow script directly, and omitting CONST_DECLARATIONS in the main script featureset.
+    // At present, the main advantage of doing so seems to be a potential performance gain because
+    // transpilation passes on the main AST can skip any features only in the shadow AST, but we
+    // don't expect that performance gain to be significant.
+    assertThat(mainScript.getProp(Node.FEATURE_SET))
+        .isEqualTo(FeatureSet.BARE_MINIMUM.with(Feature.CONST_DECLARATIONS));
+    assertThat(shadowScript.getProp(Node.FEATURE_SET)).isNull();
+  }
+
   @Test
   public void includesRuntimeLibraryPaths() throws IOException {
     enableGatherExternProperties();