Skip to content

Commit

Permalink
[NTI] Convert CheckNullableReturn to use TypeI.
Browse files Browse the repository at this point in the history
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124752748
  • Loading branch information
aravind-pg authored and blickly committed Jun 13, 2016
1 parent c91077e commit 429df80
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 70 deletions.
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/NewTypeInference.java
Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions src/com/google/javascript/jscomp/lint/CheckNullableReturn.java
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -140,14 +140,14 @@ public static boolean canReturnNull(ControlFlowGraph<Node> 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());
}

Expand Down
3 changes: 1 addition & 2 deletions src/com/google/javascript/rhino/Node.java
Expand Up @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions test/com/google/javascript/jscomp/CompilerTestCase.java
Expand Up @@ -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());
Expand Down
36 changes: 18 additions & 18 deletions test/com/google/javascript/jscomp/TypeICompilerTestCase.java
Expand Up @@ -16,41 +16,41 @@

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.
*
* @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<SourceFile> 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<SourceFile> externsInputs = ImmutableList.of(externsFile);
super.test(externsInputs, js, null, null, warning, null);
disableTypeCheck();
enableNewTypeInference();
super.testSame(externs, js, warning);
disableNewTypeInference();
}
}

100 changes: 58 additions & 42 deletions test/com/google/javascript/jscomp/lint/CheckNullableReturnTest.java
Expand Up @@ -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) {
Expand All @@ -56,25 +49,25 @@ 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() {
testBodyOk("return null;");
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 {};");
Expand All @@ -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() {
Expand All @@ -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 {};");
}

Expand All @@ -121,7 +113,7 @@ public void testKnownWhileLoop() {
testBodyError("while (0) {} return {}");

// Not known.
testBodyError("while(x) { return {}; }");
testBodyErrorOti("while(x) { return {}; }");
}

public void testTwoBranches() {
Expand Down Expand Up @@ -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() {}",
Expand Down Expand Up @@ -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) {
Expand All @@ -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));
}
}

0 comments on commit 429df80

Please sign in to comment.