Skip to content

Commit

Permalink
Tighten the featureSet validation in the ASTValidator.java
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rishipal authored and copybara-github committed Aug 7, 2023
1 parent 2979771 commit 10ea2e2
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 1 deletion.
38 changes: 37 additions & 1 deletion src/com/google/javascript/jscomp/AstValidator.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 10 additions & 0 deletions test/com/google/javascript/jscomp/AstValidatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 10ea2e2

Please sign in to comment.