Skip to content

Commit 10ea2e2

Browse files
rishipalcopybara-github
authored andcommitted
Tighten the featureSet validation in the ASTValidator.java
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
1 parent 2979771 commit 10ea2e2

File tree

2 files changed

+47
-1
lines changed

2 files changed

+47
-1
lines changed

src/com/google/javascript/jscomp/AstValidator.java

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,41 @@ public void validateScript(Node n) {
139139
} else {
140140
validateStatements(n.getFirstChild());
141141
}
142+
if (isScriptFeatureValidationEnabled) {
143+
validateScriptFeatureSet(n);
144+
}
145+
}
146+
147+
/**
148+
* Confirm that every SCRIPT node’s FEATURE_SET <= compiler's allowable featureSet. This is
149+
* possbile because with go/accurately-maintain-script-node-featureSet, each transpiler pass
150+
* updates script features anytime it updates the compiler's allowable features.
151+
*/
152+
private void validateScriptFeatureSet(Node scriptNode) {
153+
if (!scriptNode.isScript()) {
154+
violation("Not a script node", scriptNode);
155+
// unit tests for this pass perform "Negaive Testing" (i.e pass non-script nodes to {@code
156+
// validateScript}) and expect a violation {@code expectInvalid(n, Check.SCRIPT);}
157+
// report violation and return here instead of crashing below in {@code
158+
// NodeUtil.getFeatureSetofScript}
159+
// for test to complete
160+
return;
161+
}
162+
FeatureSet scriptFeatures = NodeUtil.getFeatureSetOfScript(currentScript);
163+
FeatureSet allowableFeatures = compiler.getAllowableFeatures();
164+
if (scriptFeatures == null || allowableFeatures == null) {
165+
return;
166+
}
167+
if (!allowableFeatures.contains(scriptFeatures)) {
168+
if (!scriptNode.isFromExterns()) {
169+
// Skip this check for externs because we don't need to complete transpilation on externs,
170+
// and currently only transpile externs so that we can typecheck ES6+ features in externs.
171+
FeatureSet differentFeatures = scriptFeatures.without(allowableFeatures);
172+
violation(
173+
"SCRIPT node contains these unallowable features:" + differentFeatures.getFeatures(),
174+
currentScript);
175+
}
176+
}
142177
}
143178

144179
public void validateModuleContents(Node n) {
@@ -2208,7 +2243,8 @@ private void validateMaximumChildCount(Node n, int i) {
22082243
}
22092244

22102245
private void validateFeature(Feature feature, Node n) {
2211-
if (!n.isFromExterns() && !compiler.getAllowableFeatures().has(feature)) {
2246+
FeatureSet allowbleFeatures = compiler.getAllowableFeatures();
2247+
if (!n.isFromExterns() && !allowbleFeatures.has(feature)) {
22122248
// Skip this check for externs because we don't need to complete transpilation on externs,
22132249
// and currently only transpile externs so that we can typecheck ES6+ features in externs.
22142250
violation("AST should not contain " + feature, n);

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1526,6 +1526,16 @@ public void testValidFeatureInScript() {
15261526

15271527
n.putProp(Node.FEATURE_SET, FeatureSet.BARE_MINIMUM.with(Feature.LET_DECLARATIONS));
15281528
expectValid(n, Check.SCRIPT);
1529+
1530+
setAcceptedLanguage(LanguageMode.ECMASCRIPT3); // resets compiler's allowable featureSet
1531+
expectInvalid(n, Check.SCRIPT);
1532+
// violation reported from {@code validateFeature} call of LET because
1533+
// `!allowbleFeatures.has(feature)`
1534+
assertThat(lastCheckViolationMessages).contains("AST should not contain let declaration");
1535+
// violation reported from {@code validateScript} call because script's `FEATURE_SET` is not
1536+
// a subset of compiler's allowable featureSet.
1537+
assertThat(lastCheckViolationMessages)
1538+
.contains("SCRIPT node contains these unallowable features:[let declaration]");
15291539
}
15301540

15311541
@Test

0 commit comments

Comments
 (0)