Quote all property accesses on types with index types.

TypeScript 2.3 allows users to use `dotted.access` for types that (only) have a
string index type declared, e.g. `{[k: string]: ...}`. This breaks renaming in
tools such as Closure Compiler (which tsickle targets), as some locations might
access the property with quotes, some without.

This is an incomplete fix: TypeScript allows assigning values with properties of
the matching type into types with index signatures. Users can then access the
original value with dots, but the aliased value with quotes, which breaks, too.
This is probably not fixable without a global aliasing analysis of the program.

See also:
    microsoft/TypeScript#14267
    microsoft/TypeScript#15206

Author: mprobst

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<string>();
 
+  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,15 +602,14 @@ 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.
           exportedSymbols = this.expandSymbolsFromExportStar(exportDecl);
           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 = <ts.FunctionLikeDeclaration>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,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;
     }
@@ -850,7 +866,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 +875,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<string>();
     for (let local of locals) {
       if (local.declarations &&
@@ -872,7 +887,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<ts.Symbol>();
     for (let sym of exports) {
       let name = unescapeName(sym.name);
@@ -907,9 +923,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,20 +1001,19 @@ 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 ...;
           if (!importClause.namedBindings ||
               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 +1031,13 @@ class Annotator extends ClosureRewriter {
     }
   }
 
-  private getNamedSymbols(
-      specifiers: Array<ts.ImportSpecifier|ts.ExportSpecifier>,
-      typeChecker: ts.TypeChecker): NamedSymbol[] {
+  private getNamedSymbols(specifiers: Array<ts.ImportSpecifier|ts.ExportSpecifier>): 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 +1056,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 +1079,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 +1120,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 +1229,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 +1261,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;

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;

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;

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;