Warn when enumerating static properties

[DavidSouther]

In Typescript, I can have a static class property:

class Foo {
  static module = {name: "Foo"};
}

Typescript happily compiles this to

class Foo {
}
Foo.module = { name: "Foo" };

(Static properties aren't valid ES2015: http://www.ecma-international.org/ecma-262/6.0/#sec-class-definitions)

Then, I do this weird thing:

angular.module('myModule', ([Foo, Bar].map(_ => _.module.name))

Where I map over all the classes I intend to instantiate, and prepare their modules.

JSCompiler then helpfully minimizes all this to

angular.a("myModule",[function(){},function(){}].map(function(a){return a.a.name}));

Happily throwing away all the unused properties!

I can add exports:

class Foo {
  /** @export */
  static module = {name: "Foo"};
}

Which Typescript puts in a sane place:

class Foo {
}
/** @export */
Foo.module = { name: "Foo" };

Which JSCompiler ignores.

angular.a("myModule",[function(){},function(){}].map(function(a){return a.a.name}));

I can get away with using the fully qualified form:

angular.module('myModule', [Foo.module.name, Bar.module.name]);

Becomes

angular.a("myModule",["Foo","Bar"]);

Please emit a warning in the property enumeration case.

[evmar]

Minimal repro (I think?)
https://www.typescriptlang.org/play/#src=class%20Foo%20%7B%0D%0A%09static%20bar%20%3D%203%3B%0D%0A%7D%0D%0A%0D%0A%5BFoo%5D.map(x%20%3D%3E%20x.bar)%3B%0D%0A

Input:

class Foo {
    static bar = 3;
}

[Foo].map(x => x.bar);

produces:

var Foo = (function () {
    function Foo() {
    }
    Foo.bar = 3;
    return Foo;
}());
[Foo].map(function (x) { return x.bar; });

But you can't use x.bar, you have to use Foo.bar apparently?

The hard question is, how do we detect when you're doing this!

[mprobst]

But you can't use x.bar, you have to use Foo.bar apparently?

Yes, see here, section about object flattening. This should really be an error/warning in Closure Compiler. Would you mind filing something against them, too, @DavidSouther?

[DavidSouther]

This also means I can't do this for my parameterized tests:

function testSerialization<T extends Base, S extends BaseStatic>(t: T, ctor: S) {
  const serialized = t.serialize();
  const deserialized = ctor.deserialize(serialized);
  expect(deserialized.serialize()).to.deep.equal(serialized);
}

Full example: http://goo.gl/xATcQc

Darn. :(

[DavidSouther]

This should really be an error/warning in Closure Compiler.

google/closure-compiler#1712

[DavidSouther]

....and it's closed, as "not feasible", with @nocollapse as an alternative.

[evmar]

So I think what we could do is check, when you access foo.bar, whether .bar is a static property on a class and if so, whether it has been annotated with @nocollapse. Maybe. Seems like kind of a stretch.

[DavidSouther]

If I'm understanding you, that heuristic is Static class properties that get accessed in the compilation unit get marked @nocollapse?

[evmar]

Sorry it was pretty terse. :)

The Closure rules are:

• it's always safe to use ClassName.staticProp
• if you use let x = ClassName; x.staticProp then it's only safe if staticProp is marked @nocollapse

So what I was suggesting is that for each expression that looks like x.staticProp, we could add a check that ensures that .staticProp is marked @nocollapse somehow. Pretty hand-wavy.

The hard thing is something like this, which is currently legal TS but bad Closure:

class Foo {
  x: number;
  static y: number;
}
interface HasY {
  y: number;
}
let z: HasY = Foo;
z.y;

I think the only way to protect from this is to make Foo somehow not implement the HasY interface (unless, again y is tagged @nocollapse). One hacky idea I have now is that we could rename z's y field to some other thing at tsickle-time...

[mprobst]

Maybe collect all properties that are ever accessed in a non-static way,
and warn if you have any static property with a matching name? Not sure how
many false positives that'd yield.

Evan Martin notifications@github.com schrieb am Mi., 8. Juni 2016, 08:10:

Sorry it was pretty terse. :)

The Closure rules are:

• it's always safe to use ClassName.staticProp
• if you use let x = ClassName; x.staticProp then it's only safe it
  staticProp is marked @nocollapse

So what I was suggesting is that for each expression that looks like
x.staticProp, we could add a check that ensures that .staticProp is
marked @nocollapse somehow. Pretty hand-wavy.

The hard thing is something like this, which is currently legal TS but bad
Closure:

class Foo {
x: number;
static y: number;
}interface HasY {
y: number;
}let z: HasY = Foo;z.y;

I think the only way to protect from this is to make Foo somehow not
implement the HasY interface (unless, again y is tagged @nocollapse). One
hacky idea I have now is that we could rename z's y field to some other
thing at tsickle-time...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#121 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAAcKPDfjaFBUaXTiatMaYQ8ch9x-H_1ks5qJttRgaJpZM4IBNiB
.

[evmar]

Here's the idea I alluded to at the end of my last comment:

1. whenever you see static in a class, if it doesn't have a @nocollapse on it, rewrite the variable name to tsickle_${name} or whatever
2. whenever you reference a property, if it refers to a class static, check for the @nocollapse and do the same rename

Now the class won't match the HasY interface I mentioned above unless you @nocollapse it but otherwise statics behave as before.

The tricky part is #2, especially across d.ts files...

[mprobst]

Yes, (2) is going to be hard in the general case. I think it helps to think about this as a type system feature: we're adding a feature to the type system where you may not use static properties to satisfy an instance level type. Each property of a type would carry an addition flag "isStatic", and on assignment of two values, their property types have to match, but so do their "isStatic" flags, too. Static fields with a @nocollapse would have isStatic = false (which clearly is a misnomer here, I hope it's still understandable). The analogy doesn't quite match - you cannot construct types that demand an "isStatic = true". I think this would not have to work across .d.ts files - any type will always demand only non-isStatic fields, i.e. you can never construct a reference to a value that contains non-isStatic fields. On the .d.ts level, you would thus never need to express types that have isStatic = true on any member. Thinking of it like this, tsickle would have to inspect every type compatibility check (including passing values to function calls, using types as generic type arguments), and error if the RHS has any isStatic properties that appear in the LHS. So, still hard, but maybe somewhat tractable? Another problem is that we have some language constructs like decorators that might be hard to keep track of, and of course there's <any> messing things up.

[evmar]

Another thought: always use @nocollapse and tell people to not use static methods unless they really need the method to be on the class. (You can otherwise make a top-level function.)

[tadeegan]

I do wish the Closure Compiler did a better job at warning people about this.

I'm not that familiar with the TypeScript type system, just looking around there doesn't seem to be a way of expressing a type that is a reference a class itself? If there was such a type, you could emit a warning for all property accesses to an object of that type. It definitely wouldn't catch everything.

class Foo {
  x: number;
  static y: number;
}
interface HasY {
  y: number;
}
let z: HasY = Foo;
z.y;

Could you detect here that you are exposing a static property on to a instance type and throw a warning/error?

The @nocollapse everywhere and documenting the code size implications of using static class properties SGTM.

[evmar]

Since writing

class Foo {
  static x: number;
}

is equivalent to

let Foo_x: number;
class Foo {
}

except that to access one you write Foo.x and the other you write Foo_x, I'm more inclined to strongly warn people away from statics and just always @nocollapse.

Another way of saying this is that "static" is a TS feature and we can just decide it means "never collapse".

Within Google we have a lint warning about overuse of static mostly because it's a pet peeve of mine, unrelated to all of this.

[tadeegan]

I don't think "static" is just a typescript feature. I think its part of the ES6 spec https://babeljs.io/repl/#?babili=false&evaluate=true&lineWrap=false&presets=es2015%2Creact%2Cstage-2&targets=&browsers=&builtIns=false&debug=false&code=class%20A%20%7B%0A%20%20static%20someMethod()%20%7B%7D%0A%7D

[evmar]

You are right, sorry. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static indicates that maybe it's not supported in Safari which seems surprising and maybe just wrong (?)