From 6ed62f0420f2a6b0b799f5209e203cd521f72b28 Mon Sep 17 00:00:00 2001 From: sdh Date: Tue, 25 Jun 2019 11:59:52 -0700 Subject: [PATCH] Defer applying the `!` operator on NamedType until after it's resolved. Automated g4 rollback of changelist 254149009. *** Reason for rollback *** Roll forward, fixing bugs. *** Original change description *** Automated g4 rollback of changelist 254144841. *** Reason for rollback *** 10 or more targets failed to build ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=255017937 --- .../rhino/jstype/JSTypeRegistry.java | 16 +++++-- .../javascript/rhino/jstype/NamedType.java | 28 +++++++++-- .../javascript/jscomp/CodePrinterTest.java | 3 +- .../javascript/jscomp/TypeCheckTest.java | 46 +++++++++++++++++++ 4 files changed, 83 insertions(+), 10 deletions(-) diff --git a/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java b/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java index b83e7d4566c..c72d0fcebd8 100644 --- a/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java +++ b/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java @@ -1943,9 +1943,19 @@ private JSType createFromTypeNodesInternal( n.getFirstChild(), sourceName, scope); case BANG: // Not nullable - return createFromTypeNodesInternal( - n.getFirstChild(), sourceName, scope, recordUnresolvedTypes) - .restrictByNotNullOrUndefined(); + { + JSType child = + createFromTypeNodesInternal( + n.getFirstChild(), sourceName, scope, recordUnresolvedTypes); + if (child instanceof NamedType) { + JSType type = ((NamedType) child).getBangType(); + if (type instanceof NamedType && recordUnresolvedTypes && type != child) { + unresolvedNamedTypes.add((NamedType) type); + } + return type; + } + return child.restrictByNotNullOrUndefined(); + } case QMARK: // Nullable or unknown Node firstChild = n.getFirstChild(); diff --git a/src/com/google/javascript/rhino/jstype/NamedType.java b/src/com/google/javascript/rhino/jstype/NamedType.java index f8ce1144d93..cfed4c0e629 100644 --- a/src/com/google/javascript/rhino/jstype/NamedType.java +++ b/src/com/google/javascript/rhino/jstype/NamedType.java @@ -97,6 +97,7 @@ static int nominalHashCode(ObjectType type) { private final String sourceName; private final int lineno; private final int charno; + private final boolean nonNull; /** * Validates the type resolution. @@ -135,15 +136,27 @@ static int nominalHashCode(ObjectType type) { int charno, ImmutableList templateTypes) { super(registry, registry.getNativeObjectType(JSTypeNative.UNKNOWN_TYPE)); - checkNotNull(reference); + this.nonNull = reference.startsWith("!"); this.resolutionScope = scope; - this.reference = reference; + this.reference = nonNull ? reference.substring(1) : reference; this.sourceName = sourceName; this.lineno = lineno; this.charno = charno; this.templateTypes = templateTypes; } + /** Returns a new non-null version of this type. */ + JSType getBangType() { + if (nonNull) { + return this; + } else if (resolutionScope == null) { + // Already resolved, just restrict. + return getReferencedType().restrictByNotNullOrUndefined(); + } + return new NamedType( + resolutionScope, registry, "!" + reference, sourceName, lineno, charno, templateTypes); + } + @Override public ImmutableList getTemplateTypes() { return templateTypes; @@ -291,7 +304,9 @@ private void resolveViaProperties(ErrorReporter reporter) { return; } - StaticTypedSlot slot = resolutionScope.getSlot(componentNames[0]); + StaticTypedSlot slot = + checkNotNull(resolutionScope, "resolutionScope") + .getSlot(checkNotNull(componentNames, "componentNames")[0]); if (slot == null) { handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true); return; @@ -348,6 +363,9 @@ private void resolveViaProperties(ErrorReporter reporter) { private void setReferencedAndResolvedType( JSType type, ErrorReporter reporter) { + if (nonNull) { + type = type.restrictByNotNullOrUndefined(); + } if (validator != null) { validator.apply(type); } @@ -405,8 +423,8 @@ private void handleUnresolvedType( } /** - * Check for an obscure but very confusing error condition where a local variable shadows a - * global namespace. + * Check for an obscure but very confusing error condition where a local variable shadows a global + * namespace. */ private boolean localVariableShadowsGlobalNamespace(String root) { StaticSlot rootVar = resolutionScope.getSlot(root); diff --git a/test/com/google/javascript/jscomp/CodePrinterTest.java b/test/com/google/javascript/jscomp/CodePrinterTest.java index b1c87a8f5c4..b00781ab085 100644 --- a/test/com/google/javascript/jscomp/CodePrinterTest.java +++ b/test/com/google/javascript/jscomp/CodePrinterTest.java @@ -1305,7 +1305,6 @@ public void testTypeAnnotationsTypeDef() { // TODO(johnlenz): It would be nice if there were some way to preserve // typedefs but currently they are resolved into the basic types in the // type registry. - // NOTE(sdh): The type inferrence does not correctly remove null assertTypeAnnotations( LINE_JOINER.join( "/** @const */ var goog = {};", @@ -1318,7 +1317,7 @@ public void testTypeAnnotationsTypeDef() { "/** @const */ goog.java = {};", "goog.java.Long;", "/**", - " * @param {(Array|null)} a", + " * @param {!Array} a", " * @return {undefined}", " */", "function f(a) {\n}\n")); diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index e1589c0c55a..5d0930796b4 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -24016,6 +24016,52 @@ public void testParameterShadowsNamespace() { + "the intended global namespace.")); } + @Test + public void testBangOperatorOnForwardReferencedType() { + testTypes( + lines( + "/** @typedef {?number} */", + "var Foo;", + "/** @return {!Foo} */", + "function f() {}", + "/** @const {!Foo} */", + "var x = f();")); + } + + @Test + public void testBangOperatorOnForwardReferencedType_mismatch() { + testTypes( + lines( + "/** @type {!Foo} */", + "var x;", + "/** @typedef {?number} */", + "var Foo;", + "/** @type {?Foo} */", + "var y;", + "x = y;"), + lines("assignment", "found : (null|number)", "required: number")); + } + + @Test + public void testBangOperatorOnTypedefForShadowedNamespace() { + // NOTE: This is a pattern used to work around the fact that @ngInjected constructors very + // frequently (and unavoidably) shadow global namespaces (i.e. angular.modules) with constructor + // parameters. + testTypes( + lines( + "/** @typedef {!service.Service} */", + "var serviceService;", + "/** @constructor @param {!service.Service} service */", + "var Controller = function(service) {", + " /** @private {!serviceService} */", + " this.service = service;", + "};", + "/** @const */", + "var service = {};", + "/** @constructor */", + "service.Service = function() {};")); + } + private void testClosureTypes(String js, String description) { testClosureTypesMultipleWarnings(js, description == null ? null : ImmutableList.of(description));