diff --git a/src/tsickle.ts b/src/tsickle.ts index e8c48527a..acdc1ffaa 100644 --- a/src/tsickle.ts +++ b/src/tsickle.ts @@ -488,12 +488,15 @@ class Annotator extends ClosureRewriter { /** Exported symbol names that have been generated by expanding an "export * from ...". */ private generatedExports = new Set(); + private typeChecker: ts.TypeChecker; + constructor( program: ts.Program, file: ts.SourceFile, options: Options, private pathToModuleName: (context: string, importPath: string) => string, private host?: ts.ModuleResolutionHost, private tsOpts?: ts.CompilerOptions) { super(program, file, options); this.externsWriter = new ExternsWriter(program, file, options); + this.typeChecker = program.getTypeChecker(); } annotate(): Output { @@ -552,7 +555,7 @@ class Annotator extends ClosureRewriter { } const declNames = this.getExportDeclarationNames(node); for (let decl of declNames) { - const sym = this.program.getTypeChecker().getSymbolAtLocation(decl); + const sym = this.typeChecker.getSymbolAtLocation(decl); const isValue = sym.flags & ts.SymbolFlags.Value; const declName = getIdentifierText(decl); if (node.kind === ts.SyntaxKind.VariableStatement) { @@ -599,7 +602,6 @@ class Annotator extends ClosureRewriter { this.writeRange(node.getFullStart(), node.getStart()); this.emit('export'); let exportedSymbols: NamedSymbol[] = []; - const typeChecker = this.program.getTypeChecker(); if (!exportDecl.exportClause && exportDecl.moduleSpecifier) { // It's an "export * from ..." statement. // Rewrite it to re-export each exported symbol directly. @@ -607,7 +609,7 @@ class Annotator extends ClosureRewriter { this.emit(` {${exportedSymbols.map(e => unescapeName(e.name)).join(',')}}`); } else { if (exportDecl.exportClause) { - exportedSymbols = this.getNamedSymbols(exportDecl.exportClause.elements, typeChecker); + exportedSymbols = this.getNamedSymbols(exportDecl.exportClause.elements); this.visit(exportDecl.exportClause); } } @@ -677,8 +679,7 @@ class Annotator extends ClosureRewriter { case ts.SyntaxKind.GetAccessor: case ts.SyntaxKind.SetAccessor: let fnDecl = node; - let tags = - hasExportingDecorator(node, this.program.getTypeChecker()) ? [{tagName: 'export'}] : []; + let tags = hasExportingDecorator(node, this.typeChecker) ? [{tagName: 'export'}] : []; if (!fnDecl.body) { if (hasModifierFlag(fnDecl, ts.ModifierFlags.Abstract)) { @@ -732,7 +733,7 @@ class Annotator extends ClosureRewriter { return true; case ts.SyntaxKind.NonNullExpression: const nnexpr = node as ts.NonNullExpression; - let type = this.program.getTypeChecker().getTypeAtLocation(nnexpr.expression); + let type = this.typeChecker.getTypeAtLocation(nnexpr.expression); if (type.flags & ts.TypeFlags.Union) { const nonNullUnion = (type as ts.UnionType) @@ -751,7 +752,7 @@ class Annotator extends ClosureRewriter { case ts.SyntaxKind.PropertyDeclaration: case ts.SyntaxKind.VariableStatement: let docTags = this.getJSDoc(node) || []; - if (hasExportingDecorator(node, this.program.getTypeChecker())) { + if (hasExportingDecorator(node, this.typeChecker)) { docTags.push({tagName: 'export'}); } @@ -762,6 +763,49 @@ class Annotator extends ClosureRewriter { return true; } break; + case ts.SyntaxKind.ElementAccessExpression: + // Convert quoted accesses to properties that have a symbol to dotted accesses, to get + // consistent Closure renaming. This can happen because TS allows quoted access with literal + // strings to properties. + const eae = node as ts.ElementAccessExpression; + if (!eae.argumentExpression || + eae.argumentExpression.kind !== ts.SyntaxKind.StringLiteral) { + return false; + } + const quotedPropSym = this.typeChecker.getSymbolAtLocation(eae.argumentExpression); + // If it has a symbol, it's actually a regular declared property. + if (!quotedPropSym) return false; + const propName = (eae.argumentExpression as ts.StringLiteral).text; + // Properties containing non-JS identifier names must not be unquoted. + if (!isValidClosurePropertyName(propName)) return false; + this.writeNode(eae.expression); + this.emit(`.${propName}`); + return true; + case ts.SyntaxKind.PropertyAccessExpression: + // Convert dotted accesses to types that have an index type declared to quoted accesses, to + // avoid Closure renaming one access but not the other. + // This can happen because TS allows dotted access to string index types. + const pae = node as ts.PropertyAccessExpression; + const t = this.typeChecker.getTypeAtLocation(pae.expression); + if (!t.getStringIndexType()) return false; + // Types can have string index signatures and declared properties (of the matching type). + // These properties have a symbol, as opposed to pure string index types. + const propSym = this.typeChecker.getSymbolAtLocation(pae.name); + // The decision to return below is a judgement call. Presumably, in most situations, dotted + // access to a property is correct, and should not be turned into quoted access even if + // there is a string index on the type. However it is possible to construct programs where + // this is incorrect, e.g. where user code assigns into a property through the index access + // in another location. + if (propSym) return false; + + this.debugWarn( + pae, + this.typeChecker.typeToString(t) + + ` has a string index type but is accessed using dotted access. ` + + `Quoting the access.`); + this.writeNode(pae.expression); + this.emit(`['${getIdentifierText(pae.name)}']`); + return true; default: break; } @@ -850,7 +894,6 @@ class Annotator extends ClosureRewriter { private expandSymbolsFromExportStar(exportDecl: ts.ExportDeclaration): NamedSymbol[] { // You can't have an "export *" without a module specifier. const moduleSpecifier = exportDecl.moduleSpecifier!; - let typeChecker = this.program.getTypeChecker(); // Gather the names of local exports, to avoid reexporting any // names that are already locally exported. @@ -860,7 +903,7 @@ class Annotator extends ClosureRewriter { // import {foo} from ... // so the latter is filtered below. let locals = - typeChecker.getSymbolsInScope(this.file, ts.SymbolFlags.Export | ts.SymbolFlags.Alias); + this.typeChecker.getSymbolsInScope(this.file, ts.SymbolFlags.Export | ts.SymbolFlags.Alias); let localSet = new Set(); for (let local of locals) { if (local.declarations && @@ -872,7 +915,8 @@ class Annotator extends ClosureRewriter { // Expand the export list, then filter it to the symbols we want to reexport. - let exports = typeChecker.getExportsOfModule(typeChecker.getSymbolAtLocation(moduleSpecifier)); + let exports = + this.typeChecker.getExportsOfModule(this.typeChecker.getSymbolAtLocation(moduleSpecifier)); const reexports = new Set(); for (let sym of exports) { let name = unescapeName(sym.name); @@ -907,9 +951,9 @@ class Annotator extends ClosureRewriter { */ private emitTypeDefExports(exports: NamedSymbol[]) { if (this.options.untyped) return; - const typeChecker = this.program.getTypeChecker(); for (let exp of exports) { - if (exp.sym.flags & ts.SymbolFlags.Alias) exp.sym = typeChecker.getAliasedSymbol(exp.sym); + if (exp.sym.flags & ts.SymbolFlags.Alias) + exp.sym = this.typeChecker.getAliasedSymbol(exp.sym); const isTypeAlias = (exp.sym.flags & ts.SymbolFlags.TypeAlias) !== 0 && (exp.sym.flags & ts.SymbolFlags.Value) === 0; if (!isTypeAlias) continue; @@ -985,12 +1029,11 @@ class Annotator extends ClosureRewriter { // all symbols from this import to be prefixed. if (!this.options.untyped) { let symbols: NamedSymbol[] = []; - const typeChecker = this.program.getTypeChecker(); if (importClause.name) { // import a from ...; symbols = [{ name: getIdentifierText(importClause.name), - sym: typeChecker.getSymbolAtLocation(importClause.name) + sym: this.typeChecker.getSymbolAtLocation(importClause.name) }]; } else { // import {a as b} from ...; @@ -998,7 +1041,7 @@ class Annotator extends ClosureRewriter { importClause.namedBindings.kind !== ts.SyntaxKind.NamedImports) { throw new Error('unreached'); // Guaranteed by if check above. } - symbols = this.getNamedSymbols(importClause.namedBindings.elements, typeChecker); + symbols = this.getNamedSymbols(importClause.namedBindings.elements); } this.forwardDeclare(decl.moduleSpecifier, symbols, !!importClause.name); } @@ -1016,15 +1059,13 @@ class Annotator extends ClosureRewriter { } } - private getNamedSymbols( - specifiers: Array, - typeChecker: ts.TypeChecker): NamedSymbol[] { + private getNamedSymbols(specifiers: Array): NamedSymbol[] { return specifiers.map(e => { return { // e.name might be renaming symbol as in `export {Foo as Bar}`, where e.name would be 'Bar' // and != sym.name. Store away the name so forwardDeclare below can emit the right name. name: getIdentifierText(e.name), - sym: typeChecker.getSymbolAtLocation(e.name), + sym: this.typeChecker.getSymbolAtLocation(e.name), }; }); } @@ -1043,8 +1084,8 @@ class Annotator extends ClosureRewriter { const forwardDeclarePrefix = `tsickle_forward_declare_${++this.forwardDeclareCounter}`; const moduleNamespace = nsImport !== null ? nsImport : this.pathToModuleName(this.file.fileName, importPath); - const typeChecker = this.program.getTypeChecker(); - const exports = typeChecker.getExportsOfModule(typeChecker.getSymbolAtLocation(specifier)); + const exports = + this.typeChecker.getExportsOfModule(this.typeChecker.getSymbolAtLocation(specifier)); // In TypeScript, importing a module for use in a type annotation does not cause a runtime load. // In Closure Compiler, goog.require'ing a module causes a runtime load, so emitting requires // here would cause a change in load order, which is observable (and can lead to errors). @@ -1066,7 +1107,8 @@ class Annotator extends ClosureRewriter { this.emit(`\ngoog.require('${moduleNamespace}'); // force type-only module to be loaded`); } for (let exp of exportedSymbols) { - if (exp.sym.flags & ts.SymbolFlags.Alias) exp.sym = typeChecker.getAliasedSymbol(exp.sym); + if (exp.sym.flags & ts.SymbolFlags.Alias) + exp.sym = this.typeChecker.getAliasedSymbol(exp.sym); // goog: imports don't actually use the .default property that TS thinks they have. const qualifiedName = nsImport && isDefaultImport ? forwardDeclarePrefix : forwardDeclarePrefix + '.' + exp.sym.name; @@ -1106,7 +1148,7 @@ class Annotator extends ClosureRewriter { private emitInterface(iface: ts.InterfaceDeclaration) { // If this symbol is both a type and a value, we cannot emit both into Closure's // single namespace. - let sym = this.program.getTypeChecker().getSymbolAtLocation(iface.name); + let sym = this.typeChecker.getSymbolAtLocation(iface.name); if (sym.flags & ts.SymbolFlags.Value) return; let docTags = this.getJSDoc(iface) || []; @@ -1215,7 +1257,7 @@ class Annotator extends ClosureRewriter { // If the type is also defined as a value, skip emitting it. Closure collapses type & value // namespaces, the two emits would conflict if tsickle emitted both. - let sym = this.program.getTypeChecker().getSymbolAtLocation(node.name); + let sym = this.typeChecker.getSymbolAtLocation(node.name); if (sym.flags & ts.SymbolFlags.Value) return; // Write a Closure typedef, which involves an unused "var" declaration. @@ -1247,7 +1289,7 @@ class Annotator extends ClosureRewriter { for (let member of node.members) { let memberName = member.name.getText(); if (member.initializer) { - let enumConstValue = this.program.getTypeChecker().getConstantValue(member); + let enumConstValue = this.typeChecker.getConstantValue(member); if (enumConstValue !== undefined) { members.set(memberName, enumConstValue); i = enumConstValue + 1; diff --git a/test_files/enum/enum.js b/test_files/enum/enum.js index 670891423..21b698927 100644 --- a/test_files/enum/enum.js +++ b/test_files/enum/enum.js @@ -16,7 +16,7 @@ EnumTest1[EnumTest1.PI] = "PI"; // number. Verify that the resulting TypeScript still allows you to // index into the enum with all the various ways allowed of enums. let /** @type {number} */ enumTestValue = EnumTest1.XYZ; -let /** @type {number} */ enumTestValue2 = EnumTest1['XYZ']; +let /** @type {number} */ enumTestValue2 = EnumTest1.XYZ; let /** @type {string} */ enumNumIndex = EnumTest1[((null))]; let /** @type {number} */ enumStrIndex = EnumTest1[((null))]; /** @@ -25,7 +25,8 @@ let /** @type {number} */ enumStrIndex = EnumTest1[((null))]; */ function enumTestFunction(val) { } enumTestFunction(enumTestValue); -let /** @type {number} */ enumTestLookup = EnumTest1["XYZ"]; +let /** @type {number} */ enumTestLookup = EnumTest1.XYZ; +let /** @type {?} */ enumTestLookup2 = EnumTest1["xyz".toUpperCase()]; exports.EnumTest2 = {}; /** @type {number} */ exports.EnumTest2.XYZ = 0; diff --git a/test_files/enum/enum.ts b/test_files/enum/enum.ts index 82a87233e..64d7d2e5f 100644 --- a/test_files/enum/enum.ts +++ b/test_files/enum/enum.ts @@ -14,6 +14,7 @@ function enumTestFunction(val: EnumTest1) {} enumTestFunction(enumTestValue); let enumTestLookup = EnumTest1["XYZ"]; +let enumTestLookup2 = EnumTest1["xyz".toUpperCase()]; // This additional exported enum is here to exercise the fix for issue #51. export enum EnumTest2 {XYZ, PI = 3.14159} diff --git a/test_files/enum/enum.tsickle.ts b/test_files/enum/enum.tsickle.ts index 60469ba19..91f521ef0 100644 --- a/test_files/enum/enum.tsickle.ts +++ b/test_files/enum/enum.tsickle.ts @@ -21,7 +21,7 @@ EnumTest1[EnumTest1.PI] = "PI"; // number. Verify that the resulting TypeScript still allows you to // index into the enum with all the various ways allowed of enums. let /** @type {number} */ enumTestValue: EnumTest1 = EnumTest1.XYZ; -let /** @type {number} */ enumTestValue2: EnumTest1 = EnumTest1['XYZ']; +let /** @type {number} */ enumTestValue2: EnumTest1 = EnumTest1.XYZ; let /** @type {string} */ enumNumIndex: string = EnumTest1[ /** @type {number} */(( /** @type {?} */((null as any)) as number))]; let /** @type {number} */ enumStrIndex: number = EnumTest1[ /** @type {string} */(( /** @type {?} */((null as any)) as string))]; /** @@ -31,7 +31,8 @@ let /** @type {number} */ enumStrIndex: number = EnumTest1[ /** @type {string} * function enumTestFunction(val: EnumTest1) {} enumTestFunction(enumTestValue); -let /** @type {number} */ enumTestLookup = EnumTest1["XYZ"]; +let /** @type {number} */ enumTestLookup = EnumTest1.XYZ; +let /** @type {?} */ enumTestLookup2 = EnumTest1["xyz".toUpperCase()]; export type EnumTest2 = number; export let EnumTest2: any = {}; /** @type {number} */ diff --git a/test_files/quote_props/quote.js b/test_files/quote_props/quote.js new file mode 100644 index 000000000..7d2fe9716 --- /dev/null +++ b/test_files/quote_props/quote.js @@ -0,0 +1,27 @@ +goog.module('test_files.quote_props.quote');var module = module || {id: 'test_files/quote_props/quote.js'};/** + * @fileoverview added by tsickle + * @suppress {checkTypes} checked by tsc + */ + +/** + * @record + */ +function Quoted() { } +let /** @type {!Quoted} */ quoted = {}; +console.log(quoted['hello']); +quoted['hello'] = 1; +quoted['hello'] = 1; +/** + * @record + * @extends {Quoted} + */ +function QuotedMixed() { } +/** @type {number} */ +QuotedMixed.prototype.foo; +let /** @type {!QuotedMixed} */ quotedMixed = { foo: 1, 'invalid-identifier': 2 }; +console.log(quotedMixed.foo); +quotedMixed.foo = 1; +// Should be converted to non-quoted access. +quotedMixed.foo = 1; +// Must not be converted to non-quoted access, as it's not valid JS. +quotedMixed['invalid-identifier'] = 1; diff --git a/test_files/quote_props/quote.ts b/test_files/quote_props/quote.ts new file mode 100644 index 000000000..de6d83c9a --- /dev/null +++ b/test_files/quote_props/quote.ts @@ -0,0 +1,27 @@ +// silence warnings about redeclaring vars. +export {}; + +interface Quoted { + [k: string]: number; +} +let quoted: Quoted = {}; + +console.log(quoted.hello); +quoted.hello = 1; +quoted['hello'] = 1; + +interface QuotedMixed extends Quoted { + // Assume that foo should be renamed, as it is explicitly declared. + // It's unclear whether it's the right thing to do, user code might + // access this field in a mixed fashion. + foo: number; + 'invalid-identifier': number; +} +let quotedMixed: QuotedMixed = {foo: 1, 'invalid-identifier': 2}; +console.log(quotedMixed.foo); + +quotedMixed.foo = 1; +// Should be converted to non-quoted access. +quotedMixed['foo'] = 1; +// Must not be converted to non-quoted access, as it's not valid JS. +quotedMixed['invalid-identifier'] = 1; diff --git a/test_files/quote_props/quote.tsickle.ts b/test_files/quote_props/quote.tsickle.ts new file mode 100644 index 000000000..a71313aa3 --- /dev/null +++ b/test_files/quote_props/quote.tsickle.ts @@ -0,0 +1,54 @@ +Warning at test_files/quote_props/quote.ts:9:13: Quoted has a string index type but is accessed using dotted access. Quoting the access. +Warning at test_files/quote_props/quote.ts:10:1: Quoted has a string index type but is accessed using dotted access. Quoting the access. +==== +/** + * @fileoverview added by tsickle + * @suppress {checkTypes} checked by tsc + */ + +// silence warnings about redeclaring vars. +export {}; +/** + * @record + */ +function Quoted() {} +/* TODO: handle strange member: +[k: string]: number; +*/ + + +interface Quoted { + [k: string]: number; +} +let /** @type {!Quoted} */ quoted: Quoted = {}; + +console.log(quoted['hello']); +quoted['hello'] = 1; +quoted['hello'] = 1; +/** + * @record + * @extends {Quoted} + */ +function QuotedMixed() {} +/** @type {number} */ +QuotedMixed.prototype.foo; +/* TODO: handle strange member: +'invalid-identifier': number; +*/ + + +interface QuotedMixed extends Quoted { + // Assume that foo should be renamed, as it is explicitly declared. + // It's unclear whether it's the right thing to do, user code might + // access this field in a mixed fashion. + foo: number; + 'invalid-identifier': number; +} +let /** @type {!QuotedMixed} */ quotedMixed: QuotedMixed = {foo: 1, 'invalid-identifier': 2}; +console.log(quotedMixed.foo); + +quotedMixed.foo = 1; +// Should be converted to non-quoted access. +quotedMixed.foo = 1; +// Must not be converted to non-quoted access, as it's not valid JS. +quotedMixed['invalid-identifier'] = 1;