Do not unquote declared properties, but emit a warning.

It turns out users do rely on manually quoted properties not to be
renamed, even if there is a separate declaration for the property - at
least we discovered one instance in Angular doing this.

Author: mprobst

src/tsickle.ts

@@ -764,9 +764,11 @@ class Annotator extends ClosureRewriter {
         }
         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.
+        // Warn for quoted accesses to properties that have a symbol declared.
+        // Mixing quoted and non-quoted access to a symbol (x['foo'] and x.foo) risks breaking
+        // Closure Compiler renaming. Quoted access is more cumbersome to write than dotted access
+        // though, so chances are users did intend to avoid renaming. The better fix is to use
+        // `declare interface` though.
         const eae = node as ts.ElementAccessExpression;
         if (!eae.argumentExpression ||
             eae.argumentExpression.kind !== ts.SyntaxKind.StringLiteral) {
@@ -776,11 +778,18 @@ class Annotator extends ClosureRewriter {
         // 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.
+        // Properties containing non-JS identifier names can only be accessed with quotes.
         if (!isValidClosurePropertyName(propName)) return false;
-        this.writeNode(eae.expression);
-        this.emit(`.${propName}`);
-        return true;
+        const symName = this.typeChecker.symbolToString(quotedPropSym);
+        this.debugWarn(
+            eae,
+            `Declared property ${symName} accessed with quotes. ` +
+                `This can lead to renaming bugs. A better fix is to use 'declare interface' ` +
+                `on the declaration.`);
+        // Previously, the code below changed the quoted into a non-quoted access.
+        // this.writeNode(eae.expression);
+        // this.emit(`.${propName}`);
+        return false;
       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.

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,7 @@ 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} */

test_files/enum/enum.tsickle.ts

@@ -1,4 +1,6 @@
 Warning at test_files/enum/enum.ts:2:7: should not emit a 'never' type
+Warning at test_files/enum/enum.ts:9:33: Declared property XYZ accessed with quotes. This can lead to renaming bugs. A better fix is to use 'declare interface' on the declaration.
+Warning at test_files/enum/enum.ts:16:22: Declared property XYZ accessed with quotes. This can lead to renaming bugs. A better fix is to use 'declare interface' on the declaration.
 ====
 /**
  * @fileoverview added by tsickle
@@ -21,7 +23,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 +33,7 @@ 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 = {};

test_files/quote_props/quote.js

@@ -21,7 +21,11 @@ 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;
+// Intentionally kept as a quoted access, but gives a warning.
+quotedMixed['foo'] = 1;
 // Must not be converted to non-quoted access, as it's not valid JS.
+// Does not give a warning.
 quotedMixed['invalid-identifier'] = 1;
+// any does not declare any symbols.
+let /** @type {?} */ anyTyped;
+anyTyped['token'];

test_files/quote_props/quote.ts

@@ -11,7 +11,8 @@ quoted.hello = 1;
 quoted['hello'] = 1;
 
 interface QuotedMixed extends Quoted {
-  // Assume that foo should be renamed, as it is explicitly declared.
+  // Even though foo is explicitly declared as a property, assume it should not
+  // be renamed.
   // It's unclear whether it's the right thing to do, user code might
   // access this field in a mixed fashion.
   foo: number;
@@ -21,7 +22,12 @@ let quotedMixed: QuotedMixed = {foo: 1, 'invalid-identifier': 2};
 console.log(quotedMixed.foo);
 
 quotedMixed.foo = 1;
-// Should be converted to non-quoted access.
+// Intentionally kept as a quoted access, but gives a warning.
 quotedMixed['foo'] = 1;
 // Must not be converted to non-quoted access, as it's not valid JS.
+// Does not give a warning.
 quotedMixed['invalid-identifier'] = 1;
+
+// any does not declare any symbols.
+let anyTyped: any;
+anyTyped['token'];

test_files/quote_props/quote.tsickle.ts

@@ -1,5 +1,6 @@
 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.
+Warning at test_files/quote_props/quote.ts:26:1: Declared property foo accessed with quotes. This can lead to renaming bugs. A better fix is to use 'declare interface' on the declaration.
 ====
 /**
  * @fileoverview added by tsickle
@@ -38,7 +39,8 @@ QuotedMixed.prototype.foo;
 
 
 interface QuotedMixed extends Quoted {
-  // Assume that foo should be renamed, as it is explicitly declared.
+  // Even though foo is explicitly declared as a property, assume it should not
+  // be renamed.
   // It's unclear whether it's the right thing to do, user code might
   // access this field in a mixed fashion.
   foo: number;
@@ -48,7 +50,12 @@ let /** @type {!QuotedMixed} */ quotedMixed: QuotedMixed = {foo: 1, 'invalid-ide
 console.log(quotedMixed.foo);
 
 quotedMixed.foo = 1;
-// Should be converted to non-quoted access.
-quotedMixed.foo = 1;
+// Intentionally kept as a quoted access, but gives a warning.
+quotedMixed['foo'] = 1;
 // Must not be converted to non-quoted access, as it's not valid JS.
+// Does not give a warning.
 quotedMixed['invalid-identifier'] = 1;
+
+// any does not declare any symbols.
+let /** @type {?} */ anyTyped: any;
+anyTyped['token'];