diff --git a/src/tsickle.ts b/src/tsickle.ts index 1e821096e..695129078 100644 --- a/src/tsickle.ts +++ b/src/tsickle.ts @@ -487,12 +487,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 { @@ -551,7 +554,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) { @@ -598,7 +601,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. @@ -606,7 +608,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); } } @@ -676,8 +678,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)) { @@ -731,7 +732,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) @@ -750,7 +751,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'}); } @@ -761,6 +762,21 @@ class Annotator extends ClosureRewriter { return true; } break; + 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; + 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; } @@ -849,7 +865,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. @@ -859,7 +874,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 && @@ -871,7 +886,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); @@ -906,9 +922,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; @@ -984,12 +1000,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 ...; @@ -997,7 +1012,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); } @@ -1015,15 +1030,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), }; }); } @@ -1042,8 +1055,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). @@ -1065,7 +1078,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; @@ -1105,7 +1119,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) || []; @@ -1214,7 +1228,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. @@ -1246,7 +1260,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/quote_props/quote.js b/test_files/quote_props/quote.js new file mode 100644 index 000000000..bf2992b6d --- /dev/null +++ b/test_files/quote_props/quote.js @@ -0,0 +1,22 @@ +goog.module('test_files.quote_props.quote');var module = module || {id: 'test_files/quote_props/quote.js'};/** + * @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; +// TODO(martinprobst): should 'foo: 1' below be quoted? +let /** @type {!QuotedMixed} */ quotedMixed = { foo: 1 }; +console.log(quotedMixed['foo']); +// TODO(martinprobst): should this access to a declared property be quoted? +quotedMixed['foo'] = 1; +// TODO(martinprobst): should this access to a declared property be un-quoted? +quotedMixed['foo'] = 1; diff --git a/test_files/quote_props/quote.ts b/test_files/quote_props/quote.ts new file mode 100644 index 000000000..1e54321ff --- /dev/null +++ b/test_files/quote_props/quote.ts @@ -0,0 +1,23 @@ +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; +} +// TODO(martinprobst): should 'foo: 1' below be quoted? +let quotedMixed: QuotedMixed = {foo: 1}; +console.log(quotedMixed.foo); + +// TODO(martinprobst): should this access to a declared property be quoted? +quotedMixed.foo = 1; +// TODO(martinprobst): should this access to a declared property be un-quoted? +quotedMixed['foo'] = 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..c90ac01c0 --- /dev/null +++ b/test_files/quote_props/quote.tsickle.ts @@ -0,0 +1,44 @@ +Warning at test_files/quote_props/quote.ts:6:13: Quoted has a string index type but is accessed using dotted access. Quoting the access. +Warning at test_files/quote_props/quote.ts:7:1: Quoted has a string index type but is accessed using dotted access. Quoting the access. +Warning at test_files/quote_props/quote.ts:18:13: QuotedMixed has a string index type but is accessed using dotted access. Quoting the access. +Warning at test_files/quote_props/quote.ts:21:1: QuotedMixed has a string index type but is accessed using dotted access. Quoting the access. +==== + +/** + * @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; + + +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; +} +// TODO(martinprobst): should 'foo: 1' below be quoted? +let /** @type {!QuotedMixed} */ quotedMixed: QuotedMixed = {foo: 1}; +console.log(quotedMixed['foo']); + +// TODO(martinprobst): should this access to a declared property be quoted? +quotedMixed['foo'] = 1; +// TODO(martinprobst): should this access to a declared property be un-quoted? +quotedMixed['foo'] = 1;