Skip to content

Commit 84b8137

Browse files
lauraharkercopybara-github
lauraharker
authored and
copybara-github
committed
Ignore externs & colors from TypedAst shards containing no required inputs
Followup to cl/495077323, which attempted to do this but accidentally read all TypedAST shards anyway, as it was treating the presence of a "synthetic externs" file as meaning the shard was required. This is primarily for performance but also has some behavioral implications demonstrated in the TypedAstIntegrationTest changes. PiperOrigin-RevId: 515409100
1 parent 577f4cb commit 84b8137

File tree

3 files changed

+190
-12
lines changed

3 files changed

+190
-12
lines changed

‎src/com/google/javascript/jscomp/serialization/TypedAstDeserializer.java

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -241,11 +241,15 @@ private void deserializeTypedAst(
241241
ImmutableList<SourceFile> fileShard = toFileShard(typedAstProto);
242242

243243
if (this.mode.equals(Mode.FULL_AST)) {
244+
// Exclude any TypedAST shards passed to the compiler that don't contain any actual
245+
// sources we care about. For example: if we're passed "--js=a.js --js=b.js", and see a
246+
// TypedAST only containing ["c.js", "synthetic:externs"], it's a drain on performance to
247+
// include it.
244248
boolean containsRequiredInput = false;
245249
for (LazyAst lazyAst :
246250
Iterables.concat(typedAstProto.getExternAstList(), typedAstProto.getCodeAstList())) {
247251
SourceFile file = fileShard.get(lazyAst.getSourceFile() - 1);
248-
if (shouldIncludeFileInResult(file)) {
252+
if (requiredInputFiles.get().contains(file)) {
249253
containsRequiredInput = true;
250254
break;
251255
}
@@ -297,12 +301,6 @@ private ImmutableList<SourceFile> toFileShard(TypedAst typedAstProto) {
297301
return fileShardBuilder.build();
298302
}
299303

300-
private boolean shouldIncludeFileInResult(SourceFile file) {
301-
return identical(syntheticExterns, file) // Always preserve the synthetic externs
302-
|| !requiredInputFiles.isPresent()
303-
|| requiredInputFiles.get().contains(file);
304-
}
305-
306304
private void initLazyAstDeserializer(
307305
LazyAst lazyAst,
308306
ImmutableList<SourceFile> fileShard,
@@ -313,7 +311,11 @@ private void initLazyAstDeserializer(
313311
boolean parseInlineSourceMaps) {
314312
SourceFile file = fileShard.get(lazyAst.getSourceFile() - 1);
315313

316-
if (!shouldIncludeFileInResult(file)) {
314+
if (requiredInputFiles.isPresent()
315+
&& !requiredInputFiles.get().contains(file)
316+
// Synthetic externs bypass the normal dependency system and are always included in a
317+
// compilation.
318+
&& !identical(syntheticExterns, file)) {
317319
return;
318320
}
319321

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

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3252,11 +3252,16 @@ public void testTypedAstFilesystem_syntheticExternsFile_isCattedAcrossTypedAsts(
32523252
CompilerOptions compilerOptions = new CompilerOptions();
32533253
compiler.initOptions(compilerOptions);
32543254
SourceFile syntheticFile = compiler.SYNTHETIC_EXTERNS_FILE;
3255+
SourceFile fileOne = SourceFile.fromCode("one.js", "");
3256+
SourceFile fileTwo = SourceFile.fromCode("two.js", "");
32553257

32563258
TypedAst typedAst0 =
32573259
TypedAst.newBuilder()
32583260
.setStringPool(StringPool.empty().toProto())
3259-
.setSourceFilePool(SourceFilePool.newBuilder().addSourceFile(syntheticFile.getProto()))
3261+
.setSourceFilePool(
3262+
SourceFilePool.newBuilder()
3263+
.addSourceFile(syntheticFile.getProto())
3264+
.addSourceFile(fileOne.getProto()))
32603265
.addCodeAst(
32613266
LazyAst.newBuilder()
32623267
.setSourceFile(1)
@@ -3266,11 +3271,19 @@ public void testTypedAstFilesystem_syntheticExternsFile_isCattedAcrossTypedAsts(
32663271
.addChild(AstNode.newBuilder().setKind(NodeKind.CONST_DECLARATION))
32673272
.build()
32683273
.toByteString()))
3274+
.addCodeAst(
3275+
LazyAst.newBuilder()
3276+
.setSourceFile(2)
3277+
.setScript(
3278+
AstNode.newBuilder().setKind(NodeKind.SOURCE_FILE).build().toByteString()))
32693279
.build();
32703280
TypedAst typedAst1 =
32713281
TypedAst.newBuilder()
32723282
.setStringPool(StringPool.empty().toProto())
3273-
.setSourceFilePool(SourceFilePool.newBuilder().addSourceFile(syntheticFile.getProto()))
3283+
.setSourceFilePool(
3284+
SourceFilePool.newBuilder()
3285+
.addSourceFile(syntheticFile.getProto())
3286+
.addSourceFile(fileTwo.getProto()))
32743287
.addCodeAst(
32753288
LazyAst.newBuilder()
32763289
.setSourceFile(1)
@@ -3280,6 +3293,11 @@ public void testTypedAstFilesystem_syntheticExternsFile_isCattedAcrossTypedAsts(
32803293
.addChild(AstNode.newBuilder().setKind(NodeKind.VAR_DECLARATION))
32813294
.build()
32823295
.toByteString()))
3296+
.addCodeAst(
3297+
LazyAst.newBuilder()
3298+
.setSourceFile(2)
3299+
.setScript(
3300+
AstNode.newBuilder().setKind(NodeKind.SOURCE_FILE).build().toByteString()))
32833301
.build();
32843302
InputStream typedAstListStream =
32853303
new ByteArrayInputStream(
@@ -3291,7 +3309,10 @@ public void testTypedAstFilesystem_syntheticExternsFile_isCattedAcrossTypedAsts(
32913309

32923310
// When
32933311
compiler.initWithTypedAstFilesystem(
3294-
ImmutableList.of(), ImmutableList.of(), compilerOptions, typedAstListStream);
3312+
ImmutableList.of(),
3313+
ImmutableList.of(fileOne, fileTwo),
3314+
compilerOptions,
3315+
typedAstListStream);
32953316

32963317
// Then
32973318
Node insertedExterns = compiler.getExternsRoot().getOnlyChild();

‎test/com/google/javascript/jscomp/integration/TypedAstIntegrationTest.java

Lines changed: 156 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
import static com.google.javascript.rhino.testing.NodeSubject.assertNode;
2222
import static org.junit.Assert.assertThrows;
2323

24+
import com.google.common.base.Preconditions;
2425
import com.google.common.collect.ImmutableList;
2526
import com.google.common.collect.ImmutableMap;
27+
import com.google.common.collect.ImmutableSet;
2628
import com.google.javascript.jscomp.CompilationLevel;
2729
import com.google.javascript.jscomp.Compiler;
2830
import com.google.javascript.jscomp.CompilerOptions;
@@ -181,6 +183,157 @@ public void disambiguatesAndDeletesMethodsAcrossLibraries_withTranspilation() th
181183
.isEqualTo(expectedRoot);
182184
}
183185

186+
@Test
187+
public void disambiguatesAndDeletesMethodsAcrossLibraries_skippedIfInvalidatingTypeError()
188+
throws IOException {
189+
SourceFile lib1 = code("class Lib1 { m() { return 'lib1'; } n() { return 'delete me'; } }");
190+
SourceFile lib2 = code("class Lib2 { m() { return 'delete me'; } n() { return 'lib2'; } }");
191+
precompileLibrary(lib1);
192+
precompileLibrary(lib2);
193+
precompileLibrary(
194+
extern(new TestExternsBuilder().addAlert().build()),
195+
typeSummary(lib1),
196+
typeSummary(lib2),
197+
code("alert(new Lib1().m()); alert(new Lib2().n());"));
198+
// assigning an instance of Lib1 to a variable of type 'string' causes the disambiguator to
199+
// 'invalidate' the type of Lib1 and any associated properties.
200+
SourceFile invalidating =
201+
code("/** @suppress {checkTypes} @type {string} */ const str = new Lib1();");
202+
precompileLibrary(typeSummary(lib1), invalidating);
203+
204+
CompilerOptions options = new CompilerOptions();
205+
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
206+
options.setDependencyOptions(DependencyOptions.none());
207+
options.setDisambiguateProperties(true);
208+
options.setPropertiesThatMustDisambiguate(ImmutableSet.of("m"));
209+
210+
Compiler compiler = compileTypedAstShardsWithoutErrorChecks(options);
211+
212+
assertThat(compiler.getErrors())
213+
.comparingElementsUsing(JSCompCorrespondences.DESCRIPTION_EQUALITY)
214+
.containsExactly(
215+
"Property 'm' was required to be disambiguated but was invalidated.\n"
216+
+ "See go/disambiguation-errors for more information.");
217+
}
218+
219+
@Test
220+
public void disambiguatesAndDeletesMethodsAcrossLibraries_ignoresInvalidationsInUnusedShards()
221+
throws IOException {
222+
SourceFile lib1 = code("class Lib1 { m() { return 'lib1'; } n() { return 'delete me'; } }");
223+
SourceFile lib2 = code("class Lib2 { m() { return 'delete me'; } n() { return 'lib2'; } }");
224+
precompileLibrary(lib1);
225+
precompileLibrary(lib2);
226+
precompileLibrary(
227+
extern(new TestExternsBuilder().addAlert().build()),
228+
typeSummary(lib1),
229+
typeSummary(lib2),
230+
code("alert(new Lib1().m()); alert(new Lib2().n());"));
231+
// assigning an instance of Lib1 to a variable of type 'string' causes the disambiguator to
232+
// 'invalidate' the type of Lib1 and any associated properties.
233+
SourceFile invalidating =
234+
code("/** @suppress {checkTypes} @type {string} */ const str = new Lib1();");
235+
precompileLibrary(typeSummary(lib1), invalidating);
236+
237+
CompilerOptions options = new CompilerOptions();
238+
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
239+
options.setDependencyOptions(DependencyOptions.none());
240+
options.setDisambiguateProperties(true);
241+
options.setPropertiesThatMustDisambiguate(ImmutableSet.of("m"));
242+
243+
// Drop the invalidating source from the list of SourceFile inputs to JSCompiler.
244+
// However, leave in the associated TypedAST in this.shards.
245+
// We want to verify that JSCompiler is able to disambiguate properties on Lib1 despite the
246+
// invalidation in the unused TypedAST shard.
247+
Preconditions.checkState(this.sourceFiles.get(3) == invalidating, this.sourceFiles);
248+
this.sourceFiles.remove(3);
249+
250+
Compiler compiler = compileTypedAstShards(options);
251+
252+
Node expectedRoot = parseExpectedCode("", "", "alert('lib1'); alert('lib2')");
253+
assertNode(compiler.getRoot().getSecondChild())
254+
.usingSerializer(compiler::toSource)
255+
.isEqualTo(expectedRoot);
256+
}
257+
258+
@Test
259+
public void exportAnnotationOnPropertyPreventsRenaming() throws IOException {
260+
SourceFile externs = extern(new TestExternsBuilder().addAlert().build());
261+
SourceFile lib1 =
262+
code(
263+
lines(
264+
"class C {",
265+
" constructor(foo, bar) {",
266+
" this.foo = foo;",
267+
" this.bar = bar;",
268+
" }",
269+
"}",
270+
"alert(new C(1, 2))"));
271+
SourceFile lib2 = code("const obj = { /** @export */ foo: 0, bar: 1};");
272+
precompileLibrary(externs, lib1);
273+
precompileLibrary(lib2);
274+
275+
CompilerOptions options = new CompilerOptions();
276+
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
277+
options.setDependencyOptions(DependencyOptions.none());
278+
options.setDisambiguateProperties(true);
279+
280+
Compiler compiler = compileTypedAstShards(options);
281+
282+
Node expectedRoot =
283+
parseExpectedCode(
284+
lines(
285+
"class a {", //
286+
"constructor() { this.foo = 1; }",
287+
"}",
288+
"alert(new a());"),
289+
"");
290+
assertNode(compiler.getRoot().getSecondChild())
291+
.usingSerializer(compiler::toSource)
292+
.isEqualTo(expectedRoot);
293+
}
294+
295+
@Test
296+
public void exportAnnotationOnProperty_ignoredInUnusedTypedAstShard() throws IOException {
297+
SourceFile externs = extern(new TestExternsBuilder().addAlert().build());
298+
SourceFile lib1 =
299+
code(
300+
lines(
301+
"class C {",
302+
" constructor(foo, bar) {",
303+
" this.foo = foo;",
304+
" this.bar = bar;",
305+
" }",
306+
"}",
307+
"alert(new C(1, 2))"));
308+
SourceFile unusedLib = code("const obj = { /** @export */ foo: 0, bar: 1};");
309+
precompileLibrary(externs, lib1);
310+
precompileLibrary(unusedLib);
311+
312+
CompilerOptions options = new CompilerOptions();
313+
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
314+
options.setDependencyOptions(DependencyOptions.none());
315+
options.setDisambiguateProperties(true);
316+
317+
// Drop the unusedLib source from the list of SourceFile inputs to JSCompiler.
318+
// However, leave in the associated TypedAST in this.shards.
319+
// We want to verify that JSCompiler does /not/ pay attention to the @export in
320+
// the unusedLib file, as it's not part of the compilation.
321+
Preconditions.checkState(this.sourceFiles.size() == 2, this.sourceFiles);
322+
Preconditions.checkState(this.shards.size() == 2, this.shards);
323+
this.sourceFiles.remove(1);
324+
325+
Compiler compiler = compileTypedAstShards(options);
326+
327+
Node expectedRoot =
328+
parseExpectedCode(
329+
lines(
330+
"class a {}", //
331+
"alert(new a());"));
332+
assertNode(compiler.getRoot().getSecondChild())
333+
.usingSerializer(compiler::toSource)
334+
.isEqualTo(expectedRoot);
335+
}
336+
184337
@Test
185338
public void lateFulfilledGlobalVariableIsRenamed() throws IOException {
186339
SourceFile lib1 =
@@ -509,7 +662,9 @@ private Compiler compileTypedAstShardsWithoutErrorChecks(CompilerOptions options
509662
}
510663
compiler.parse();
511664
compiler.stage2Passes();
512-
compiler.stage3Passes();
665+
if (!compiler.hasErrors()) {
666+
compiler.stage3Passes();
667+
}
513668

514669
return compiler;
515670
}

0 commit comments

Comments
 (0)