-
Notifications
You must be signed in to change notification settings - Fork 396
Clear cached JS for stale types even when currently unreachable #10390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.function.Predicate; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| /** | ||
|
|
@@ -1502,6 +1503,127 @@ public void testIncrementalRecompile_dateStampChange() throws UnableToCompleteEx | |
| checkIncrementalRecompile_dateStampChange(JsOutputOption.DETAILED); | ||
| } | ||
|
|
||
| // Repro for issue #9565. A stale type that is not currently reachable must still have its | ||
| // cached JS cleared; otherwise, when it becomes reachable again in a later compile, its | ||
| // outdated JS is reused and can reference (or collide with) global names inconsistently with | ||
| // freshly generated code. | ||
| public void testIncrementalRecompile_unreachableStaleTypeRegainsReachability() | ||
| throws UnableToCompleteException, IOException, InterruptedException { | ||
| // Uses PRETTY output so that synthetic lambda type names are recognizable in the JS. | ||
| JsOutputOption output = JsOutputOption.PRETTY; | ||
|
|
||
| MockJavaResource widgetsWithThreeLambdas = | ||
| JavaResourceBase.createMockJavaResource("com.foo.Widgets", | ||
| "package com.foo;", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Recent PRs are using text blocks, this is one of the places where it could increase readability.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in aaf4998, matching the existing text block usage in |
||
| "public class Widgets {", | ||
| " interface IntFilter {", | ||
| " boolean test(int value);", | ||
| " }", | ||
| " public static int state;", | ||
| " public static void ping() {", | ||
| " state++;", | ||
| " }", | ||
| " public static void trigger() {", | ||
| " Runnable a = () -> state++;", | ||
| " int cursorRow = 10;", | ||
| " IntFilter filter = value -> value >= cursorRow;", | ||
| " Runnable c = () -> state--;", | ||
| " a.run();", | ||
| " if (filter.test(11)) {", | ||
| " c.run();", | ||
| " }", | ||
| " }", | ||
| "}"); | ||
| // The same type with an extra lambda inserted first, so that the synthetic lambda types of | ||
| // trigger() are renumbered and the name 'Widgets$lambda$1$Type' etc. now denote different | ||
| // lambdas than before. | ||
| MockJavaResource widgetsWithFourLambdas = | ||
| JavaResourceBase.createMockJavaResource("com.foo.Widgets", | ||
| "package com.foo;", | ||
| "public class Widgets {", | ||
| " interface IntFilter {", | ||
| " boolean test(int value);", | ||
| " }", | ||
| " public static int state;", | ||
| " public static void ping() {", | ||
| " state++;", | ||
| " }", | ||
| " public static void trigger() {", | ||
| " Runnable z = () -> state = state + 12345;", | ||
| " z.run();", | ||
| " Runnable a = () -> state++;", | ||
| " int cursorRow = 10;", | ||
| " IntFilter filter = value -> value >= cursorRow;", | ||
| " Runnable c = () -> state--;", | ||
| " a.run();", | ||
| " if (filter.test(11)) {", | ||
| " c.run();", | ||
| " }", | ||
| " }", | ||
| "}"); | ||
| MockJavaResource entryPointCallingTrigger = | ||
| JavaResourceBase.createMockJavaResource("com.foo.TestEntryPoint", | ||
| "package com.foo;", | ||
| "import com.google.gwt.core.client.EntryPoint;", | ||
| "public class TestEntryPoint implements EntryPoint {", | ||
| " @Override", | ||
| " public void onModuleLoad() {", | ||
| " Widgets.ping();", | ||
| " Widgets.trigger();", | ||
| " }", | ||
| "}"); | ||
| MockJavaResource entryPointNotCallingTrigger = | ||
| JavaResourceBase.createMockJavaResource("com.foo.TestEntryPoint", | ||
| "package com.foo;", | ||
| "import com.google.gwt.core.client.EntryPoint;", | ||
| "public class TestEntryPoint implements EntryPoint {", | ||
| " @Override", | ||
| " public void onModuleLoad() {", | ||
| " Widgets.ping();", | ||
| " }", | ||
| "}"); | ||
|
|
||
| MinimalRebuildCache relinkMinimalRebuildCache = new MinimalRebuildCache(); | ||
| File relinkApplicationDir = createTempDir(); | ||
|
|
||
| // Compile the app so that the lambda types in Widgets.trigger() are reachable and their JS is | ||
| // cached. | ||
| compileToJs(relinkApplicationDir, "com.foo.SimpleModule", Lists.newArrayList( | ||
| simpleModuleResource, entryPointCallingTrigger, widgetsWithThreeLambdas), | ||
| relinkMinimalRebuildCache, null, output); | ||
|
|
||
| // Stop calling trigger(). Its lambda types become unreachable, but since Widgets was not | ||
| // modified their cached JS is retained. | ||
| compileToJs(relinkApplicationDir, "com.foo.SimpleModule", | ||
| Lists.<MockResource> newArrayList(entryPointNotCallingTrigger), relinkMinimalRebuildCache, | ||
| null, output); | ||
|
|
||
| // Call trigger() again and modify Widgets so that its lambda types are renumbered. The lambda | ||
| // types are stale but were unreachable in the previous compile; their outdated cached JS must | ||
| // not leak into the output. | ||
| String relinkedJs = compileToJs(relinkApplicationDir, "com.foo.SimpleModule", | ||
| Lists.<MockResource> newArrayList(entryPointCallingTrigger, widgetsWithFourLambdas), | ||
| relinkMinimalRebuildCache, null, output); | ||
|
|
||
| // The output must contain the current version of trigger(), not the cached stale one. | ||
| assertTrue("expected the regenerated trigger() body to be present in the output", | ||
| relinkedJs.contains("12345")); | ||
|
|
||
| // Every referenced lambda type constructor must be defined in the output. A dangling | ||
| // reference indicates that a stale, differently-named version of the type was reused. | ||
| Matcher useMatcher = | ||
| Pattern.compile("new\\s+([\\w$]*Widgets\\$lambda\\$\\d+\\$Type[\\w$]*)\\(") | ||
| .matcher(relinkedJs); | ||
| int lambdaCtorUses = 0; | ||
| while (useMatcher.find()) { | ||
| lambdaCtorUses++; | ||
| String usedCtorName = useMatcher.group(1); | ||
| assertTrue("lambda type constructor " + usedCtorName + " is referenced but never defined", | ||
| relinkedJs.contains("function " + usedCtorName + "(")); | ||
| } | ||
| assertTrue("expected at least one lambda type constructor reference", lambdaCtorUses > 0); | ||
| } | ||
|
|
||
| // Repro for bug #9518 | ||
| public void testIncrementalRecompile_jsPropertyConsistencyCheck() | ||
| throws UnableToCompleteException, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the Javadoc for the method should make it clear that the set of types that are removed from cache is not the same as the set returned from this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point -- the two sets diverged with this change. Added a paragraph in 756c8a6 noting that cached output is cleared for every stale type while the returned set is narrowed to the reachable ones.