Skip to content

Commit

Permalink
Move RemoveSuperMethodsPass before OptimizeCalls
Browse files Browse the repository at this point in the history
OptimizeCalls can change the number of parameters, which breaks the assumption
of RemoveSuperMethodsPass in some cases.

Fixes #2122 on GitHub.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=137999659
  • Loading branch information
Dominator008 authored and blickly committed Nov 3, 2016
1 parent f3a36c9 commit 58e612c
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 7 deletions.
27 changes: 20 additions & 7 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
Expand Up @@ -222,7 +222,7 @@ protected List<PassFactory> getTranspileOnlyPasses() {
}

assertAllOneTimePasses(passes);
assertValidOrder(passes);
assertValidOrderForChecks(passes);
return passes;
}

Expand Down Expand Up @@ -422,7 +422,7 @@ protected List<PassFactory> getChecks() {
checks.add(createEmptyPass("afterStandardChecks"));

assertAllOneTimePasses(checks);
assertValidOrder(checks);
assertValidOrderForChecks(checks);

return checks;
}
Expand Down Expand Up @@ -731,14 +731,15 @@ protected List<PassFactory> getOptimizations() {
passes.add(flowSensitiveInlineVariables);
}

passes.addAll(getMainOptimizationLoop());
passes.add(createEmptyPass("afterMainOptimizations"));

// Must run after ProcessClosurePrimitives, Es6ConvertSuper, and assertion removals.
// Must run after ProcessClosurePrimitives, Es6ConvertSuper, and assertion removals, but
// before OptimizeCalls (specifically, OptimizeParameters).
if (options.removeSuperMethods) {
passes.add(removeSuperMethodsPass);
}

passes.addAll(getMainOptimizationLoop());
passes.add(createEmptyPass("afterMainOptimizations"));

passes.add(createEmptyPass("beforeModuleMotion"));

if (options.crossModuleCodeMotion) {
Expand Down Expand Up @@ -912,6 +913,7 @@ protected List<PassFactory> getOptimizations() {
passes.add(rewriteBindThis);
}

assertValidOrderForOptimizations(passes);
return passes;
}

Expand Down Expand Up @@ -1086,7 +1088,7 @@ private void assertPassOrder(
* This enforces those constraints.
* @param checks The list of check passes
*/
private void assertValidOrder(List<PassFactory> checks) {
private void assertValidOrderForChecks(List<PassFactory> checks) {
assertPassOrder(
checks,
closureRewriteModule,
Expand Down Expand Up @@ -1147,6 +1149,17 @@ private void assertValidOrder(List<PassFactory> checks) {
+ "removing assertions may make more super calls eligible to be stripped.");
}

/**
* Certain optimizations need to run in a particular order. For example, OptimizeCalls must run
* before RemoveSuperMethodsPass, because the former can invalidate assumptions in the latter.
* This enforces those constraints.
* @param optimizations The list of optimization passes
*/
private void assertValidOrderForOptimizations(List<PassFactory> optimizations) {
assertPassOrder(optimizations, removeSuperMethodsPass, optimizeCalls,
"RemoveSuperMethodsPass must run before OptimizeCalls");
}

/** Checks that all constructed classes are goog.require()d. */
private final HotSwapPassFactory checkRequires =
new HotSwapPassFactory("checkRequires", true) {
Expand Down
37 changes: 37 additions & 0 deletions test/com/google/javascript/jscomp/IntegrationTest.java
Expand Up @@ -3734,6 +3734,43 @@ public void testStrictEqualityComparisonAgainstUndefined() {
"null===window['c']&&(window['d']=12)");
}

// GitHub issue #2122: https://github.com/google/closure-compiler/issues/2122
public void testNoRemoveSuperCallWithWrongArgumentCount() {
CompilerOptions options = createCompilerOptions();
options.setCheckTypes(true);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
test(options,
LINE_JOINER.join(
"var goog = {}",
"goog.inherits = function(childCtor, parentCtor) {",
" childCtor.superClass_ = parentCtor.prototype;",
"};",
"/** @constructor */",
"var Foo = function() {}",
"/**",
" * @param {number=} width",
" * @param {number=} height",
" */",
"Foo.prototype.resize = function(width, height) {",
" window.size = width * height;",
"}",
"/**",
" * @constructor",
" * @extends {Foo}",
" */",
"var Bar = function() {}",
"goog.inherits(Bar, Foo);",
"/** @override */",
"Bar.prototype.resize = function(width, height) {",
" goog.base(this, 'resize', width);",
"};",
"(new Bar).resize(100, 200);"),
LINE_JOINER.join(
"function a(){}a.prototype.a=function(b,e){window.c=b*e};",
"function d(){}d.b=a.prototype;d.prototype.a=function(b){d.b.a.call(this,b)};",
"(new d).a(100, 200);"));
}

/** Creates a CompilerOptions object with google coding conventions. */
@Override
protected CompilerOptions createCompilerOptions() {
Expand Down

0 comments on commit 58e612c

Please sign in to comment.