Skip to content

Commit 92557f3

Browse files
shickscopybara-github
shicks
authored and
copybara-github
committed
Only record inline toggles that are actually used.
PiperOrigin-RevId: 561118170
1 parent 3b72456 commit 92557f3

File tree

3 files changed

+149
-21
lines changed

3 files changed

+149
-21
lines changed

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

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@
3232
import com.google.javascript.rhino.Node;
3333
import com.google.javascript.rhino.QualifiedName;
3434
import java.util.HashMap;
35+
import java.util.HashSet;
3536
import java.util.Map;
37+
import java.util.Set;
3638
import java.util.regex.Pattern;
3739
import org.jspecify.nullness.Nullable;
3840

@@ -81,10 +83,8 @@ public final class GatherModuleMetadata implements CompilerPass {
8183
static final DiagnosticType INVALID_NESTED_LOAD_MODULE =
8284
DiagnosticType.error("JSC_INVALID_NESTED_LOAD_MODULE", "goog.loadModule cannot be nested.");
8385

84-
static final DiagnosticType INVALID_READ_TOGGLE =
85-
DiagnosticType.error(
86-
"JSC_INVALID_READ_TOGGLE",
87-
"Argument to goog.readToggleInternalDoNotCallDirectly must be a string.");
86+
static final DiagnosticType INVALID_TOGGLE_USAGE =
87+
DiagnosticType.error("JSC_INVALID_TOGGLE_USAGE", "Invalid toggle usage: {0}");
8888

8989
private static final Node GOOG_PROVIDE = IR.getprop(IR.name("goog"), "provide");
9090
private static final Node GOOG_MODULE = IR.getprop(IR.name("goog"), "module");
@@ -95,13 +95,13 @@ public final class GatherModuleMetadata implements CompilerPass {
9595
private static final Node GOOG_MODULE_DECLARELEGACYNAMESPACE =
9696
IR.getprop(GOOG_MODULE.cloneTree(), "declareLegacyNamespace");
9797
private static final Node GOOG_DECLARE_MODULE_ID = IR.getprop(IR.name("goog"), "declareModuleId");
98-
private static final Node GOOG_READ_TOGGLE =
99-
IR.getprop(IR.name("goog"), "readToggleInternalDoNotCallDirectly");
10098

10199
// TODO(johnplaisted): Remove once clients have migrated to declareModuleId
102100
private static final Node GOOG_MODULE_DECLARNAMESPACE =
103101
IR.getprop(GOOG_MODULE.cloneTree(), "declareNamespace");
104102

103+
private static final String TOGGLE_NAME_PREFIX = "TOGGLE_";
104+
105105
/**
106106
* Map from module path to module. These modules represent files and thus will contain all goog
107107
* namespaces that are in the file. These are not the same modules in modulesByGoogNamespace.
@@ -216,6 +216,14 @@ ModuleMetadata build() {
216216

217217
/** Traverses the AST and build a sets of {@link ModuleMetadata}s. */
218218
private final class Finder implements NodeTraversal.Callback {
219+
220+
// Store both names and vars. Strings alone is insufficient to determine whether a name is
221+
// actually a toggle module (since it could have been shadowed, or may have been defined in a
222+
// different file), but looking up by only vars is much slower. This way we can do a fast name
223+
// lookup, followed by a slower var lookup only if the name is known to be a toggle module name.
224+
final Set<String> toggleModuleNames = new HashSet<>();
225+
final Set<Var> toggleModules = new HashSet<>();
226+
219227
@Override
220228
public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
221229
switch (n.getToken()) {
@@ -337,7 +345,22 @@ private boolean isFromGoogImport(Var goog) {
337345
}
338346

339347
private void visitName(NodeTraversal t, Node n) {
340-
if (!"goog".equals(n.getString())) {
348+
String name = n.getString();
349+
if (toggleModuleNames.contains(name)) {
350+
Var nameVar = t.getScope().getVar(name);
351+
if (toggleModules.contains(nameVar)) {
352+
Node parent = n.getParent();
353+
if (parent.isGetProp()) {
354+
addToggle(t, n, parent.getString());
355+
} else if (!NodeUtil.isNameDeclaration(parent)) {
356+
t.report(
357+
n,
358+
INVALID_TOGGLE_USAGE,
359+
"toggle modules may not be used other than looking up properties");
360+
}
361+
}
362+
}
363+
if (!"goog".equals(name)) {
341364
return;
342365
}
343366

@@ -421,10 +444,31 @@ private void visitGoogCall(NodeTraversal t, Node n) {
421444
}
422445
} else if (getprop.matchesQualifiedName(GOOG_REQUIRE)) {
423446
if (n.hasTwoChildren() && n.getLastChild().isStringLit()) {
424-
currentModule
425-
.metadataBuilder
426-
.stronglyRequiredGoogNamespacesBuilder()
427-
.add(n.getLastChild().getString());
447+
String namespace = n.getLastChild().getString();
448+
currentModule.metadataBuilder.stronglyRequiredGoogNamespacesBuilder().add(namespace);
449+
if (namespace.endsWith("$2etoggles")) {
450+
// Track imports of *.toggles.ts, which are rewritten to $2etoggles.
451+
Node callParent = n.getParent();
452+
Node lhs = callParent.getFirstChild();
453+
if (callParent.isDestructuringLhs()) {
454+
// const {TOGGLE_foo} = goog.require('foo$2etoggles');
455+
for (Node key : lhs.children()) {
456+
if (key.isStringKey()) {
457+
addToggle(t, n, key.getString());
458+
} else {
459+
t.report(n, INVALID_TOGGLE_USAGE, "must be destructured with string keys");
460+
}
461+
}
462+
} else if (callParent.isName()) {
463+
// const fooToggles = goog.require('foo$2etoggles');
464+
String name = callParent.getString();
465+
Var nameVar = t.getScope().getVar(name);
466+
toggleModules.add(nameVar);
467+
toggleModuleNames.add(name);
468+
} else {
469+
t.report(n, INVALID_TOGGLE_USAGE, "import must be assigned");
470+
}
471+
}
428472
} else {
429473
t.report(n, INVALID_REQUIRE_NAMESPACE);
430474
}
@@ -452,12 +496,16 @@ private void visitGoogCall(NodeTraversal t, Node n) {
452496
} else {
453497
t.report(n, INVALID_REQUIRE_DYNAMIC);
454498
}
455-
} else if (getprop.matchesQualifiedName(GOOG_READ_TOGGLE)) {
456-
if (n.hasTwoChildren() && n.getLastChild().isStringLit()) {
457-
currentModule.metadataBuilder.readTogglesBuilder().add(n.getLastChild().getString());
458-
} else {
459-
t.report(n, INVALID_READ_TOGGLE);
460-
}
499+
}
500+
}
501+
502+
/** Record a toggle usage (either a destructured import or a property lookup on a module). */
503+
private void addToggle(NodeTraversal t, Node n, String name) {
504+
if (name.startsWith(TOGGLE_NAME_PREFIX)) {
505+
String toggleName = name.substring(TOGGLE_NAME_PREFIX.length());
506+
currentModule.metadataBuilder.readTogglesBuilder().add(toggleName);
507+
} else {
508+
t.report(n, INVALID_TOGGLE_USAGE, "all toggle names must start with `TOGGLE_`");
461509
}
462510
}
463511

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

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -737,11 +737,91 @@ public void testDynamicImport() {
737737

738738
@Test
739739
public void testReadToggle() {
740+
test(
741+
srcs(
742+
SourceFile.fromCode(
743+
"foo$2etoggles.js",
744+
lines(
745+
"goog.module('foo$2etoggles');",
746+
"exports.TOGGLE_foo = goog.readToggleInternalDoNotCallDirectly('foo');",
747+
"exports.TOGGLE_bar = goog.readToggleInternalDoNotCallDirectly('bar');",
748+
"exports.TOGGLE_b_a_z = goog.readToggleInternalDoNotCallDirectly('b_a_z');"))));
749+
ModuleMetadata m = metadataMap().getModulesByPath().get("foo$2etoggles.js");
750+
assertThat(m.readToggles()).isEmpty();
751+
}
752+
753+
@Test
754+
public void testToggleModuleImportDestructured() {
755+
testSame("const {TOGGLE_foo, TOGGLE_b_a_z} = goog.require('foo$2etoggles');");
756+
ModuleMetadata m = metadataMap().getModulesByPath().get("testcode");
757+
assertThat(m.readToggles()).containsExactly("foo", "b_a_z");
758+
}
759+
760+
@Test
761+
public void testToggleModuleImportDestructuredAliased() {
762+
testSame("const {TOGGLE_bar: baz} = goog.require('foo$2etoggles');");
763+
ModuleMetadata m = metadataMap().getModulesByPath().get("testcode");
764+
assertThat(m.readToggles()).containsExactly("bar");
765+
}
766+
767+
@Test
768+
public void testToggleModuleImportAsModule() {
769+
testSame(
770+
lines(
771+
"const toggles = goog.require('foo$2etoggles');",
772+
"function foo() {",
773+
" console.log(toggles.TOGGLE_foo);",
774+
" console.log(toggles.TOGGLE_b_a_z);",
775+
"}"));
776+
ModuleMetadata m = metadataMap().getModulesByPath().get("testcode");
777+
assertThat(m.readToggles()).containsExactly("foo", "b_a_z");
778+
}
779+
780+
@Test
781+
public void testToggleModuleImportAsSideEffect() {
782+
test(
783+
srcs("goog.require('foo$2etoggles');"), //
784+
error(GatherModuleMetadata.INVALID_TOGGLE_USAGE));
785+
}
786+
787+
@Test
788+
public void testToggleModuleImportDestructuredWithInvalidName() {
789+
test(
790+
srcs("const {foo} = goog.require('foo$2etoggles');"),
791+
error(GatherModuleMetadata.INVALID_TOGGLE_USAGE));
792+
}
793+
794+
@Test
795+
public void testToggleModuleImportAsModuleWithInvalidPropertyName() {
796+
test(
797+
srcs(
798+
lines(
799+
"const foo = goog.require('foo$2etoggles');", //
800+
"console.log(foo.bar);")),
801+
error(GatherModuleMetadata.INVALID_TOGGLE_USAGE));
802+
}
803+
804+
@Test
805+
public void testToggleModuleInvalidModuleUsage() {
806+
test(
807+
srcs(
808+
lines(
809+
"const foo = goog.require('foo$2etoggles');", //
810+
"console.log(foo);")),
811+
error(GatherModuleMetadata.INVALID_TOGGLE_USAGE));
812+
}
813+
814+
@Test
815+
public void testToggleModuleIgnoreShadowedUsage() {
740816
testSame(
741817
lines(
742-
"goog.readToggleInternalDoNotCallDirectly('foo_bar');",
743-
"goog.readToggleInternalDoNotCallDirectly('baz');"));
818+
"const toggles = goog.require('foo$2etoggles');",
819+
"function foo() {",
820+
" const toggles = {};",
821+
" console.log(toggles.foo)",
822+
" console.log(toggles.TOGGLE_bar)",
823+
"}"));
744824
ModuleMetadata m = metadataMap().getModulesByPath().get("testcode");
745-
assertThat(m.readToggles()).containsExactly("foo_bar", "baz");
825+
assertThat(m.readToggles()).isEmpty();
746826
}
747827
}

‎test/com/google/javascript/jscomp/deps/JsFileFullParserTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ public void testSoyDelcall_legacy() {
219219

220220
@Test
221221
public void testReadToggle() {
222-
FileInfo info = parse("goog.readToggleInternalDoNotCallDirectly('foo_bar');");
222+
FileInfo info = parse("const {TOGGLE_foo_bar} = goog.require('foo$2etoggles');");
223223
assertThat(info.readToggles).containsExactly("foo_bar");
224224
}
225225

0 commit comments

Comments
 (0)