From 10ea2e277d871c3b260276b77596671d8ba963c8 Mon Sep 17 00:00:00 2001
From: rishipal <rishipal@google.com>
Date: Mon, 7 Aug 2023 15:39:26 -0700
Subject: [PATCH] Tighten the featureSet validation in the ASTValidator.java
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With the recent go/accurately-maintain-script-node-featureSet change, we can make the ASTValidator to confirm after each pass that:

1. Features encountered during AST traversal of a SCRIPT  <= compiler's allowable featureSet [Source] (becomes optional)
2. Features encountered during AST traversal of a SCRIPT <= that particular SCRIPT node’s FEATURE_SET (accurate, without overestimation during transpile)
3. every SCRIPT node’s FEATURE_SET <= compiler's allowable featureSet (possible to do)

#1 and #2 happened automatically as part of go/accurately-maintain-script-node-featureSet.

This CL adds #3 above.

PiperOrigin-RevId: 554614152
---
 .../javascript/jscomp/AstValidator.java       | 38 ++++++++++++++++++-
 .../javascript/jscomp/AstValidatorTest.java   | 10 +++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/com/google/javascript/jscomp/AstValidator.java b/src/com/google/javascript/jscomp/AstValidator.java
index 910ad9c22bf..442af2af3c1 100644
--- a/src/com/google/javascript/jscomp/AstValidator.java
+++ b/src/com/google/javascript/jscomp/AstValidator.java
@@ -139,6 +139,41 @@ public void validateScript(Node n) {
     } else {
       validateStatements(n.getFirstChild());
     }
+    if (isScriptFeatureValidationEnabled) {
+      validateScriptFeatureSet(n);
+    }
+  }
+
+  /**
+   * Confirm that every SCRIPT node’s FEATURE_SET <= compiler's allowable featureSet. This is
+   * possbile because with go/accurately-maintain-script-node-featureSet, each transpiler pass
+   * updates script features anytime it updates the compiler's allowable features.
+   */
+  private void validateScriptFeatureSet(Node scriptNode) {
+    if (!scriptNode.isScript()) {
+      violation("Not a script node", scriptNode);
+      // unit tests for this pass perform "Negaive Testing" (i.e pass non-script nodes to {@code
+      // validateScript}) and expect a violation {@code expectInvalid(n, Check.SCRIPT);}
+      // report violation and return here instead of crashing below in {@code
+      // NodeUtil.getFeatureSetofScript}
+      // for test to complete
+      return;
+    }
+    FeatureSet scriptFeatures = NodeUtil.getFeatureSetOfScript(currentScript);
+    FeatureSet allowableFeatures = compiler.getAllowableFeatures();
+    if (scriptFeatures == null || allowableFeatures == null) {
+      return;
+    }
+    if (!allowableFeatures.contains(scriptFeatures)) {
+      if (!scriptNode.isFromExterns()) {
+        // Skip this check for externs because we don't need to complete transpilation on externs,
+        // and currently only transpile externs so that we can typecheck ES6+ features in externs.
+        FeatureSet differentFeatures = scriptFeatures.without(allowableFeatures);
+        violation(
+            "SCRIPT node contains these unallowable features:" + differentFeatures.getFeatures(),
+            currentScript);
+      }
+    }
   }
 
   public void validateModuleContents(Node n) {
@@ -2208,7 +2243,8 @@ private void validateMaximumChildCount(Node n, int i) {
   }
 
   private void validateFeature(Feature feature, Node n) {
-    if (!n.isFromExterns() && !compiler.getAllowableFeatures().has(feature)) {
+    FeatureSet allowbleFeatures = compiler.getAllowableFeatures();
+    if (!n.isFromExterns() && !allowbleFeatures.has(feature)) {
       // Skip this check for externs because we don't need to complete transpilation on externs,
       // and currently only transpile externs so that we can typecheck ES6+ features in externs.
       violation("AST should not contain " + feature, n);
diff --git a/test/com/google/javascript/jscomp/AstValidatorTest.java b/test/com/google/javascript/jscomp/AstValidatorTest.java
index 3d0c72137e7..e27618c7a93 100644
--- a/test/com/google/javascript/jscomp/AstValidatorTest.java
+++ b/test/com/google/javascript/jscomp/AstValidatorTest.java
@@ -1526,6 +1526,16 @@ public void testValidFeatureInScript() {
 
     n.putProp(Node.FEATURE_SET, FeatureSet.BARE_MINIMUM.with(Feature.LET_DECLARATIONS));
     expectValid(n, Check.SCRIPT);
+
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT3); // resets compiler's allowable featureSet
+    expectInvalid(n, Check.SCRIPT);
+    // violation reported from {@code validateFeature} call of LET because
+    // `!allowbleFeatures.has(feature)`
+    assertThat(lastCheckViolationMessages).contains("AST should not contain let declaration");
+    // violation reported from {@code validateScript} call because script's `FEATURE_SET` is not
+    // a subset of compiler's allowable featureSet.
+    assertThat(lastCheckViolationMessages)
+        .contains("SCRIPT node contains these unallowable features:[let declaration]");
   }
 
   @Test