From 429df8011af890baa6fbf94c3c91cc4a8ab8cad8 Mon Sep 17 00:00:00 2001 From: aravindpg Date: Mon, 13 Jun 2016 12:06:13 -0700 Subject: [PATCH] [NTI] Convert CheckNullableReturn to use TypeI. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=124752748 --- .../javascript/jscomp/NewTypeInference.java | 3 +- .../jscomp/lint/CheckNullableReturn.java | 14 +-- src/com/google/javascript/rhino/Node.java | 3 +- .../javascript/jscomp/CompilerTestCase.java | 2 + .../jscomp/TypeICompilerTestCase.java | 36 +++---- .../jscomp/lint/CheckNullableReturnTest.java | 100 ++++++++++-------- 6 files changed, 88 insertions(+), 70 deletions(-) diff --git a/src/com/google/javascript/jscomp/NewTypeInference.java b/src/com/google/javascript/jscomp/NewTypeInference.java index f3c18c59c68..4752862ad6d 100644 --- a/src/com/google/javascript/jscomp/NewTypeInference.java +++ b/src/com/google/javascript/jscomp/NewTypeInference.java @@ -3740,7 +3740,8 @@ private JSType markAndGetTypeOfPreanalyzedNode(Node qnameNode, TypeEnv env, bool private void maybeSetTypeI(Node n, JSType t) { TypeI ti = n.getTypeI(); - JSType oldType = (ti instanceof JSType) ? (JSType) ti : null; + Preconditions.checkState(ti == null || ti instanceof JSType); + JSType oldType = (JSType) ti; // When creating a function summary, we set a precise type on the function's // name node. Since we're visiting inner scopes first, the name node is // revisited after the function's scope is analyzed, and its type then can diff --git a/src/com/google/javascript/jscomp/lint/CheckNullableReturn.java b/src/com/google/javascript/jscomp/lint/CheckNullableReturn.java index d716bf6525b..413fe333876 100644 --- a/src/com/google/javascript/jscomp/lint/CheckNullableReturn.java +++ b/src/com/google/javascript/jscomp/lint/CheckNullableReturn.java @@ -25,11 +25,11 @@ import com.google.javascript.jscomp.NodeTraversal; import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge; +import com.google.javascript.rhino.FunctionTypeI; import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; -import com.google.javascript.rhino.jstype.FunctionType; -import com.google.javascript.rhino.jstype.JSType; +import com.google.javascript.rhino.TypeI; /** * Checks when a function is annotated as returning {SomeType} (nullable) @@ -96,7 +96,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { */ private static boolean hasSingleThrow(Node blockNode) { if (blockNode.getChildCount() == 1 - && blockNode.getFirstChild().getType() == Token.THROW) { + && blockNode.getFirstChild().getKind() == Token.THROW) { // Functions consisting of a single "throw FOO" can be actually abstract, // so do not check their return type nullability. return true; @@ -113,12 +113,12 @@ private static boolean isReturnTypeNullable(Node n) { if (n == null || !n.isFunction()) { return false; } - FunctionType functionType = n.getJSType().toMaybeFunctionType(); + FunctionTypeI functionType = n.getTypeI().toMaybeFunctionType(); if (functionType == null) { // If the JSDoc declares a non-function type on a function node, we still shouldn't crash. return false; } - JSType returnType = functionType.getReturnType(); + TypeI returnType = functionType.getReturnType(); if (returnType == null || returnType.isUnknownType() || !returnType.isNullable()) { return false; @@ -140,14 +140,14 @@ public static boolean canReturnNull(ControlFlowGraph graph) { /** * @return True if the node represents a nullable value. Essentially, this - * is just n.getJSType().isNullable(), but for purposes of this pass, + * is just n.getTypeI().isNullable(), but for purposes of this pass, * the expression {@code x || null} is considered nullable even if * x is always truthy. This often happens with expressions like * {@code arr[i] || null}: The compiler doesn't know that arr[i] can * be undefined. */ private static boolean isNullable(Node n) { - return n.getJSType().isNullable() + return n.getTypeI().isNullable() || (n.isOr() && n.getLastChild().isNull()); } diff --git a/src/com/google/javascript/rhino/Node.java b/src/com/google/javascript/rhino/Node.java index d752633e2e3..ee0e3e21757 100644 --- a/src/com/google/javascript/rhino/Node.java +++ b/src/com/google/javascript/rhino/Node.java @@ -2161,8 +2161,7 @@ public void setJSType(JSType jsType) { } public TypeI getTypeI() { - // For the time being, we only want to return the type iff it's an old type. - return getJSType(); + return typei; } public void setTypeI(TypeI type) { diff --git a/test/com/google/javascript/jscomp/CompilerTestCase.java b/test/com/google/javascript/jscomp/CompilerTestCase.java index f214d4b4da8..751a7e8fa7b 100644 --- a/test/com/google/javascript/jscomp/CompilerTestCase.java +++ b/test/com/google/javascript/jscomp/CompilerTestCase.java @@ -1194,6 +1194,8 @@ private void test( RecentChange recentChange = new RecentChange(); compiler.addChangeHandler(recentChange); + compiler.getOptions().setNewTypeInference(newTypeInferenceEnabled); + Node root = compiler.parseInputs(); String errorMsg = LINE_JOINER.join(compiler.getErrors()); diff --git a/test/com/google/javascript/jscomp/TypeICompilerTestCase.java b/test/com/google/javascript/jscomp/TypeICompilerTestCase.java index 242f392a057..f1d06ad059f 100644 --- a/test/com/google/javascript/jscomp/TypeICompilerTestCase.java +++ b/test/com/google/javascript/jscomp/TypeICompilerTestCase.java @@ -16,6 +16,10 @@ package com.google.javascript.jscomp; +import com.google.common.collect.ImmutableList; + +import java.util.List; + /** * CompilerTestCase for passes that run after type checking and use type information. * Allows us to test those passes with both type checkers. @@ -23,34 +27,30 @@ * @author dimvar@google.com (Dimitris Vardoulakis) */ public abstract class TypeICompilerTestCase extends CompilerTestCase { - @Override - public void testSame(String js) { - enableTypeCheck(); - super.testSame(js); - disableTypeCheck(); - enableNewTypeInference(); - super.testSame(js); - disableNewTypeInference(); - } @Override - public void testSame(String js, DiagnosticType warning) { + public void test( + List externs, + String js, + String expected, + DiagnosticType error, + DiagnosticType warning, + String description) { enableTypeCheck(); - super.testSame(js, warning); + super.test(externs, js, expected, error, warning, description); disableTypeCheck(); enableNewTypeInference(); - super.testSame(js, warning); + super.test(externs, js, expected, error, warning, description); disableNewTypeInference(); } - @Override - public void testSame(String externs, String js, DiagnosticType warning) { + public void testSameOtiOnly(String externs, String js, DiagnosticType warning) { enableTypeCheck(); - super.testSame(externs, js, warning); + SourceFile externsFile = SourceFile.fromCode("externs", externs); + externsFile.setIsExtern(true); + List externsInputs = ImmutableList.of(externsFile); + super.test(externsInputs, js, null, null, warning, null); disableTypeCheck(); - enableNewTypeInference(); - super.testSame(externs, js, warning); - disableNewTypeInference(); } } diff --git a/test/com/google/javascript/jscomp/lint/CheckNullableReturnTest.java b/test/com/google/javascript/jscomp/lint/CheckNullableReturnTest.java index b51a8359cbb..8e174d5dd65 100644 --- a/test/com/google/javascript/jscomp/lint/CheckNullableReturnTest.java +++ b/test/com/google/javascript/jscomp/lint/CheckNullableReturnTest.java @@ -21,22 +21,15 @@ import com.google.javascript.jscomp.CompilerOptions; import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.CompilerPass; -import com.google.javascript.jscomp.CompilerTestCase; import com.google.javascript.jscomp.DiagnosticGroups; - -import java.io.IOException; +import com.google.javascript.jscomp.TypeICompilerTestCase; /** * Test case for {@link CheckNullableReturn}. * */ -public final class CheckNullableReturnTest extends CompilerTestCase { - private static String externs = "/** @constructor */ function SomeType() {}"; - - @Override - public void setUp() throws IOException { - enableTypeCheck(); - } +public final class CheckNullableReturnTest extends TypeICompilerTestCase { + private static String externs = DEFAULT_EXTERNS + "/** @constructor */ function SomeType() {}"; @Override protected CompilerPass getProcessor(Compiler compiler) { @@ -56,11 +49,11 @@ protected int getNumRepetitions() { } public void testSimpleWarning() { - testError("" - + "/** @return {SomeType} */\n" - + "function f() {\n" - + " return new SomeType();\n" - + "}"); + testError(LINE_JOINER.join( + "/** @return {SomeType} */", + "function f() {", + " return new SomeType();", + "}")); } public void testNullableReturn() { @@ -68,13 +61,13 @@ public void testNullableReturn() { testBodyOk("if (a) { return null; } return {};"); testBodyOk("switch(1) { case 12: return null; } return {};"); testBodyOk( - "/** @return {number} */ function f() { var x; }; return null;"); + "/** @return {number} */ function f() { return 42; }; return null;"); } public void testNotNullableReturn() { // Empty function body. Ignore this case. The remainder of the functions in // this test have non-empty bodies. - testBodyOk(""); + testBodyOkOti(""); // Simple case. testBodyError("return {};"); @@ -91,11 +84,11 @@ public void testNotNullableReturn() { } public void testFinallyStatements() { - testBodyOk("try { return null; } finally { }"); + testBodyOk("try { return null; } finally { return {}; }"); testBodyOk("try { } finally { return null; }"); testBodyOk("try { return {}; } finally { return null; }"); testBodyOk("try { return null; } finally { return {}; }"); - testBodyError("try { } catch (e) { return null; } finally { return {}; }"); + testBodyErrorOti("try { } catch (e) { return null; } finally { return {}; }"); } public void testKnownConditions() { @@ -105,13 +98,12 @@ public void testKnownConditions() { testBodyOk("if (false) return {}; return null;"); testBodyOk("if (false) return null; else return {};"); - testBodyError("if (1) return {}"); + testBodyError("if (1) return {}; return {x: 42};"); testBodyOk("if (1) { return null; } else { return {}; }"); testBodyOk("if (0) return {}; return null;"); testBodyOk("if (0) { return null; } else { return {}; }"); - testBodyError("if (3) return {}"); testBodyOk("if (3) return null; else return {};"); } @@ -121,7 +113,7 @@ public void testKnownWhileLoop() { testBodyError("while (0) {} return {}"); // Not known. - testBodyError("while(x) { return {}; }"); + testBodyErrorOti("while(x) { return {}; }"); } public void testTwoBranches() { @@ -183,37 +175,37 @@ public void testTryCatch() { "try {", " return bar();", "} catch (e) {", - "} finally { }")); + "} finally { return baz(); }")); } public void testNoExplicitReturn() { - testError("" - + "/** @return {SomeType} */\n" - + "function f() {\n" - + " if (foo) {\n" - + " return new SomeType();\n" - + " }\n" - + "}"); + testErrorOti(LINE_JOINER.join( + "/** @return {SomeType} */", + "function f() {", + " if (foo) {", + " return new SomeType();", + " }", + "}")); } public void testNoWarningIfCanReturnNull() { - testOk("" - + "/** @return {SomeType} */\n" - + "function f() {\n" - + " if (foo) {\n" - + " return new SomeType();\n" - + " } else {\n" - + " return null;\n" - + " }\n" - + "}"); + testOk(LINE_JOINER.join( + "/** @return {SomeType} */", + "function f() {", + " if (foo) {", + " return new SomeType();", + " } else {", + " return null;", + " }", + "}")); } public void testNoWarningOnEmptyFunction() { - testOk(LINE_JOINER.join( + testOkOti(LINE_JOINER.join( "/** @return {SomeType} */", "function f() {}")); setAcceptedLanguage(LanguageMode.ECMASCRIPT6); - testOk(LINE_JOINER.join( + testOkOti(LINE_JOINER.join( "var obj = {", " /** @return {SomeType} */\n", " f() {}", @@ -244,7 +236,11 @@ public void testNoWarningOnXOrNull() { public void testNonfunctionTypeDoesntCrash() { enableClosurePass(); - test("goog.forwardDeclare('FunType'); /** @type {!FunType} */ (function() { return; })", null); + test(DEFAULT_EXTERNS, + "goog.forwardDeclare('FunType'); /** @type {!FunType} */ (function() { return; })", + (String) null, + null, + null); } private static String createFunction(String body) { @@ -259,19 +255,39 @@ private void testOk(String js) { testSame(externs, js, null); } + private void testOkOti(String js) { + testSameOtiOnly(externs, js, null); + } + private void testError(String js) { testSame(externs, js, CheckNullableReturn.NULLABLE_RETURN_WITH_NAME); } + private void testErrorOti(String js) { + testSameOtiOnly(externs, js, CheckNullableReturn.NULLABLE_RETURN_WITH_NAME); + } + private void testBodyOk(String body) { testOk(createFunction(body)); setAcceptedLanguage(LanguageMode.ECMASCRIPT6); testOk(createShorthandFunctionInObjLit(body)); } + private void testBodyOkOti(String body) { + testOkOti(createFunction(body)); + setAcceptedLanguage(LanguageMode.ECMASCRIPT6); + testOkOti(createShorthandFunctionInObjLit(body)); + } + private void testBodyError(String body) { testError(createFunction(body)); setAcceptedLanguage(LanguageMode.ECMASCRIPT6); testError(createShorthandFunctionInObjLit(body)); } + + private void testBodyErrorOti(String body) { + testErrorOti(createFunction(body)); + setAcceptedLanguage(LanguageMode.ECMASCRIPT6); + testErrorOti(createShorthandFunctionInObjLit(body)); + } }