Proposal: quoted and unquoted property names distinct

[alexeagle]

Closure Compiler is a JavaScript optimizer that also works well with TypeScript (using our https://github.com/angular/tsickle as an intermediate re-writer). It produces the smallest bundles, and we use this internally at Google and externally for some Angular users to get the smallest application.

In ADVANCED_OPTIMIZATIONS mode, Closure Compiler renames non-local properties. It uses a simple rule: unquoted property accesses can be renamed, but quoted property access cannot. This can break a program in the presence of mixed quoted and non-quoted property access, as a trivial example:

window.foo = "hello world";
console.log(window["foo"]);

Is minified by Closure Compiler [1] as

window.a = "hello world";
console.log(window.foo); // prints "undefined"

Currently, Closure Compiler puts the burden of correct quoted/unquoted access on the author. This is documented here: https://developers.google.com/closure/compiler/docs/api-tutorial3#propnames

With TypeScript's type-checker we believe we could flag the majority of cases where property renaming breaks a users program. We propose to introduce an option that makes quoted and unquoted properties be separate, non-matching members. In the proposal below, assume we enable this behavior with an option --strictPropertyNaming

Treat quoted and unquoted properties as distinct

Currently, TypeScript allows quoted access to named properties (and vice versa):

interface SeemsSafe {
    foo?: {};
}

let b: SeemsSafe = {};
b["foo"] = 1; // This access should fail under --strictPropertyNaming
b.foo = 2; // okay

Also, newly introduced in TypeScript 2.2, the inverse problem exists:

interface HasIndexSig {
  [key: string]: boolean;
}
let c: HasIndexSig;
c.foo = true; // This access should fail under --strictPropertyNaming

Defining types whose members should not be renamed

It's convenient for users to specify a type that insures the properties are not renamed. For example, when an XHR returns, property accesses of the JSON data must not be renamed.

// Under --strictPropertyNaming, the quotes on these properties matter.
// They must be accessed quoted, not unquoted.
interface JSONData {
  'username': string;
  'phone': number;
}

let data = JSON.parse(xhrResult.text) as JSONData;
console.log(data['phone']); // okay
console.log(data.username); // should be error under --strictPropertyNaming 

Structural matches

Two types should not be a structural match if their quoting differs:

interface JSONData {
  'username': string;
}

class SomeInternalType {
  username: string;
}

let data = JSON.parse(xhrResult.text) as JSONData;
let myObj: SomeInternalType = data;  // should fail under --strictPropertyNaming
console.log(myObj.username); // would get broken by property renaming

Avoid a mix of Index Signatures and named properties

Optional: we could add a semantic check for .ts inputs that disallows any type to have both an index signature and named properties.

interface Unsafe {
    [prop: string]: {};
    foo?: {}; // This could be a semantic error with --strictPropertyNaming
}

let a: Unsafe = {};
a.foo = 1;
a["foo"] = 2;

Note that the intersection operator & still defeats such a check:

type Unsafe = {
    [prop: string]: {};
} & {
    foo?: {}; // This is uncheckable because the intersection never fails
}

We should not check .d.ts inputs as they may have been compiled without --strictPropertyNaming.

Compatibility with libraries

If a library is developed with --strictPropertyNaming, the resulting .d.ts files should be usable by any program whether it opts into the flag or not. There is one corner case however.

The following example should probably produce a declarationDiagnostic, because the generated .d.ts would be an error when used in a compilation without --strictPropertyNaming.

type C = {
  a: number;
  'a': string; // this one should probably error
  [key: string]: number;
}

A simpler alternative is just to allow this case, and produce it in .d.ts files. Then downstream consumers will have an error unless they turn on --strictPropertyNaming or --noLibCheck.

Either choice here is okay with us.

Property names that require quoting

In this case, there is no choice but to quote the identifier:

interface JSONData {
  'hy-phen': number;
}

This continues to work under --strictPropertyNaming, but the implication is that such an identifier is forced to be non-renamable since there is no unquoted syntax to declare it. This seems fine, it results in a lost optimization only for such names, which we assume are rare.

Property-renaming safety

There are some cases that will remain unsafe:

• the any type still turns off type-checking, including checking quoted vs. unquoted access
• libraries developed without --strictPropertyNaming use unquoted identifiers which should not be renamed (such as document.getElementById. Closure Compiler already has an 'externs' mechanism that prevents the renaming. In the TypeScript code it will not be evident that the properties are not renamed, but this is the same situation we have today.

[1] https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540formatting%2520pretty_print%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Awindow.foo%2520%253D%2520%2522hello%2520world%2522%253B%250Aconsole.log(window%255B%2522foo%2522%255D)%253B%250A

[mhegazy]

Wouldn't this be something that a tool like https://github.com/angular/tsickle should handle instead?

[alexeagle]

Why not do it in tsickle?

• we would have to implement our own type-checker. Right now tsickle is just a syntax-directed tree transform. We'll move it into the emit transform pipeline API soon. Imagine if some emit transform wanted to modify the type system in this way.
• it would be slow to type-check the program a second time during emit
• we want editor tooling to suggest correct completions, error highlighting, etc

[mhegazy]

• we would have to implement our own type-checker. Right now tsickle is just a syntax-directed tree transform. We'll move it into the emit transform pipeline API soon. Imagine if some emit transform wanted to modify the type system in this way.
• it would be slow to type-check the program a second time during emit

The new transfomration pipeline should allow tsickle to just plugin with no need for an extra pass. You get the typechecker state as an input, so no need for extra type checking.

• we want editor tooling to suggest correct completions, error highlighting, etc

I was suggesting that tsickle rewrites the output based on the type. i.e. i.value => i["value"] if typeof i does not have an explicit declaration for value. So this way, no need for errors to be reported in the editor, it should just work.

[alexeagle]

As you can see in the linked history from tsickle, @mprobst tried converting in both directions (change to quoted access on types with an index signature, change to unquoted access on types with named properties) and we rolled it out internally at Google.

We hit a number of problems. For one thing, this becomes type-directed emit which is just not how TS works. Practically it also had a bunch of holes (eg. the types remain assignable, initial assignment is unchecked).

I think next step is for me, Martin, and @rkirov to write up more clearly why the tsickle re-writing doesn't work. We could then either

• revisit this issue so that we have rigor and correctness
• try another experiment where tsickle (or tslint) produces warnings when we think the quoting might be wrong

[rkirov]

An update on what transpired in the last 1.5 years. As Alex described we implemented the minimal transformation in angular/tsickle - if the user wrote foo.bar on foo which was of a type T that had an index signature and there was no other definition of the property bar in T, we emitted foo['bar'] in the .js file instead of the original foo.bar in the .ts file.

This mostly worked, but every month or so someone will stumble upon this. In some scenarios this change (along with the usual Closure optimization behavior) actually breaks code that is working fine without it. For example:

let x: {[key: string]: string} = {someProperty: 'foo'};
x.someProperty;

we would emit:

let x = {someProperty: 'foo'};
x['someProperty'];

which Closure will change to (remember the rule is simple in closure, all non-quoted properties get changed)

let x = {a: 'foo'};
x['someProperty'];

You might wonder why would one write an index signature in this scenario. Usually, this happens with a large object with many properties that the user is too lazy to spell out again. I recommend just using type-inference, which sidesteps the whole issue, but we can't really check each index signature usage to see if it is legitimate.

Also debugging this type of issues has proved nightmarish for our users because noone expects that tsickle will change the emit. Users mostly look at the source .ts and the minified output, and cannot reason through each step of the pipeline. Also, as Alex said, at a high-level non-type directed .js emit is generally how TS works, and every time we have violated that it has caused surprise and confusion. I think TS team has similar experience with for-of, const enums, etc.

So, in the last month, we switched to making this a compilation error instead of tsickle rewrite -
https://github.com/bazelbuild/rules_typescript/blob/master/internal/tsetse/rules/property_renaming_safe.ts and
angular/tsickle@ba13814

We have received some fair criticisms that we are inventing a custom flavor of TS that is not officially approved, so we would still love to see some action on --strictPropertyNaming. We might further downgrade this to tslint, so that it is easier to run this check in repositories that don't use Bazel.

Here is one stylistic benefit that comes to mind with --strictPropertyNaming (in the minimal version that we have implemented). With that check on when one sees myObj.myLongProperty they know that the string myLongProperty is checked for typos (unless myObj is any as usual). Without --strictPropertyNaming one can get by without checking on the name of the property through an index signature and is exposed to a runtime error in case of a typo. Most TS authors know to limit usage of any, but index signatures still slide by as not everyone realizes the loss of static guarantees that come with them.