From 1776c1563abe1fea4a03cd3485f21df3951aae02 Mon Sep 17 00:00:00 2001 From: sdh Date: Sun, 27 Aug 2017 18:37:12 -0700 Subject: [PATCH] Prevent calls to getLastCompiler() from TypeICompilerTestCase. When TypeInferenceMode is BOTH, this call is practically guaranteed to be an error. The entire purpose is to do some additional check on the AST or some other aspect of the compile, but in BOTH mode, there are two different compilers created, and only the second one (NTI) is accessible afterwards. The fix is to add a new Postcondition TestPart to CompilerTestCase, which contains a callback that accepts the Compiler and is called at the end of CompilerTestCase#testInternal, so that all test cases can be verified separately. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=166652077 --- .../javascript/jscomp/CompilerTestCase.java | 131 ++++++++---- .../jscomp/DisambiguatePropertiesTest.java | 112 ++++++---- .../jscomp/PureFunctionIdentifierTest.java | 199 +++++++++--------- .../jscomp/TypeICompilerTestCase.java | 47 ++++- 4 files changed, 290 insertions(+), 199 deletions(-) diff --git a/test/com/google/javascript/jscomp/CompilerTestCase.java b/test/com/google/javascript/jscomp/CompilerTestCase.java index 75ed84d3542..b96d4eab337 100644 --- a/test/com/google/javascript/jscomp/CompilerTestCase.java +++ b/test/com/google/javascript/jscomp/CompilerTestCase.java @@ -46,6 +46,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.function.Consumer; import junit.framework.TestCase; /** @@ -874,16 +875,16 @@ protected void testError(String js, DiagnosticType error, String description) { /** * Verifies that the compiler generates the given error and description for the given input. */ - protected void testError(Sources srcs, ErrorDiagnostic error) { - assertNotNull(error); + protected void testError(Sources srcs, Diagnostic error) { + assertThat(error.level).isEqualTo(CheckLevel.ERROR); test(srcs, error); } /** * Verifies that the compiler generates the given error and description for the given input. */ - protected void testError(Externs externs, Sources srcs, ErrorDiagnostic error) { - assertNotNull(error); + protected void testError(Externs externs, Sources srcs, Diagnostic error) { + assertThat(error.level).isEqualTo(CheckLevel.ERROR); test(externs, srcs, error); } @@ -901,17 +902,17 @@ protected void testError(String[] js, DiagnosticType error) { /** * Verifies that the compiler generates the given warning for the given input. */ - protected void testError(List inputs, DiagnosticType warning) { - assertNotNull(warning); - test(srcs(inputs), error(warning)); + protected void testError(List inputs, DiagnosticType error) { + assertNotNull(error); + test(srcs(inputs), error(error)); } /** * Verifies that the compiler generates the given warning for the given input. */ - protected void testError(List inputs, DiagnosticType warning, String description) { - assertNotNull(warning); - test(srcs(inputs), error(warning, description)); + protected void testError(List inputs, DiagnosticType error, String description) { + assertNotNull(error); + test(srcs(inputs), error(error, description)); } /** @@ -931,8 +932,8 @@ protected void testWarning(String js, DiagnosticType warning) { * @param srcs Inputs * @param warning Expected warning */ - protected void testWarning(Sources srcs, WarningDiagnostic warning) { - assertNotNull(warning); + protected void testWarning(Sources srcs, Diagnostic warning) { + assertThat(warning.level).isEqualTo(CheckLevel.WARNING); test(srcs, warning); } @@ -943,8 +944,8 @@ protected void testWarning(Sources srcs, WarningDiagnostic warning) { * @param srcs The input * @param warning Expected warning */ - protected void testWarning(Externs externs, Sources srcs, WarningDiagnostic warning) { - assertNotNull(warning); + protected void testWarning(Externs externs, Sources srcs, Diagnostic warning) { + assertThat(warning.level).isEqualTo(CheckLevel.WARNING); test(externs, srcs, warning); } @@ -1071,7 +1072,8 @@ protected void testInternal( Externs externs, Sources inputs, Expected expected, - Diagnostic diagnostic) { + Diagnostic diagnostic, + List postconditions) { Compiler compiler = createCompiler(); lastCompiler = compiler; @@ -1089,7 +1091,7 @@ protected void testInternal( BaseJSTypeTestCase.addNativeProperties(compiler.getTypeRegistry()); } - testInternal(compiler, inputs, expected, diagnostic); + testInternal(compiler, inputs, expected, diagnostic, postconditions); } private static List maybeCreateSources(String name, String srcText) { @@ -1300,11 +1302,12 @@ protected void testSame(JSModule[] modules, DiagnosticType warning) { * @param expectedObj Expected outputs * @param diagnostic Expected warning/error diagnostic */ - protected void testInternal( + private void testInternal( Compiler compiler, Sources inputsObj, // TODO remove this parameter Expected expectedObj, - Diagnostic diagnostic) { + Diagnostic diagnostic, + List postconditions) { List inputs = (inputsObj instanceof FlatSources) ? ((FlatSources) inputsObj).sources @@ -1331,8 +1334,9 @@ protected void testInternal( assertWithMessage("Unexpected parse error(s): " + errorMsg) .that(actualError.getType()) .isEqualTo(diagnostic.diagnostic); - if (diagnostic.match != null) { - assertThat(actualError.description).isEqualTo(diagnostic.match); + diagnostic.verifyMessage(actualError.description); + for (Postcondition postcondition : postconditions) { + postcondition.verify(compiler); } return; } @@ -1529,10 +1533,7 @@ protected void testInternal( JSError actual = warnings[0]; assertError(actual).hasType(diagnostic.diagnostic); validateSourceLocation(actual); - - if (diagnostic.match != null) { - assertThat(actual.description).isEqualTo(diagnostic.match); - } + diagnostic.verifyMessage(actual.description); } } @@ -1650,9 +1651,7 @@ protected void testInternal( JSError actualError = compiler.getErrors()[0]; assertEquals(errorMsg, diagnostic.diagnostic, actualError.getType()); validateSourceLocation(actualError); - if (diagnostic.match != null) { - assertThat(actualError.description).isEqualTo(diagnostic.match); - } + diagnostic.verifyMessage(actualError.description); assertWithMessage("Some placeholders in the error message were not replaced") .that(actualError.description) .doesNotContainMatch("\\{\\d\\}"); @@ -1669,6 +1668,9 @@ protected void testInternal( assertEquals(warnings, diagnostic.diagnostic, compiler.getWarnings()[0].getType()); } } + for (Postcondition postcondition : postconditions) { + postcondition.verify(compiler); + } } private static void transpileToEs5(AbstractCompiler compiler, Node externsRoot, Node codeRoot) { @@ -2026,22 +2028,30 @@ protected static Externs externs(List files) { return new Externs(files); } - protected static WarningDiagnostic warning(DiagnosticType type) { - return warning(type, null); + protected static Diagnostic warning(DiagnosticType type) { + return new WarningDiagnostic(type); } - protected static WarningDiagnostic warning(DiagnosticType type, String match) { + protected static Diagnostic warning(DiagnosticType type, String match) { // TODO(johnlenz): change this to reject null - return type != null ? new WarningDiagnostic(type, match) : null; + if (type == null) { + return null; + } + Diagnostic diagnostic = warning(type); + return match != null ? diagnostic.withMessage(match) : diagnostic; } - protected static ErrorDiagnostic error(DiagnosticType type) { - return error(type, null); + protected static Diagnostic error(DiagnosticType type) { + return new ErrorDiagnostic(type); } - protected static ErrorDiagnostic error(DiagnosticType type, String match) { + protected static Diagnostic error(DiagnosticType type, String match) { // TODO(johnlenz): change this to reject null - return type != null ? new ErrorDiagnostic(type, match) : null; + if (type == null) { + return null; + } + Diagnostic diagnostic = error(type); + return match != null ? diagnostic.withMessage(match) : diagnostic; } protected void testSame(TestPart... parts) { @@ -2058,6 +2068,7 @@ private void testInternal(Iterable parts) { Sources srcs = null; Expected expected = null; Diagnostic diagnostic = null; + List postconditions = new ArrayList<>(); for (TestPart part : parts) { if (part instanceof Externs) { checkState(externs == null); @@ -2071,6 +2082,8 @@ private void testInternal(Iterable parts) { } else if (part instanceof Diagnostic) { checkState(diagnostic == null); diagnostic = (Diagnostic) part; + } else if (part instanceof Postcondition) { + postconditions.add((Postcondition) part); } else { throw new IllegalStateException("unexepected " + part.getClass().getName()); } @@ -2081,7 +2094,7 @@ private void testInternal(Iterable parts) { if (externs == null) { externs = externs(externsInputs); } - testInternal(externs, srcs, expected, diagnostic); + testInternal(externs, srcs, expected, diagnostic, postconditions); } private static Expected fromSources(Sources srcs) { @@ -2158,24 +2171,52 @@ protected static final class Externs implements TestPart { protected static class Diagnostic implements TestPart { final CheckLevel level; final DiagnosticType diagnostic; - final String match; + final Consumer messagePostcondition; - Diagnostic(CheckLevel level, DiagnosticType diagnostic, String match) { + Diagnostic(CheckLevel level, DiagnosticType diagnostic, Consumer messagePostcondition) { this.level = level; this.diagnostic = diagnostic; - this.match = match; + this.messagePostcondition = messagePostcondition; + } + + protected void verifyMessage(String message) { + if (messagePostcondition != null) { + messagePostcondition.accept(message); + } + } + + protected Diagnostic withMessage(final String expected) { + checkState(messagePostcondition == null); + return new Diagnostic(level, diagnostic, new Consumer() { + @Override public void accept(String message) { + assertThat(message).isEqualTo(expected); + } + }); + } + + protected Diagnostic withMessageContaining(final String substring) { + checkState(messagePostcondition == null); + return new Diagnostic(level, diagnostic, new Consumer() { + @Override public void accept(String message) { + assertThat(message).contains(substring); + } + }); } } - protected static class ErrorDiagnostic extends Diagnostic { - ErrorDiagnostic(DiagnosticType diagnostic, String match) { - super(CheckLevel.ERROR, diagnostic, match); + private static class ErrorDiagnostic extends Diagnostic { + ErrorDiagnostic(DiagnosticType diagnostic) { + super(CheckLevel.ERROR, diagnostic, null); } } - protected static class WarningDiagnostic extends Diagnostic { - WarningDiagnostic(DiagnosticType diagnostic, String match) { - super(CheckLevel.WARNING, diagnostic, match); + private static class WarningDiagnostic extends Diagnostic { + WarningDiagnostic(DiagnosticType diagnostic) { + super(CheckLevel.WARNING, diagnostic, null); } } + + protected abstract static class Postcondition implements TestPart { + abstract void verify(Compiler compiler); + } } diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java index e96b8487e11..0559818e1dc 100644 --- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java @@ -15,8 +15,6 @@ */ package com.google.javascript.jscomp; -import static com.google.common.truth.Truth.assertThat; - import com.google.common.collect.Multimap; import com.google.javascript.rhino.Node; import java.util.Collection; @@ -2275,42 +2273,44 @@ public void testUnsafeTypingOfThis() { } public void testErrorOnProtectedProperty() { - testError("function addSingletonGetter(foo) { foo.foobar = 'a'; };", - DisambiguateProperties.Warnings.INVALIDATION); - assertThat(getLastCompiler().getErrors()[0].toString()).contains("foobar"); + test( + srcs("function addSingletonGetter(foo) { foo.foobar = 'a'; };"), + error(DisambiguateProperties.Warnings.INVALIDATION).withMessageContaining("foobar")); } public void testMismatchForbiddenInvalidation() { - testError( - "/** @constructor */ function F() {}" - + "/** @type {number} */ F.prototype.foobar = 3;" - + "/** @return {number} */ function g() { return new F(); }", - DisambiguateProperties.Warnings.INVALIDATION); - assertThat(getLastCompiler().getErrors()[0].toString()).contains("Consider fixing errors"); + test( + srcs(lines( + "/** @constructor */ function F() {}", + "/** @type {number} */ F.prototype.foobar = 3;", + "/** @return {number} */ function g() { return new F(); }")), + error(DisambiguateProperties.Warnings.INVALIDATION) + .withMessageContaining("Consider fixing errors")); } public void testUnionTypeInvalidationError() { - String externs = "" - + "/** @constructor */ function Baz() {}" - + "Baz.prototype.foobar"; - String js = "" - + "/** @constructor */ function Ind() {this.foobar=0}\n" - + "/** @constructor */ function Foo() {}\n" - + "Foo.prototype.foobar = 0;\n" - + "/** @constructor */ function Bar() {}\n" - + "Bar.prototype.foobar = 0;\n" - + "/** @type {Foo|Bar} */\n" - + "var F = new Foo;\n" - + "F.foobar = 1\n;" - + "F = new Bar;\n" - + "/** @type {Baz} */\n" - + "var Z = new Baz;\n" - + "Z.foobar = 1\n;"; + String externs = lines( + "/** @constructor */ function Baz() {}", + "Baz.prototype.foobar"); + String js = lines( + "/** @constructor */ function Ind() {this.foobar=0}", + "/** @constructor */ function Foo() {}", + "Foo.prototype.foobar = 0;", + "/** @constructor */ function Bar() {}", + "Bar.prototype.foobar = 0;", + "/** @type {Foo|Bar} */", + "var F = new Foo;", + "F.foobar = 1;", + "F = new Bar;", + "/** @type {Baz} */", + "var Z = new Baz;", + "Z.foobar = 1;\n"); - testError( - DEFAULT_EXTERNS + externs, js, - DisambiguateProperties.Warnings.INVALIDATION_ON_TYPE); - assertThat(getLastCompiler().getErrors()[0].toString()).contains("foobar"); + test( + externs(DEFAULT_EXTERNS + externs), + srcs(js), + error(DisambiguateProperties.Warnings.INVALIDATION_ON_TYPE) + .withMessageContaining("foobar")); } public void testDontCrashOnNonConstructorsWithPrototype() { @@ -2321,20 +2321,34 @@ public void testDontCrashOnNonConstructorsWithPrototype() { test(DEFAULT_EXTERNS + externs, "" , ""); } - private void testSets(String js, String expected, String fieldTypes) { - test(js, expected); - assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + private void testSets(String js, String expected, final String fieldTypes) { + test(srcs(js), expected(expected), new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + } + }); } - private void testSets(String externs, String js, String expected, String fieldTypes) { - test(DEFAULT_EXTERNS + externs, js, expected); - assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + private void testSets(String externs, String js, String expected, final String fieldTypes) { + test(externs(DEFAULT_EXTERNS + externs), srcs(js), expected(expected), new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + } + }); } private void testSets(String externs, String js, String expected, - String fieldTypes, DiagnosticType warning, String description) { - test(DEFAULT_EXTERNS + externs, js, expected, warning(warning, description)); - assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + final String fieldTypes, DiagnosticType warning, String description) { + test( + externs(DEFAULT_EXTERNS + externs), + srcs(js), + expected(expected), + warning(warning, description), + new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + } + }); } /** @@ -2344,9 +2358,12 @@ private void testSets(String externs, String js, String expected, *

The format for the set of types for fields is: * {field=[[Type1, Type2]]} */ - private void testSets(String js, String fieldTypes) { - testNoWarning(js); - assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + private void testSets(String js, final String fieldTypes) { + test(srcs(js), new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + } + }); } /** @@ -2356,9 +2373,12 @@ private void testSets(String js, String fieldTypes) { *

The format for the set of types for fields is: * {field=[[Type1, Type2]]} */ - private void testSets(String js, String fieldTypes, DiagnosticType warning) { - testWarning(js, warning); - assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + private void testSets(String js, final String fieldTypes, DiagnosticType warning) { + test(srcs(js), warning(warning), new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(fieldTypes, mapToString(lastPass.getRenamedTypesForTesting())); + } + }); } /** Sorts the map and converts to a string for comparison purposes. */ diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 95eab013905..bf35da22017 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -187,6 +187,12 @@ protected int getNumRepetitions() { return 1; } + @Override + protected void setUp() throws Exception { + super.setUp(); + ignoreWarnings(NewTypeInference.GLOBAL_THIS, NewTypeInference.INEXISTENT_PROPERTY); + } + @Override protected void tearDown() throws Exception { super.tearDown(); @@ -1291,16 +1297,7 @@ public void testAmbiguousDefinitionsCallWithThis() throws Exception { ); // Can't tell which modifiesThis is being called so it assumes both. - - this.mode = TypeInferenceMode.OTI_ONLY; assertPureCallsMarked(source, ImmutableList.of("Constructor")); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked( - source, - ImmutableList.of("Constructor"), - // modifiesThis not defined on Constructor - NewTypeInference.GLOBAL_THIS); } public void testAmbiguousDefinitionsBothCallThis() throws Exception { @@ -1316,12 +1313,7 @@ public void testAmbiguousDefinitionsBothCallThis() throws Exception { " this.x = 2;", "}", "new C();"); - - this.mode = TypeInferenceMode.OTI_ONLY; assertPureCallsMarked(source, ImmutableList.of("C")); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked(source, ImmutableList.of("C"), NewTypeInference.GLOBAL_THIS); } public void testAmbiguousDefinitionsAllCallThis() throws Exception { @@ -1336,12 +1328,7 @@ public void testAmbiguousDefinitionsAllCallThis() throws Exception { "new h();", "i();" // With better locals tracking i could be identified as pure ); - - this.mode = TypeInferenceMode.OTI_ONLY; assertPureCallsMarked(source, ImmutableList.of("F.f.apply", "h")); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked(source, ImmutableList.of("F.f.apply", "h"), NewTypeInference.GLOBAL_THIS); } public void testAmbiguousDefinitionsMutatesGlobalArgument() throws Exception { @@ -1454,15 +1441,7 @@ public void testAmbiguousDefinitionsDoubleDefinition6() { public void testCallBeforeDefinition() throws Exception { assertPureCallsMarked("f(); function f(){}", ImmutableList.of("f")); - - this.mode = TypeInferenceMode.OTI_ONLY; assertPureCallsMarked("var a = {}; a.f(); a.f = function (){}", ImmutableList.of("a.f")); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked( - "var a = {}; a.f(); a.f = function (){}", - ImmutableList.of("a.f"), - NewTypeInference.INEXISTENT_PROPERTY); } public void testConstructorThatModifiesThis1() throws Exception { @@ -1503,12 +1482,7 @@ public void testConstructorThatModifiesThis4() throws Exception { "function f() {return new A}", "f()" ); - - this.mode = TypeInferenceMode.OTI_ONLY; assertPureCallsMarked(source, ImmutableList.of("A", "f")); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked(source, ImmutableList.of("A", "f"), NewTypeInference.GLOBAL_THIS); } public void testConstructorThatModifiesGlobal1() throws Exception { @@ -1751,24 +1725,16 @@ public void testFunctionProperties1() throws Exception { "g.call(x);", "x.bar();" ); - - this.mode = TypeInferenceMode.OTI_ONLY; - assertPureCallsMarked(source, ImmutableList.of("F")); - Node lastRoot = getLastCompiler().getRoot(); - Node call = findQualifiedNameNode("g.call", lastRoot).getParent(); - assertEquals( - new Node.SideEffectFlags() - .clearAllFlags().setMutatesArguments().valueOf(), - call.getSideEffectFlags()); - - this.mode = TypeInferenceMode.NTI_ONLY; - assertPureCallsMarked(source, ImmutableList.of("F"), NewTypeInference.GLOBAL_THIS); - lastRoot = getLastCompiler().getRoot(); - call = findQualifiedNameNode("g.call", lastRoot).getParent(); - assertEquals( - new Node.SideEffectFlags() - .clearAllFlags().setMutatesArguments().valueOf(), - call.getSideEffectFlags()); + assertPureCallsMarked(source, ImmutableList.of("F"), new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot(); + Node call = findQualifiedNameNode("g.call", lastRoot).getParent(); + assertEquals( + new Node.SideEffectFlags() + .clearAllFlags().setMutatesArguments().valueOf(), + call.getSideEffectFlags()); + } + }); } public void testCallCache() throws Exception { @@ -1776,11 +1742,14 @@ public void testCallCache() throws Exception { "var valueFn = function() {};", "goog.reflect.cache(externObj, \"foo\", valueFn)" ); - assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache")); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isTrue(); - assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache"), new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isTrue(); + assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + } + }); } public void testCallCache_withKeyFn() throws Exception { @@ -1789,20 +1758,26 @@ public void testCallCache_withKeyFn() throws Exception { "var keyFn = function(v) { return v };", "goog.reflect.cache(externObj, \"foo\", valueFn, keyFn)" ); - assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache")); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isTrue(); - assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache"), new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isTrue(); + assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + } + }); } public void testCallCache_anonymousFn() throws Exception { String source = "goog.reflect.cache(externObj, \"foo\", function(v) { return v })"; - assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache")); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isTrue(); - assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache"), new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isTrue(); + assertThat(call.mayMutateGlobalStateOrThrow()).isFalse(); + } + }); } public void testCallCache_anonymousFn_hasSideEffects() throws Exception { @@ -1810,11 +1785,14 @@ public void testCallCache_anonymousFn_hasSideEffects() throws Exception { "var x = 0;", "goog.reflect.cache(externObj, \"foo\", function(v) { return (x+=1) })" ); - assertNoPureCalls(source); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isFalse(); - assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + assertNoPureCalls(source, new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isFalse(); + assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + } + }); } public void testCallCache_hasSideEffects() throws Exception { @@ -1823,11 +1801,14 @@ public void testCallCache_hasSideEffects() throws Exception { "var valueFn = function() { return (x+=1); };", "goog.reflect.cache(externObj, \"foo\", valueFn)" ); - assertNoPureCalls(source); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isFalse(); - assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + assertNoPureCalls(source, new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isFalse(); + assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + } + }); } public void testCallCache_withKeyFn_hasSideEffects() throws Exception { @@ -1837,11 +1818,14 @@ public void testCallCache_withKeyFn_hasSideEffects() throws Exception { "var valueFn = function(v) { return v };", "goog.reflect.cache(externObj, \"foo\", valueFn, keyFn)" ); - assertNoPureCalls(source); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(call.isNoSideEffectsCall()).isFalse(); - assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + assertNoPureCalls(source, new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node call = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(call.isNoSideEffectsCall()).isFalse(); + assertThat(call.mayMutateGlobalStateOrThrow()).isTrue(); + } + }); } public void testCallCache_propagatesSideEffects() throws Exception { @@ -1850,38 +1834,53 @@ public void testCallCache_propagatesSideEffects() throws Exception { "var helper = function(x) { return goog.reflect.cache(externObj, x, valueFn); };", "helper(10);" ); - assertPureCallsMarked(source, ImmutableList.of("goog.reflect.cache", "helper")); - Node lastRoot = getLastCompiler().getRoot().getLastChild(); - Node cacheCall = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); - assertThat(cacheCall.isNoSideEffectsCall()).isTrue(); - assertThat(cacheCall.mayMutateGlobalStateOrThrow()).isFalse(); - - Node helperCall = Iterables.getLast(findQualifiedNameNodes("helper", lastRoot)).getParent(); - assertThat(helperCall.isNoSideEffectsCall()).isTrue(); - assertThat(helperCall.mayMutateGlobalStateOrThrow()).isFalse(); + assertPureCallsMarked( + source, + ImmutableList.of("goog.reflect.cache", "helper"), + new Postcondition() { + @Override public void verify(Compiler compiler) { + Node lastRoot = compiler.getRoot().getLastChild(); + Node cacheCall = findQualifiedNameNode("goog.reflect.cache", lastRoot).getParent(); + assertThat(cacheCall.isNoSideEffectsCall()).isTrue(); + assertThat(cacheCall.mayMutateGlobalStateOrThrow()).isFalse(); + + Node helperCall = + Iterables.getLast(findQualifiedNameNodes("helper", lastRoot)).getParent(); + assertThat(helperCall.isNoSideEffectsCall()).isTrue(); + assertThat(helperCall.mayMutateGlobalStateOrThrow()).isFalse(); + } + }); } void assertNoPureCalls(String source) { assertPureCallsMarked(source, ImmutableList.of(), null); } + void assertNoPureCalls(String source, Postcondition post) { + assertPureCallsMarked(source, ImmutableList.of(), post); + } + void assertPureCallsMarked(String source, List expected) { assertPureCallsMarked(source, expected, null); } - void assertPureCallsMarked(String source, List expected, DiagnosticType warning) { - if (warning != null) { - testSame(source, warning); - } else { - testSame(source); - } - assertEquals(expected, noSideEffectCalls); + void assertPureCallsMarked(String source, final List expected, final Postcondition post) { + testSame(srcs(source), new Postcondition() { + @Override public void verify(Compiler compiler) { + assertEquals(expected, noSideEffectCalls); + if (post != null) { + post.verify(compiler); + } + } + }); } - - void checkLocalityOfMarkedCalls(String source, List expected) { - testSame(source); - assertEquals(expected, localResultCalls); + void checkLocalityOfMarkedCalls(String source, final List expected) { + testSame(srcs(source), new Postcondition() { + @Override public void verify(Compiler unused) { + assertEquals(expected, localResultCalls); + } + }); } @Override diff --git a/test/com/google/javascript/jscomp/TypeICompilerTestCase.java b/test/com/google/javascript/jscomp/TypeICompilerTestCase.java index a1edcddc7c5..a7ed161eea5 100644 --- a/test/com/google/javascript/jscomp/TypeICompilerTestCase.java +++ b/test/com/google/javascript/jscomp/TypeICompilerTestCase.java @@ -17,6 +17,7 @@ package com.google.javascript.jscomp; import java.io.IOException; +import java.util.List; /** * CompilerTestCase for passes that run after type checking and use type information. @@ -70,18 +71,22 @@ protected void setUp() throws Exception { @Override protected void testInternal( - Externs externs, Sources js, Expected expected, Diagnostic diagnostic) { + Externs externs, + Sources js, + Expected expected, + Diagnostic diagnostic, + List postconditions) { if (this.mode.runsOTI()) { - testOTI(externs, js, expected, diagnostic); + testOTI(externs, js, expected, diagnostic, postconditions); } if (this.mode.runsNTI()) { if (!findMinimalExterns(externs.externs)) { fail("NTI reqires at least the MINIMAL_EXTERNS"); } - testNTI(externs, js, expected, diagnostic); + testNTI(externs, js, expected, diagnostic, postconditions); } if (this.mode.runsNeither()) { - super.testInternal(externs, js, expected, diagnostic); + super.testInternal(externs, js, expected, diagnostic, postconditions); } } @@ -98,20 +103,36 @@ private static boolean findMinimalExterns(Iterable externs) { return false; } - private void testOTI(Externs externs, Sources js, Expected expected, Diagnostic diagnostic) { + private void testOTI( + Externs externs, + Sources js, + Expected expected, + Diagnostic diagnostic, + List postconditions) { + TypeInferenceMode saved = this.mode; + this.mode = TypeInferenceMode.OTI_ONLY; enableTypeCheck(); Diagnostic oti = diagnostic instanceof OtiNtiDiagnostic ? ((OtiNtiDiagnostic) diagnostic).oti : diagnostic; - super.testInternal(externs, js, expected, oti); + super.testInternal(externs, js, expected, oti, postconditions); disableTypeCheck(); + this.mode = saved; } - private void testNTI(Externs externs, Sources js, Expected expected, Diagnostic diagnostic) { + private void testNTI( + Externs externs, + Sources js, + Expected expected, + Diagnostic diagnostic, + List postconditions) { + TypeInferenceMode saved = this.mode; + this.mode = TypeInferenceMode.NTI_ONLY; enableNewTypeInference(); Diagnostic nti = diagnostic instanceof OtiNtiDiagnostic ? ((OtiNtiDiagnostic) diagnostic).nti : diagnostic; - super.testInternal(externs, js, expected, nti); + super.testInternal(externs, js, expected, nti, postconditions); disableNewTypeInference(); + this.mode = saved; } void testWarningOtiNti( @@ -124,6 +145,16 @@ void testWarningOtiNti( this.mode = saved; } + @Override + protected Compiler getLastCompiler() { + switch (this.mode) { + case BOTH: + throw new AssertionError("getLastCompiler does not work correctly in BOTH mode."); + default: + return super.getLastCompiler(); + } + } + // Helpers to test separate warnings/errors with OTI and NTI. protected static OtiNtiDiagnostic warningOtiNti(DiagnosticType oti, DiagnosticType nti) {