Skip to content

Commit

Permalink
remove function wrappers for member type declarations
Browse files Browse the repository at this point in the history
To declare the types of class members, we want to emit e.g.
  /** @type {Foo} */
  MyClass.prototype.foo;

We cannot declare these in the class constructor because sometimes
we don't have a constructor and it's difficult to create our own
(we'd need to match the superclass in that case).

We sometimes cannot declare these at the top level because it's possible
there's a supertype that throws on get, which means the code cannot be
loaded as written without compiling it first.  (We know this actually
can happen because we tried it.)

In the past we declared these in a helper method or bare function,
but it turns out declarations in those are ignored by Closure in
unpredictable circumstances.

So our final approach is:
- for interfaces, declare them at the top level
- for classes, wrap them in an 'if (false)' block.
To do so we must suppress the 'useless code' compiler check, because
putting code in an 'if (false)' is pretty suspicious looking otherwise.
  • Loading branch information
evmar committed Jun 27, 2018
1 parent d73e7fd commit 249273f
Show file tree
Hide file tree
Showing 140 changed files with 274 additions and 351 deletions.
3 changes: 3 additions & 0 deletions src/fileoverview_comment_transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ function augmentFileoverviewComments(tags: jsdoc.Tag[]) {
// 2) Suppress extraRequire. We remove extra requires at the TypeScript level, so any require
// that gets to the JS level is a load-bearing require.
suppressions.add('extraRequire');
// 3) Suppress uselessCode. We emit an "if (false)" around type declarations,
// which is flagged as unused code unless we suppress it.
suppressions.add('uselessCode');
suppressTag.type = Array.from(suppressions.values()).sort().join(',');

return tags;
Expand Down
17 changes: 7 additions & 10 deletions src/tsickle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1288,7 +1288,7 @@ class Annotator extends ClosureRewriter {
if (docTags.length > 0) this.emit(jsdoc.toString(docTags));
// this.writeNode(classDecl, true /*skipComments*/);
this.writeNodeFrom(classDecl, classDecl.getStart(), classDecl.getEnd());
this.emitTypeAnnotationsHelper(classDecl);
this.emitMemberTypes(classDecl);
return true;
}

Expand All @@ -1312,23 +1312,20 @@ class Annotator extends ClosureRewriter {
const name = getIdentifierText(iface.name);
this.emit(`function ${name}() {}\n`);

this.emit(`\n\nfunction ${name}_tsickle_Closure_declarations() {\n`);
const memberNamespace = [name, 'prototype'];
for (const elem of iface.members) {
const isOptional = elem.questionToken != null;
this.visitProperty(memberNamespace, elem, isOptional);
}
this.emit(`}\n`);
}

/**
* emitTypeAnnotationsHelper produces a _tsickle_typeAnnotationsHelper() where
* none existed in the original source. It's necessary in the case where
* TypeScript syntax specifies there are additional properties on the class,
* because to declare these in Closure you must declare these in a method
* somewhere.
* emitMemberTypes emits the type annotations for members of a class.
* It's necessary in the case where TypeScript syntax specifies
* there are additional properties on the class, because to declare
* these in Closure you must declare these separately from the class.
*/
private emitTypeAnnotationsHelper(classDecl: ts.ClassDeclaration) {
private emitMemberTypes(classDecl: ts.ClassDeclaration) {
// Gather parameter properties from the constructor, if it exists.
const ctors: ts.ConstructorDeclaration[] = [];
let paramProps: ts.ParameterDeclaration[] = [];
Expand Down Expand Up @@ -1372,7 +1369,7 @@ class Annotator extends ClosureRewriter {
const className = getIdentifierText(classDecl.name);

// See test_files/fields/fields.ts:BaseThatThrows for a note on this wrapper.
this.emit(`\n\nfunction ${className}_tsickle_Closure_declarations() {\n`);
this.emit(`\n\nif (false) {`);
staticProps.forEach(p => this.visitProperty([className], p));
const memberNamespace = [className, 'prototype'];
nonStaticProps.forEach((p) => this.visitProperty(memberNamespace, p));
Expand Down
4 changes: 2 additions & 2 deletions test_files/abstract/abstract.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.abstract.abstract');
var module = module || { id: 'test_files/abstract/abstract.ts' };
Expand All @@ -19,7 +19,7 @@ class Base {
this.hasReturnType();
}
}
function Base_tsickle_Closure_declarations() {
if (false) {
/**
* @abstract
* @return {void}
Expand Down
2 changes: 1 addition & 1 deletion test_files/arrow_fn.es5/arrow_fn_es5.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview Reproduces an error that caused incorrect Automatic Semicolon Insertion.
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.arrow_fn.es5.arrow_fn_es5');
var module = module || { id: 'test_files/arrow_fn.es5/arrow_fn_es5.ts' };
Expand Down
2 changes: 1 addition & 1 deletion test_files/arrow_fn.untyped/arrow_fn.untyped.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.arrow_fn.untyped.arrow_fn.untyped');
var module = module || { id: 'test_files/arrow_fn.untyped/arrow_fn.untyped.ts' };
Expand Down
2 changes: 1 addition & 1 deletion test_files/arrow_fn/arrow_fn.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.arrow_fn.arrow_fn');
var module = module || { id: 'test_files/arrow_fn/arrow_fn.ts' };
Expand Down
4 changes: 2 additions & 2 deletions test_files/basic.untyped/basic.untyped.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.basic.untyped.basic.untyped');
var module = module || { id: 'test_files/basic.untyped/basic.untyped.ts' };
Expand All @@ -20,7 +20,7 @@ class Foo {
this.field = 'hello';
}
}
function Foo_tsickle_Closure_declarations() {
if (false) {
/** @type {?} */
Foo.prototype.field;
/** @type {?} */
Expand Down
12 changes: 5 additions & 7 deletions test_files/class.untyped/class.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.class.untyped.class');
var module = module || { id: 'test_files/class.untyped/class.ts' };
/**
* @record
*/
function Interface() { }
function Interface_tsickle_Closure_declarations() {
/** @type {?} */
Interface.prototype.interfaceFunc;
}
/** @type {?} */
Interface.prototype.interfaceFunc;
class Super {
/**
* @return {?}
Expand Down Expand Up @@ -62,15 +60,15 @@ superVar = new ImplementsTypeAlias();
function Zone() { }
class ZoneImplementsInterface {
}
function ZoneImplementsInterface_tsickle_Closure_declarations() {
if (false) {
/** @type {?} */
ZoneImplementsInterface.prototype.zone;
}
/** @typedef {?} */
var ZoneAlias;
class ZoneImplementsAlias {
}
function ZoneImplementsAlias_tsickle_Closure_declarations() {
if (false) {
/** @type {?} */
ZoneImplementsAlias.prototype.zone;
}
26 changes: 10 additions & 16 deletions test_files/class/class.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// test_files/class/class.ts(129,1): warning TS0: type/symbol conflict for Zone, using {?} for now
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
// This test exercises the various ways classes and interfaces can interact.
// There are three types of classy things:
Expand All @@ -17,10 +17,8 @@ var module = module || { id: 'test_files/class/class.ts' };
* @record
*/
function Interface() { }
function Interface_tsickle_Closure_declarations() {
/** @type {function(): void} */
Interface.prototype.interfaceFunc;
}
/** @type {function(): void} */
Interface.prototype.interfaceFunc;
class Class {
/**
* @return {void}
Expand All @@ -36,7 +34,7 @@ class AbstractClass {
*/
nonAbstractFunc() { }
}
function AbstractClass_tsickle_Closure_declarations() {
if (false) {
/**
* @abstract
* @return {void}
Expand All @@ -48,10 +46,8 @@ function AbstractClass_tsickle_Closure_declarations() {
* @extends {Interface}
*/
function InterfaceExtendsInterface() { }
function InterfaceExtendsInterface_tsickle_Closure_declarations() {
/** @type {function(): void} */
InterfaceExtendsInterface.prototype.interfaceFunc2;
}
/** @type {function(): void} */
InterfaceExtendsInterface.prototype.interfaceFunc2;
/** @type {!InterfaceExtendsInterface} */
let interfaceExtendsInterface = {
/**
Expand All @@ -67,10 +63,8 @@ let interfaceExtendsInterface = {
* @record
*/
function InterfaceExtendsClass() { }
function InterfaceExtendsClass_tsickle_Closure_declarations() {
/** @type {function(): void} */
InterfaceExtendsClass.prototype.interfaceFunc3;
}
/** @type {function(): void} */
InterfaceExtendsClass.prototype.interfaceFunc3;
/**
* @implements {Interface}
*/
Expand Down Expand Up @@ -198,15 +192,15 @@ abstractClassVar = new ClassExtendsAbstractClass();
function Zone() { }
class ZoneImplementsInterface {
}
function ZoneImplementsInterface_tsickle_Closure_declarations() {
if (false) {
/** @type {string} */
ZoneImplementsInterface.prototype.zone;
}
/** @typedef {?} */
var ZoneAlias;
class ZoneImplementsAlias {
}
function ZoneImplementsAlias_tsickle_Closure_declarations() {
if (false) {
/** @type {string} */
ZoneImplementsAlias.prototype.zone;
}
2 changes: 1 addition & 1 deletion test_files/clutz.no_externs/strip_clutz_type.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.clutz.no_externs.strip_clutz_type');
var module = module || { id: 'test_files/clutz.no_externs/strip_clutz_type.ts' };
Expand Down
2 changes: 1 addition & 1 deletion test_files/coerce/coerce.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.coerce.coerce');
var module = module || { id: 'test_files/coerce/coerce.ts' };
Expand Down
4 changes: 2 additions & 2 deletions test_files/comments/comments.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.comments.comments');
var module = module || { id: 'test_files/comments/comments.ts' };
class Comments {
}
function Comments_tsickle_Closure_declarations() {
if (false) {
/**
* @export
* @type {string}
Expand Down
4 changes: 2 additions & 2 deletions test_files/ctors/ctors.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.ctors.ctors');
var module = module || { id: 'test_files/ctors/ctors.ts' };
Expand All @@ -14,7 +14,7 @@ class X {
this.a = a;
}
}
function X_tsickle_Closure_declarations() {
if (false) {
/** @type {number} */
X.prototype.a;
}
Expand Down
2 changes: 1 addition & 1 deletion test_files/declare/user.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.declare.user');
var module = module || { id: 'test_files/declare/user.ts' };
Expand Down
2 changes: 1 addition & 1 deletion test_files/declare_class_ns/declare_class_ns.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.declare_class_ns.declare_class_ns');
var module = module || { id: 'test_files/declare_class_ns/declare_class_ns.ts' };
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.declare_class_overloads.declare_class_overloads');
var module = module || { id: 'test_files/declare_class_overloads/declare_class_overloads.ts' };
2 changes: 1 addition & 1 deletion test_files/declare_export.untyped/declare_export.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.declare_export.untyped.declare_export');
var module = module || { id: 'test_files/declare_export.untyped/declare_export.ts' };
Expand Down
2 changes: 1 addition & 1 deletion test_files/declare_export/declare_export.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
// All of the types/values declared in this file should
// 1) generate externs
Expand Down
6 changes: 3 additions & 3 deletions test_files/decorator/decorator.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.decorator.decorator');
var module = module || { id: 'test_files/decorator/decorator.ts' };
Expand Down Expand Up @@ -119,7 +119,7 @@ tslib_1.__decorate([
decorator,
tslib_1.__metadata("design:type", external_1.AClass)
], DecoratorTest.prototype, "z", void 0);
function DecoratorTest_tsickle_Closure_declarations() {
if (false) {
/**
* Some comment
* @type {number}
Expand All @@ -135,7 +135,7 @@ let DecoratedClass = class DecoratedClass {
DecoratedClass = tslib_1.__decorate([
classDecorator
], DecoratedClass);
function DecoratedClass_tsickle_Closure_declarations() {
if (false) {
/** @type {string} */
DecoratedClass.prototype.z;
}
4 changes: 2 additions & 2 deletions test_files/decorator/default_export.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/**
* @fileoverview Tests using a default imported class for in a decorated ctor.
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.decorator.default_export');
var module = module || { id: 'test_files/decorator/default_export.ts' };
class DefaultExport {
}
exports.default = DefaultExport;
function DefaultExport_tsickle_Closure_declarations() {
if (false) {
/** @type {string} */
DefaultExport.prototype.field;
}
4 changes: 1 addition & 3 deletions test_files/decorator/external.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.decorator.external');
var module = module || { id: 'test_files/decorator/external.ts' };
Expand All @@ -21,5 +21,3 @@ exports.AClassWithGenerics = AClassWithGenerics;
*/
function AType() { }
exports.AType = AType;
function AType_tsickle_Closure_declarations() {
}
2 changes: 1 addition & 1 deletion test_files/decorator/external2.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @fileoverview added by tsickle
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.decorator.external2');
var module = module || { id: 'test_files/decorator/external2.ts' };
Expand Down
8 changes: 3 additions & 5 deletions test_files/decorator/only_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* @fileoverview only_types only exports types, so TypeScript will elide the
* import entirely.
*
* @suppress {checkTypes,extraRequire} checked by tsc
* @suppress {checkTypes,extraRequire,uselessCode} checked by tsc
*/
goog.module('test_files.decorator.only_types');
var module = module || { id: 'test_files/decorator/only_types.ts' };
Expand All @@ -12,7 +12,5 @@ var module = module || { id: 'test_files/decorator/only_types.ts' };
*/
function AnotherType() { }
exports.AnotherType = AnotherType;
function AnotherType_tsickle_Closure_declarations() {
/** @type {string} */
AnotherType.prototype.field;
}
/** @type {string} */
AnotherType.prototype.field;

0 comments on commit 249273f

Please sign in to comment.