diff --git a/src/tsickle.ts b/src/tsickle.ts index 695129078..9a1385a52 100644 --- a/src/tsickle.ts +++ b/src/tsickle.ts @@ -762,6 +762,22 @@ 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; + 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. @@ -769,6 +785,16 @@ class Annotator extends ClosureRewriter { 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) + 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 index bf2992b6d..1cdff7147 100644 --- a/test_files/quote_props/quote.js +++ b/test_files/quote_props/quote.js @@ -1,4 +1,5 @@ -goog.module('test_files.quote_props.quote');var module = module || {id: 'test_files/quote_props/quote.js'};/** +goog.module('test_files.quote_props.quote');var module = module || {id: 'test_files/quote_props/quote.js'}; +/** * @record */ function Quoted() { } @@ -13,10 +14,8 @@ quoted['hello'] = 1; 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; +console.log(quotedMixed.foo); +quotedMixed.foo = 1; +// Should be converted to non-quoted access. +quotedMixed.foo = 1; diff --git a/test_files/quote_props/quote.ts b/test_files/quote_props/quote.ts index 1e54321ff..4a8be3113 100644 --- a/test_files/quote_props/quote.ts +++ b/test_files/quote_props/quote.ts @@ -1,3 +1,6 @@ +// silence warnings about redeclaring vars. +export {}; + interface Quoted { [k: string]: number; } @@ -13,11 +16,9 @@ interface QuotedMixed extends Quoted { // 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? +// Should be converted to non-quoted access. quotedMixed['foo'] = 1; diff --git a/test_files/quote_props/quote.tsickle.ts b/test_files/quote_props/quote.tsickle.ts index c90ac01c0..37d90aa27 100644 --- a/test_files/quote_props/quote.tsickle.ts +++ b/test_files/quote_props/quote.tsickle.ts @@ -1,9 +1,8 @@ -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. +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. ==== - +// silence warnings about redeclaring vars. +export {}; /** * @record */ @@ -11,6 +10,8 @@ function Quoted() {} /* TODO: handle strange member: [k: string]: number; */ + + interface Quoted { [k: string]: number; } @@ -34,11 +35,9 @@ interface QuotedMixed extends Quoted { // 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']); +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; +quotedMixed.foo = 1; +// Should be converted to non-quoted access. +quotedMixed.foo = 1;