Skip to content

Commit 481ef6f

Browse files
author
tbreisacher
committed
Treat goog.forwardDeclare the same as goog.require for purposes of sorting, except that all forwardDeclares should come after all requires.
------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=158766918
1 parent 0180fe6 commit 481ef6f

File tree

4 files changed

+135
-27
lines changed

4 files changed

+135
-27
lines changed

‎src/com/google/javascript/jscomp/lint/CheckRequiresAndProvidesSorted.java

+40-10
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
*/
1616
package com.google.javascript.jscomp.lint;
1717

18+
import static com.google.common.base.Preconditions.checkNotNull;
19+
1820
import com.google.common.base.Function;
1921
import com.google.common.base.Preconditions;
2022
import com.google.common.collect.Ordering;
@@ -81,27 +83,52 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) {
8183
new Function<Node, String>() {
8284
@Override
8385
public String apply(Node n) {
86+
String key = null;
87+
boolean isForwardDeclare = false;
8488
if (NodeUtil.isNameDeclaration(n)) {
8589
if (n.getFirstChild().isName()) {
86-
// Case 1: var x = goog.require('w.x');
87-
return n.getFirstChild().getString();
90+
// Case 1:
91+
// var x = goog.require('w.x');
92+
// or
93+
// var x = goog.forwardDeclare('w.x');
94+
key = n.getFirstChild().getString();
95+
if (n.getFirstFirstChild()
96+
.getFirstChild()
97+
.matchesQualifiedName("goog.forwardDeclare")) {
98+
isForwardDeclare = true;
99+
}
88100
} else if (n.getFirstChild().isDestructuringLhs()) {
89101
// Case 2: var {y} = goog.require('w.x');
90-
// All case 2 nodes should come after all case 1 nodes.
102+
// All case 2 nodes should come after all case 1 nodes. ('{' sorts after a-z)
91103
Node pattern = n.getFirstFirstChild();
92104
Preconditions.checkState(pattern.isObjectPattern(), pattern);
105+
Node call = n.getFirstChild().getLastChild();
106+
Preconditions.checkState(call.isCall(), call);
107+
Preconditions.checkState(
108+
call.getFirstChild().matchesQualifiedName("goog.require"),
109+
call.getFirstChild());
93110
if (!pattern.hasChildren()) {
94-
return "{";
111+
key = "{";
112+
} else {
113+
key = "{" + pattern.getFirstChild().getString();
95114
}
96-
return "{" + pattern.getFirstChild().getString();
97115
}
98116
} else if (n.isExprResult()) {
99-
// Case 3: goog.require('a.b.c');
117+
// Case 3, one of:
118+
// goog.provide('a.b.c');
119+
// goog.require('a.b.c');
120+
// goog.forwardDeclare('a.b.c');
100121
// All case 3 nodes should come after case 1 and 2 nodes, so prepend
101122
// '|' which sorts after '{'
102-
return "|" + n.getFirstChild().getLastChild().getString();
123+
key = "|" + n.getFirstChild().getLastChild().getString();
124+
if (n.getFirstFirstChild().matchesQualifiedName("goog.forwardDeclare")) {
125+
isForwardDeclare = true;
126+
}
127+
} else {
128+
throw new IllegalArgumentException("Unexpected node " + n);
103129
}
104-
throw new IllegalArgumentException("Unexpected node " + n);
130+
// Make sure all forwardDeclares come after all requires.
131+
return (isForwardDeclare ? "z" : "a") + checkNotNull(key);
105132
}
106133
};
107134

@@ -136,6 +163,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
136163
case CALL:
137164
Node callee = n.getFirstChild();
138165
if (!callee.matchesQualifiedName("goog.require")
166+
&& !callee.matchesQualifiedName("goog.forwardDeclare")
139167
&& !callee.matchesQualifiedName("goog.provide")
140168
&& !callee.matchesQualifiedName("goog.module")) {
141169
return;
@@ -150,7 +178,8 @@ public void visit(NodeTraversal t, Node n, Node parent) {
150178
if (namespace == null) {
151179
return;
152180
}
153-
if (callee.matchesQualifiedName("goog.require")) {
181+
if (callee.matchesQualifiedName("goog.require")
182+
|| callee.matchesQualifiedName("goog.forwardDeclare")) {
154183
requires.add(parent);
155184
} else {
156185
if (!requires.isEmpty()) {
@@ -161,7 +190,8 @@ public void visit(NodeTraversal t, Node n, Node parent) {
161190
}
162191
}
163192
} else if (NodeUtil.isNameDeclaration(parent.getParent())
164-
&& callee.matchesQualifiedName("goog.require")) {
193+
&& (callee.matchesQualifiedName("goog.require")
194+
|| callee.matchesQualifiedName("goog.forwardDeclare"))) {
165195
requires.add(parent.getParent());
166196
}
167197
break;

‎src/com/google/javascript/refactoring/ErrorToFixMapper.java

+27-16
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@
1515
*/
1616
package com.google.javascript.refactoring;
1717

18+
import static com.google.common.base.Preconditions.checkState;
1819
import static com.google.javascript.refactoring.SuggestedFix.getShortNameForRequire;
1920

20-
import com.google.common.base.Preconditions;
2121
import com.google.common.collect.ImmutableList;
22+
import com.google.common.collect.ImmutableSet;
2223
import com.google.common.collect.Iterables;
2324
import com.google.javascript.jscomp.AbstractCompiler;
2425
import com.google.javascript.jscomp.JSError;
@@ -82,9 +83,10 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp
8283
case "JSC_MISSING_SEMICOLON":
8384
return getFixForMissingSemicolon(error, compiler);
8485
case "JSC_REQUIRES_NOT_SORTED":
85-
return getFixForUnsortedRequiresOrProvides("goog.require", error, compiler);
86+
return getFixForUnsortedRequiresOrProvides(
87+
error, compiler, "goog.require", "goog.forwardDeclare");
8688
case "JSC_PROVIDES_NOT_SORTED":
87-
return getFixForUnsortedRequiresOrProvides("goog.provide", error, compiler);
89+
return getFixForUnsortedRequiresOrProvides(error, compiler, "goog.provide");
8890
case "JSC_DEBUGGER_STATEMENT_PRESENT":
8991
return removeNode(error, compiler);
9092
case "JSC_USELESS_EMPTY_STATEMENT":
@@ -114,7 +116,7 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp
114116

115117
private static SuggestedFix getFixForRedeclaration(JSError error, AbstractCompiler compiler) {
116118
Node name = error.node;
117-
Preconditions.checkState(name.isName(), name);
119+
checkState(name.isName(), name);
118120
Node parent = name.getParent();
119121
if (!NodeUtil.isNameDeclaration(parent)) {
120122
return null;
@@ -202,7 +204,7 @@ private static SuggestedFix getFixForReferenceToShortImportByLongName(
202204
Match match = new Match(error.node, metadata);
203205

204206
Matcher fullNameMatcher = FULLY_QUALIFIED_NAME.matcher(error.description);
205-
Preconditions.checkState(fullNameMatcher.matches(), error.description);
207+
checkState(fullNameMatcher.matches(), error.description);
206208
String fullName = fullNameMatcher.group(1);
207209

208210
Matcher shortNameMatcher = USE_SHORT_NAME.matcher(error.description);
@@ -294,7 +296,7 @@ private static SuggestedFix getFixForInexistentProperty(
294296

295297
private static SuggestedFix getFixForMissingRequire(JSError error, AbstractCompiler compiler) {
296298
Matcher regexMatcher = MISSING_REQUIRE.matcher(error.description);
297-
Preconditions.checkState(regexMatcher.matches(),
299+
checkState(regexMatcher.matches(),
298300
"Unexpected error description: %s", error.description);
299301
String namespaceToRequire = regexMatcher.group(1);
300302
NodeMetadata metadata = new NodeMetadata(compiler);
@@ -325,7 +327,7 @@ private static SuggestedFix getFixForDuplicateRequire(JSError error, AbstractCom
325327
return null;
326328
}
327329
Matcher regexMatcher = DUPLICATE_REQUIRE.matcher(error.description);
328-
Preconditions.checkState(
330+
checkState(
329331
regexMatcher.matches(), "Unexpected error description: %s", error.description);
330332
String namespace = regexMatcher.group(1);
331333
NodeMetadata metadata = new NodeMetadata(compiler);
@@ -344,7 +346,7 @@ private static SuggestedFix getFixForExtraRequire(JSError error, AbstractCompile
344346
if (error.node.isStringKey()) {
345347
fix.delete(error.node);
346348
} else {
347-
Preconditions.checkState(error.node.getParent().isStringKey());
349+
checkState(error.node.getParent().isStringKey(), error.node.getParent());
348350
fix.delete(error.node.getParent());
349351
}
350352
} else {
@@ -354,11 +356,11 @@ private static SuggestedFix getFixForExtraRequire(JSError error, AbstractCompile
354356
}
355357

356358
private static SuggestedFix getFixForUnsortedRequiresOrProvides(
357-
String closureFunction, JSError error, AbstractCompiler compiler) {
359+
JSError error, AbstractCompiler compiler, String... closureFunctions) {
358360
SuggestedFix.Builder fix = new SuggestedFix.Builder();
359361
fix.attachMatchedNodeInfo(error.node, compiler);
360362
Node script = NodeUtil.getEnclosingScript(error.node);
361-
RequireProvideSorter cb = new RequireProvideSorter(closureFunction);
363+
RequireProvideSorter cb = new RequireProvideSorter(closureFunctions);
362364
NodeTraversal.traverseEs6(compiler, script, cb);
363365
Node first = cb.calls.get(0);
364366
Node last = Iterables.getLast(cb.calls);
@@ -380,28 +382,37 @@ private static SuggestedFix getFixForUnsortedRequiresOrProvides(
380382

381383
private static class RequireProvideSorter extends NodeTraversal.AbstractShallowCallback
382384
implements Comparator<Node> {
383-
private final String closureFunction;
385+
private final ImmutableSet<String> closureFunctions;
384386
private final List<Node> calls = new ArrayList<>();
385387

386-
RequireProvideSorter(String closureFunction) {
387-
this.closureFunction = closureFunction;
388+
RequireProvideSorter(String... closureFunctions) {
389+
this.closureFunctions = ImmutableSet.copyOf(closureFunctions);
388390
}
389391

390392
@Override
391393
public final void visit(NodeTraversal nodeTraversal, Node n, Node parent) {
392394
if (n.isCall()
393395
&& parent.isExprResult()
394-
&& n.getFirstChild().matchesQualifiedName(closureFunction)) {
396+
&& matchName(n.getFirstChild())) {
395397
calls.add(parent);
396398
} else if (NodeUtil.isNameDeclaration(parent)
397399
&& n.hasChildren()
398400
&& n.getLastChild().isCall()
399-
&& n.getLastChild().getFirstChild().matchesQualifiedName(closureFunction)) {
400-
Preconditions.checkState(n.isName() || n.isDestructuringLhs());
401+
&& matchName(n.getLastChild().getFirstChild())) {
402+
checkState(n.isName() || n.isDestructuringLhs(), n);
401403
calls.add(parent);
402404
}
403405
}
404406

407+
private boolean matchName(Node n) {
408+
for (String closureFn : closureFunctions) {
409+
if (n.matchesQualifiedName(closureFn)) {
410+
return true;
411+
}
412+
}
413+
return false;
414+
}
415+
405416
public void sortCallsAlphabetically() {
406417
Collections.sort(calls, this);
407418
}

‎test/com/google/javascript/jscomp/lint/CheckRequiresAndProvidesSortedTest.java

+22
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,28 @@ public void testGoogModule_standalone() {
202202
REQUIRES_NOT_SORTED);
203203
}
204204

205+
public void testGoogModule_forwardDeclares() {
206+
testWarning(
207+
LINE_JOINER.join(
208+
"goog.module('x');",
209+
"",
210+
"const s = goog.require('s');",
211+
"const f = goog.forwardDeclare('f');",
212+
"const r = goog.require('r');"),
213+
REQUIRES_NOT_SORTED);
214+
}
215+
216+
public void testForwardDeclares() {
217+
testWarning(
218+
LINE_JOINER.join(
219+
"goog.provide('x');",
220+
"",
221+
"goog.require('s');",
222+
"goog.forwardDeclare('f');",
223+
"goog.require('r');"),
224+
REQUIRES_NOT_SORTED);
225+
}
226+
205227
public void testDuplicate() {
206228
testWarning(
207229
LINE_JOINER.join(

‎test/com/google/javascript/refactoring/ErrorToFixMapperTest.java

+46-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import org.junit.runner.RunWith;
3636
import org.junit.runners.JUnit4;
3737

38-
3938
/**
4039
* Test case for {@link ErrorToFixMapper}.
4140
*/
@@ -563,6 +562,52 @@ public void testSortRequiresInGoogModule_allThreeStyles() {
563562
"goog.require('standalone.two');"));
564563
}
565564

565+
@Test
566+
public void testSortRequiresInGoogModule_withFwdDeclare() {
567+
assertChanges(
568+
LINE_JOINER.join(
569+
"goog.module('x');",
570+
"",
571+
"const s = goog.require('s');",
572+
"const g = goog.forwardDeclare('g');",
573+
"const f = goog.forwardDeclare('f');",
574+
"const r = goog.require('r');",
575+
"",
576+
"alert(r, s);"),
577+
LINE_JOINER.join(
578+
"goog.module('x');",
579+
"",
580+
"const r = goog.require('r');",
581+
"const s = goog.require('s');",
582+
"const f = goog.forwardDeclare('f');",
583+
"const g = goog.forwardDeclare('g');",
584+
"",
585+
"alert(r, s);"));
586+
}
587+
588+
@Test
589+
public void testSortRequiresAndForwardDeclares() {
590+
assertChanges(
591+
LINE_JOINER.join(
592+
"goog.provide('x');",
593+
"",
594+
"goog.require('s');",
595+
"goog.forwardDeclare('g');",
596+
"goog.forwardDeclare('f');",
597+
"goog.require('r');",
598+
"",
599+
"alert(r, s);"),
600+
LINE_JOINER.join(
601+
"goog.provide('x');",
602+
"",
603+
"goog.require('r');",
604+
"goog.require('s');",
605+
"goog.forwardDeclare('f');",
606+
"goog.forwardDeclare('g');",
607+
"",
608+
"alert(r, s);"));
609+
}
610+
566611
@Test
567612
public void testMissingRequireInGoogProvideFile() {
568613
assertChanges(

0 commit comments

Comments
 (0)