Quote all property accesses on types with index types. #494
Conversation
|
I'm not sure how to resolve the mixed property/index declaration case. I'm leaning to think that we should not quote properties that have an explicit declaration, but emit a warning in the generated code (or maybe even during the build). Wdyt? |
|
|
||
| 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 |
There was a problem hiding this comment.
in the past we've thought that tsickle should produce a diagnostic for this case, which would be treated as a build error in tsc-wrapped.
You could still get around that with
type QuotedMixed = Quoted & {foo: number};
because type intersections are totally unchecked. Maybe that's good since it provides a way to suppress the tsickle check.
There was a problem hiding this comment.
So the reasoning here is that if you end up with a type that has a property declaration and a string index type, and you do foo.bar, it should come out has foo.bar, but if you do foo['baz'], it should come out quoted. Quoting a property access when the user typed it unquoted and it has been declared somewhere seems wrong.
There's a separate question as to whether we want to disallow mixing string index types and properties in one type, as Closure does. We might want to, but that'll certainly bring some compatibility problems. It's also not generally possible without revisiting every type constructing construct in the program, as you note with the intersection type.
We should think about that, but it's a later decision.
Does that make sense?
|
@alexeagle friendly ping. |
| @@ -26,7 +26,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()]; | |||
There was a problem hiding this comment.
This is trouble, right? could we somehow warn/lint about this kind of computation in a property name?
|
Make sure it is green in google3 too... |
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
TypeScript allows accessing properties with an element access expression, as
long as the argument is a string literal:
const x = { y: number; }
x['y']; // legal
This change turns all those accesses into dotted accesses, to make sure they are
consistently renamed by Closure Compiler.
const x = { y: number; }
x.y; // changed to use dotted access
|
Confirmed working. |
TypeScript 2.3 allows users to use
dotted.accessfor types that (only) have astring index type declared, e.g.
{[k: string]: ...}. This breaks renaming intools 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