Skip to content

Commit

Permalink
Rollforward: Resolve CAST types immediately after evaluation
Browse files Browse the repository at this point in the history
Currently they are resolved after TypeInference runs on all scopes, in TypeInferencePass.

This is a problem when other code in the TypeInference pass tries to read properties off those types. It causes unknown type warnings when trying to delete InlineTypeAliases.

NEW: loosen Preconditions check to ignore info.hasType() and add some repro test cases

*** Reason for rollback ***

Roll forward with looser Preconditions check

*** Reason for original rollback ***

Caused crash due to a CAST node missing @type JSDocInfo

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=233692996
  • Loading branch information
lauraharker authored and EatingW committed Feb 13, 2019
1 parent 815301d commit 407b171
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 10 deletions.
16 changes: 14 additions & 2 deletions src/com/google/javascript/jscomp/TypeInference.java
Expand Up @@ -668,8 +668,20 @@ private FlowScope traverse(Node n, FlowScope scope) {
case CAST: case CAST:
scope = traverseChildren(n, scope); scope = traverseChildren(n, scope);
JSDocInfo info = n.getJSDocInfo(); JSDocInfo info = n.getJSDocInfo();
if (info != null && info.hasType()) { // TODO(b/123955687): also check that info.hasType() is true
n.setJSType(info.getType().evaluate(scope.getDeclarationScope(), registry)); checkNotNull(info, "CAST node should always have JSDocInfo");
if (info.hasType()) {
// NOTE(lharker) - I tried moving CAST type evaluation into the typed scope creation
// phase.
// Since it caused a few new, seemingly spurious, 'Bad type annotation' and
// 'unknown property type' warnings, and having it in TypeInference seems to work, we just
// do the lookup + resolution here.
n.setJSType(
info.getType()
.evaluate(scope.getDeclarationScope(), registry)
.resolve(registry.getErrorReporter()));
} else {
n.setJSType(unknownType);
} }
break; break;


Expand Down
14 changes: 8 additions & 6 deletions test/com/google/javascript/jscomp/IntegrationTest.java
Expand Up @@ -331,13 +331,15 @@ public void testObjDestructuringConst() {
public void testConstructorCycle() { public void testConstructorCycle() {
CompilerOptions options = createCompilerOptions(); CompilerOptions options = createCompilerOptions();
options.setCheckTypes(true); options.setCheckTypes(true);
test( testNoWarnings(
options, options,
"/** @return {function()} */ var AsyncTestCase = function() {};\n" lines(
+ "/**\n" "/** @return {function()} */",
+ " * @constructor\n" "var AsyncTestCase = function() {};",
+ " */ Foo = /** @type {function(new:Foo)} */ (AyncTestCase());", "/**",
RhinoErrorReporter.PARSE_ERROR); " * @constructor",
" */",
"const Foo = /** @type {function(new:Foo)} */ (AsyncTestCase());"));
} }


// b/27531865 // b/27531865
Expand Down
44 changes: 44 additions & 0 deletions test/com/google/javascript/jscomp/TypeCheckTest.java
Expand Up @@ -11643,6 +11643,34 @@ public void testCast34b() {
"var y = /** @type {Object} */(x);"); "var y = /** @type {Object} */(x);");
} }


@Test
public void testCastToNameRequiringPropertyResolution() {
// regression test for correctly typing properties off of types in CASTs.
// The type JSDoc in a cast is currently evaluated during TypeInference. In the past any
// 'unresolved' types in cast JSDoc were not resolved until after type inference completed. This
// caused type inference to infer properties off of those unresolved types as unknown.
testTypesWithExterns(
"var unknownVar;",
lines(
"const foo = {bar: {}};",
"const bar = foo.bar;",
"bar.Class = class {",
" /** @return {number} */",
" id() { return 0; }",
"};",
// Because `foo.bar.Class = ...` was never directly assigned, the type 'foo.bar.Class'
// is not in the JSTypeRegistry. It's resolved through NamedType#resolveViaProperty.
// The same thing would have occurred if we assigned 'foo.bar.Class = ...' then
// referred to '!bar.Class' in the JSDoc.
// Verify that type inference correctly infers the id property's type.

"const /** null */ n = /** @type {!foo.bar.Class} */ (unknownVar).id;"),
lines(
"initializing variable", //
"found : function(this:bar.Class): number",
"required: null"));
}

@Test @Test
public void testNestedCasts() { public void testNestedCasts() {
testTypes("/** @constructor */var T = function() {};\n" + testTypes("/** @constructor */var T = function() {};\n" +
Expand Down Expand Up @@ -22751,6 +22779,22 @@ public void testCastOnLhsOfAssignBlocksBadAssignmentWarning() {
testTypes("var /** number */ x = 1; (/** @type {?} */ (x)) = 'foobar';"); testTypes("var /** number */ x = 1; (/** @type {?} */ (x)) = 'foobar';");
} }


@Test
public void testCastOnLhsWithAdditionalJSDoc() {
// TODO(b/123955687): this should issue a type error because Foo.prototype.takesString is passed
// a number
testTypes(
lines(
"class Foo {",
" takesString(/** string */ stringParameter) {}",
"}",
"/** @param {*} all */",
"function f(all) {",
" /** some jsdoc */",
" (/** @type {!Foo} */ (all)).takesString(0);",
"}"));
}

@Test @Test
public void testInvalidComparisonsInStrictOperatorsMode() { public void testInvalidComparisonsInStrictOperatorsMode() {
testTypes( testTypes(
Expand Down
15 changes: 13 additions & 2 deletions test/com/google/javascript/jscomp/parsing/ParserTest.java
Expand Up @@ -967,7 +967,7 @@ public void testJSDocAttachment16() {
} }


@Test @Test
public void testJSDocAttachment17() { public void testJSDocAttachmentForCastFnCall() {
Node fn = Node fn =
parse( parse(
"function f() { " + "function f() { " +
Expand All @@ -979,7 +979,7 @@ public void testJSDocAttachment17() {
} }


@Test @Test
public void testJSDocAttachment18() { public void testJSDocAttachmentForCastName() {
Node fn = Node fn =
parse( parse(
"function f() { " + "function f() { " +
Expand All @@ -990,6 +990,17 @@ public void testJSDocAttachment18() {
assertNode(cast).hasType(Token.CAST); assertNode(cast).hasType(Token.CAST);
} }


@Test
public void testJSDocAttachmentForCastLhs() {
Node expr = parse("/** some jsdoc */ (/** @type {?} */ (a)).b = 0;").getOnlyChild();
Node lhs = expr.getFirstFirstChild(); // child is ASSIGN, grandchild is the GETPROP
Node cast = lhs.getFirstChild();
assertNode(cast).hasToken(Token.CAST);
assertThat(cast.getJSDocInfo()).isNotNull();
// TODO(b/123955687): this should be true
assertThat(cast.getJSDocInfo().hasType()).isFalse();
}

@Test @Test
public void testJSDocAttachment19() { public void testJSDocAttachment19() {
Node fn = Node fn =
Expand Down

0 comments on commit 407b171

Please sign in to comment.