Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impossible to define static 'length' function on class #442

Closed
hesselink opened this issue Aug 12, 2014 · 22 comments · Fixed by #12065
Closed

Impossible to define static 'length' function on class #442

hesselink opened this issue Aug 12, 2014 · 22 comments · Fixed by #12065
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this

Comments

@hesselink
Copy link

The following typescript program is accepted:

class C {
    static length ()  { return "twelve"; }
}

However, in the generated code:

var C = (function () {
    function C() {
    }
    C.length = function () {
        return "twelve";
    };
    return C;
})();

Here, an attempt is made to assign to the length property of a function. This doesn't work (at least in Firefox and Chrome), causing subsequent calls of C.length() to crash. Perhaps calling a static class function length should just be disallowed?

Migrated from codeplex issue #1260.

@danquirk danquirk added the Bug label Aug 12, 2014
@sophiajt sophiajt added this to the TypeScript 2.0 milestone Aug 12, 2014
@electricessence
Copy link

I ran into this recently. :(

@basarat
Copy link
Contributor

basarat commented Aug 15, 2014

Also a few others are disallowed e.g. the following code should error (if we do this feature):

class C {
    static name = 'something';
    static arguments = 'args';
    static caller = 'caller';
}

console.log(C.name); // 'C'
console.log(C.arguments); // null
console.log(C.caller); // null

Not all of these are stardard however.

@sophiajt sophiajt modified the milestones: TypeScript 2.0, TypeScript 1.2, Community, TypeScript 1.3 Sep 2, 2014
@mhegazy mhegazy added the Help Wanted You can do this label Apr 21, 2015
@tinganho
Copy link
Contributor

I don't think the properties name, caller and length are doable. They are read-only properties and can't be overridden. All assignments to them will be ignored.

@basarat
Copy link
Contributor

basarat commented May 11, 2015

@tinganho agreed

@lazdmx
Copy link

lazdmx commented May 17, 2015

Ok, but since people catch this kind of error from time to time I think that TS should notify them about doing something wrong. It because properties like name or length are so natural to keep in mind so people will try to redefine them again and again.

@Ghayyas
Copy link

Ghayyas commented Oct 21, 2015

May be because Lengh, Name are Already Defined.

@cjbarre
Copy link

cjbarre commented Oct 23, 2015

I'm going to try and work on this. Apart from what has already been mentioned, are there more property names that shouldn't be allowed?

So far I see:
length, name, arguments, caller

What exactly should happen when someone tries to set a static property with one of the forbidden names? Compiler error? Visible warning to the user?

I'm new to this so if I'm missing something, then some guidance would be appreciated. 👍

@zerkms
Copy link

zerkms commented Jan 7, 2016

Why not compile

class C {
    static name = 'something';
    static arguments = 'args';
    static caller = 'caller';
}

console.log(C.name); // 'C'
console.log(C.arguments); // null
console.log(C.caller); // null

to something like

var C = (function () {
    function C() {
    }

    C.__statics = {
        name: 'something',
        arguments: 'args',
        caller: 'caller'
    };

    return C;
})();
console.log(C.__statics.name); // 'something'
console.log(C.__statics.arguments); // 'args'
console.log(C.__statics.caller); // 'caller'

@basarat
Copy link
Contributor

basarat commented Jan 7, 2016

Question came up on stackoverflow as well : http://stackoverflow.com/a/34644236/390330 🌹

@RyanCavanaugh
Copy link
Member

@zerkms please don't post the same comment in two places; it's confusing.

You can't arbitrarily sometimes rewrite names in a structural type system.

Consider some code like this

interface HasName {
  name: string;
}
class Foo {
  static name = 'fooClass';
}
let bar: HasName = { name: string };
let q = Math.random() > 0.5 ? Foo : bar;
console.log(q.name);

@zerkms
Copy link

zerkms commented Jan 7, 2016

@RyanCavanaugh that's a fair example, indeed (I don't develop on TS, but gosh - its type system is weird as soon as it treats this code as valid).

@mattoni
Copy link

mattoni commented Jan 8, 2016

If it's not possible to transpile them without risk, perhaps we should just disallow them?

@basarat
Copy link
Contributor

basarat commented Jan 8, 2016

If it's not possible to transpile them without risk, perhaps we should just disallow them?

Definitely not changing the transpile, just making it a compile error 🌹

@RyanCavanaugh
Copy link
Member

[...] perhaps we should just disallow them?

Some enterprising developer should send us a PR! 😉

@nicolo-ribaudo
Copy link

Why not use Object.defineProperty?

e.g.

class C {
    static length() { return "twelve"; }
}

would be transplied to something like

var C = (function () {
    function C() {
    }
    Object.defineProperty(C, "length", {
        value: function () { return "twelve"; },
        writable: true
    });
    return C;
}());

@basarat
Copy link
Contributor

basarat commented Jul 6, 2016

@nicolo-ribaudo That does indeed work. My opinion: but I would rather see an error instead of such a transpile. TypeScript generally leans on the side of graceful error instead of fixing JavaScript 🌹

@zerkms
Copy link

zerkms commented Jul 6, 2016

TypeScript generally leans on the side of graceful error instead of fixing JavaScript

Is not the whole idea of TS is to fix the JS by filling the gaps it cannot and will never be able to.

@basarat
Copy link
Contributor

basarat commented Jul 6, 2016

Is not the whole idea of TS is to fix the JS by filling the gaps it cannot and will never be able to.

Yes. But by understanding how JavaScript works. Take an example of null and undefined. TypeScript choses to understand both (instead of consolidating it into a single thing like Dart does https://www.dartlang.org/docs/synonyms/). TypeScript gives errors (instead of fixing it in transpile) if you use the wrong JavaScript pattern 🌹

My opinions are my own and not endorsed by anyone but me 🌹

@about-code
Copy link
Contributor

about-code commented Jul 19, 2016

Meanwhile ES6/2015 native support has landed in Chrome and Firefox. If we observe how these handle the issue natively then it comes close to what @nicolo-ribaudo and I have proposed (see #9778).

Snippet to illustrate native behavior:

class Foo {
    constructor() {}
}
class Bar {
    static length() {}
    static name() {}
    static caller() {}
}
Foo.name = "FooChanged";
Bar.name = "Baz";

console.log(Foo.name) // Logs "Foo". Foo.name remains unwritable
console.log(Bar.name) // Logs "Baz".  Bar.name became writable

With respect to what @tinganho has written earlier about the readonlyness of name, length and caller we can observe for the native implementations that the properties are writable on a constructor function once we have defined either of those as a static class member (feel free to review or modify my recent warnings on MDN regarding dangerous assumptions about Function.name for getting a class name, because of this).

So using Object.defineProperty as suggested by @nicolo-ribaudo would emulate the native behavior accurately. A thing which needs consideration is that some older but ES5 compatible browser versions had set the properties to be configurable: false. So trying to make them writable: true will fail for them (e.g. see configurability of Function.name)

To sum things up, my take on the issue were:

  • We can't use Object.defineProperty() for compile-target es3. Without Object.defineProperty, running ES3 code in an ES5 environment, though, would hit the read-only problem. Therefore we need a compile error for target es3
  • compile it with Object.defineProperty() and writable: true for compile-target es5 and issue a warning/error stating that using these property names can cause errors in browsers x of version <= ysuggesting that, if these browsers must be supported, the safest option were to choose another name.
  • if you absolutely don't want people to use the properties, generate a compile error for either target.

Independent of the compile output I think any kind of additional message would be better than how the situation is today (TS 1.8.10) where we get no hint at all about potential problems with the compiler output.

Edit: Snipped added

@about-code
Copy link
Contributor

@RyanCavanaugh: could you please revisit this issue for milestone TS 2.0.1. Solution seems straight forward but depending on which option you choose emitter output may need to change (see my previous comment). Release of TS 2.0 then may be best to include the fix.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 21, 2016

The tags on the PR indicate that the TypeScript team, which have limited resources, are leaving this open to the community to address. If you want this issue addressed, someone in the wider community will have to tackle it until the TypeScript core team feel they have sufficient room in the backlog to put it into a release (which likely would be a long long time).

@about-code
Copy link
Contributor

about-code commented Aug 21, 2016

@kitsonk Thank you for explaining the PR label. I think I understood that. What I suggested was to reevaluate whether its still the right way to handle the issue. Static class properties called name or length are not so unlikely to happen and TS produces errorneous output for that. Hence I wouldn't even call the issue and "edge case".

I consider static properties being at the core of the language and even if there aren't pull requests yet, solutions for the issue have been presented in this thread.

Nevertheless, I will evaluate whether I can provide a PR but I must admit that I am not yet that knowledgeable about TS sources and I am afraid TS 2.0 will be released before.

plantain-00 added a commit to plantain-00/TypeScript that referenced this issue Sep 14, 2016
plantain-00 added a commit to plantain-00/TypeScript that referenced this issue Sep 14, 2016
sandersn added a commit that referenced this issue Jan 17, 2017
Fixing #442: Impossible to define static 'length' function on class
@mhegazy mhegazy modified the milestones: TypeScript 2.2, Community Jan 26, 2017
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 26, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.