Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 249c0d3

Browse files
lauraharkercopybara-github
authored andcommitted
Improve readability of validity check errors
PiperOrigin-RevId: 823553783
1 parent 802f7ae commit 249c0d3

File tree

3 files changed

+189
-30
lines changed

3 files changed

+189
-30
lines changed

‎src/com/google/javascript/jscomp/ChangeVerifier.java‎

Lines changed: 82 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,19 @@
1515
*/
1616
package com.google.javascript.jscomp;
1717

18+
import static com.google.common.base.Preconditions.checkNotNull;
1819
import static com.google.common.base.Preconditions.checkState;
1920

2021
import com.google.common.collect.BiMap;
2122
import com.google.common.collect.HashBiMap;
2223
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2324
import com.google.javascript.jscomp.NodeUtil.Visitor;
2425
import com.google.javascript.rhino.Node;
26+
import java.util.ArrayList;
27+
import java.util.Collections;
2528
import java.util.LinkedHashSet;
2629
import java.util.Set;
30+
import java.util.function.Supplier;
2731

2832
/**
2933
* A Class to assist in AST change tracking verification. To validate a "snapshot" is taken
@@ -180,15 +184,30 @@ private void verifyNodeChange(final String passNameMsg, Node n, Node snapshot) {
180184
if (n.isRoot()) {
181185
return;
182186
}
187+
EqualsResult result = getInequivalenceReasonExcludingFunctions(n, snapshot);
183188
if (n.getChangeTime() > snapshot.getChangeTime()) {
184-
if (equalsExcludingFunctions(n, snapshot)) {
189+
// If the current node is marked as changed (changeTime > snapshot.getChangeTime)
190+
// but is actually equal to the snapshot, that's an error.
191+
if (result.equals()) {
185192
throw new IllegalStateException(
186193
passNameMsg + "unchanged scope marked as changed: " + getNameForNode(n));
187194
}
188195
} else {
189-
if (!equalsExcludingFunctions(n, snapshot)) {
196+
// If the current node is NOT marked as changed (changeTime <= snapshot.getChangeTime)
197+
// but is actually different from the snapshot, that's an error.
198+
if (!result.equals()) {
190199
throw new IllegalStateException(
191-
passNameMsg + "changed scope not marked as changed: " + getNameForNode(n));
200+
String.format(
201+
"""
202+
"%schanged scope not marked as changed: %s.
203+
%s
204+
Ancestor nodes:
205+
%s
206+
""",
207+
passNameMsg,
208+
getNameForNode(n),
209+
result.errorMessage.get(),
210+
path(n, result.errorNode())));
192211
}
193212
}
194213
}
@@ -210,30 +229,65 @@ String getNameForNode(Node n) {
210229
}
211230
}
212231

232+
/** Returns the path from the ancestor to the child node. */
233+
private String path(Node ancestor, Node child) {
234+
ArrayList<String> childToAncestor = new ArrayList<>();
235+
for (Node current = child; current != ancestor; current = current.getParent()) {
236+
childToAncestor.add(current.toString());
237+
}
238+
childToAncestor.add(ancestor.toString());
239+
Collections.reverse(childToAncestor);
240+
StringBuilder result = new StringBuilder();
241+
for (int i = 0; i < childToAncestor.size(); i++) {
242+
result.repeat(' ', i * 2); // indent
243+
result.append(childToAncestor.get(i));
244+
result.append("\n");
245+
}
246+
return result.toString();
247+
}
248+
249+
private record EqualsResult(boolean equals, Node errorNode, Supplier<String> errorMessage) {
250+
static EqualsResult equal() {
251+
return new EqualsResult(true, null, () -> null);
252+
}
253+
254+
static EqualsResult notEqual(Node errorNode, String error, Object after, Object before) {
255+
return new EqualsResult(
256+
false,
257+
errorNode,
258+
() ->
259+
String.format(
260+
"""
261+
%s
262+
Before: %s
263+
After: %s
264+
""",
265+
error, before, after));
266+
}
267+
}
268+
213269
/**
214-
* @return Whether the two node are equivalent while ignoring differences any descendant functions
215-
* differences.
270+
* Checks whether the two given nodes are equivalent, while ignoring differences in descendant
271+
* functions.
216272
*/
217-
private static boolean equalsExcludingFunctions(Node thisNode, Node thatNode) {
218-
if (thisNode == null || thatNode == null) {
219-
return thisNode == null && thatNode == null;
273+
private static EqualsResult getInequivalenceReasonExcludingFunctions(
274+
Node thisNode, Node thatNode) {
275+
checkNotNull(thisNode);
276+
checkNotNull(thatNode);
277+
if (thisNode.getChildCount() != thatNode.getChildCount()) {
278+
return EqualsResult.notEqual(
279+
thisNode,
280+
"differing child count",
281+
thisNode + ": " + thisNode.getChildCount(),
282+
thatNode + ": " + thatNode.getChildCount());
220283
}
221284
if (!thisNode.isEquivalentWithSideEffectsToShallow(thatNode)) {
222-
return false;
223-
}
224-
if (thisNode.getChildCount() != thatNode.getChildCount()) {
225-
return false;
285+
return EqualsResult.notEqual(thisNode, "shallow inequivalence", thisNode, thatNode);
226286
}
227-
228287
if (thisNode.isFunction() && thatNode.isFunction()) {
229288
if (NodeUtil.isFunctionDeclaration(thisNode) != NodeUtil.isFunctionDeclaration(thatNode)) {
230-
return false;
231-
}
232-
}
233-
234-
if (thisNode.hasParent() && thisNode.getParent().isParamList()) {
235-
if (thisNode.isUnusedParameter() != thatNode.isUnusedParameter()) {
236-
return false;
289+
return EqualsResult.notEqual(
290+
thisNode, "mismatched isFunctionDeclaration", thisNode, thatNode);
237291
}
238292
}
239293

@@ -244,25 +298,27 @@ private static boolean equalsExcludingFunctions(Node thisNode, Node thatNode) {
244298
// Don't compare function expression name, parameters or bodies.
245299
// But do check that that the node is there.
246300
if (thatChild.getToken() != thisChild.getToken()) {
247-
return false;
301+
return EqualsResult.notEqual(thisNode, "different tokens", thisChild, thatChild);
248302
}
249303
// Only compare function names for function declarations (not function expressions)
250304
// as they change the outer scope definition.
251305
if (thisChild.isFunction() && NodeUtil.isFunctionDeclaration(thisChild)) {
252306
String thisName = thisChild.getFirstChild().getString();
253307
String thatName = thatChild.getFirstChild().getString();
254308
if (!thisName.equals(thatName)) {
255-
return false;
309+
return EqualsResult.notEqual(thisNode, "function name changed", thisName, thatName);
256310
}
257311
}
258-
} else if (!equalsExcludingFunctions(thisChild, thatChild)) {
259-
return false;
312+
} else {
313+
EqualsResult result = getInequivalenceReasonExcludingFunctions(thisChild, thatChild);
314+
if (!result.equals()) {
315+
return result;
316+
}
260317
}
261318
thisChild = thisChild.getNext();
262319
thatChild = thatChild.getNext();
263320
}
264321

265-
return true;
322+
return EqualsResult.equal();
266323
}
267324
}
268-

‎src/com/google/javascript/jscomp/ValidityCheck.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import static com.google.common.base.Preconditions.checkState;
1919

2020
import com.google.common.annotations.VisibleForTesting;
21-
import com.google.common.collect.ImmutableMap;
2221
import com.google.common.collect.ImmutableSet;
2322
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
2423
import com.google.javascript.rhino.JSDocInfo;
@@ -193,8 +192,12 @@ public void visit(NodeTraversal t, Node n, Node parent) {
193192
throw new IllegalStateException(
194193
"The name "
195194
+ name
196-
+ " is not consistently annotated as constant. Expected "
197-
+ ImmutableMap.copyOf(constantMap));
195+
+ " is not consistently annotated as constant. Current constness: "
196+
+ isConst
197+
+ ", previous constness: "
198+
+ value.booleanValue()
199+
+ "\n\nAt node: "
200+
+ n);
198201
}
199202
}
200203
}

‎test/com/google/javascript/jscomp/ChangeVerifierTest.java‎

Lines changed: 101 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public void testCorrectValidationOfScriptWithChangeAfterFunction() {
6969
}
7070

7171
@Test
72-
public void testChangeToScriptNotReported() {
72+
public void testChangeToScriptNotReported_newChild() {
7373
Node script = parse("function A() {} if (0) { A(); }");
7474
checkState(script.isScript());
7575

@@ -85,6 +85,70 @@ public void testChangeToScriptNotReported() {
8585
assertThrows(
8686
IllegalStateException.class, () -> verifier.checkRecordedChanges("test2", script));
8787
assertThat(e).hasMessageThat().contains("changed scope not marked as changed");
88+
assertThat(e).hasMessageThat().contains("differing child count");
89+
}
90+
91+
@Test
92+
public void testChangeToScriptNotReported_changeToChild() {
93+
Node script = parse("function A() {} if (0) { A(); }");
94+
checkState(script.isScript());
95+
96+
ChangeVerifier verifier = new ChangeVerifier(compiler).snapshot(script);
97+
98+
// no change, no problem
99+
verifier.checkRecordedChanges("test1", script);
100+
101+
// modify the if condition, but don't report the change.
102+
Node ifStatement = script.getLastChild();
103+
Node condition = NodeUtil.getConditionExpression(ifStatement);
104+
condition.replaceWith(IR.number(1));
105+
106+
IllegalStateException e =
107+
assertThrows(
108+
IllegalStateException.class, () -> verifier.checkRecordedChanges("test2", script));
109+
assertThat(e)
110+
.hasMessageThat()
111+
.contains(
112+
"""
113+
test2: changed scope not marked as changed: SCRIPT: testcode.
114+
shallow inequivalence
115+
Before: NUMBER 0.0 1:20 [length: 1] [source_file: testcode]
116+
After: NUMBER 1.0 1:20 [length: 1] [source_file: testcode]
117+
118+
Ancestor nodes:
119+
SCRIPT 1:0 [length: 31] [source_file: testcode] [input_id: InputId: testcode] [feature_set: []]
120+
IF 1:16 [length: 15] [source_file: testcode]
121+
NUMBER 1.0 1:20 [length: 1] [source_file: testcode]
122+
""");
123+
}
124+
125+
@Test
126+
public void testChangeToScriptNotReported_changeToFunctionName() {
127+
Node script = parse("function A() {} if (0) { A(); }");
128+
checkState(script.isScript());
129+
130+
ChangeVerifier verifier = new ChangeVerifier(compiler).snapshot(script);
131+
132+
// no change, no problem
133+
verifier.checkRecordedChanges("test1", script);
134+
135+
// modify the if condition, but don't report the change.
136+
Node functionNode = script.getFirstChild();
137+
Node functionNameNode = NodeUtil.getNameNode(functionNode);
138+
functionNameNode.replaceWith(IR.name("B"));
139+
140+
IllegalStateException e =
141+
assertThrows(
142+
IllegalStateException.class, () -> verifier.checkRecordedChanges("test2", script));
143+
assertThat(e).hasMessageThat().contains("changed scope not marked as changed");
144+
assertThat(e)
145+
.hasMessageThat()
146+
.contains(
147+
"""
148+
function name changed
149+
Before: A
150+
After: B
151+
""");
88152
}
89153

90154
@Test
@@ -108,6 +172,42 @@ public void testChangeToFunction_notReported() {
108172
assertThat(e).hasMessageThat().contains("changed scope not marked as changed");
109173
}
110174

175+
@Test
176+
public void testChangeToFunction_newlyUnusedParameter() {
177+
Node script = parse("function f(x) {}");
178+
checkState(script.isScript(), script);
179+
Node function = script.getFirstChild();
180+
checkState(function.isFunction(), function);
181+
Node xParam = NodeUtil.getFunctionParameters(function).getOnlyChild();
182+
checkState(xParam.matchesName("x"), xParam);
183+
184+
ChangeVerifier verifier = new ChangeVerifier(compiler).snapshot(script);
185+
186+
// no change, no problem.
187+
verifier.checkRecordedChanges("test1", script);
188+
189+
// mark parameter x as unused
190+
xParam.setUnusedParameter(true);
191+
192+
IllegalStateException e =
193+
assertThrows(
194+
IllegalStateException.class, () -> verifier.checkRecordedChanges("test2", script));
195+
assertThat(e).hasMessageThat().contains("changed scope not marked as changed");
196+
assertThat(e)
197+
.hasMessageThat()
198+
.contains(
199+
"""
200+
shallow inequivalence
201+
Before: NAME x 1:11 [length: 1] [source_file: testcode]
202+
After: NAME x 1:11 [length: 1] [source_file: testcode] [is_unused_parameter: 1]
203+
204+
Ancestor nodes:
205+
FUNCTION f 1:0 [length: 16] [source_file: testcode]
206+
PARAM_LIST 1:10 [length: 3] [source_file: testcode]
207+
NAME x 1:11 [length: 1] [source_file: testcode] [is_unused_parameter: 1]
208+
""");
209+
}
210+
111211
@Test
112212
public void testChangeToArrowFunction_notReported() {
113213
Node script = parse("() => {}");

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /