Skip to content

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

Closed
plantain-00 wants to merge 2 commits into
microsoft:masterfrom
plantain-00:442
Closed

Impossible to define static 'length' function on class#9215
plantain-00 wants to merge 2 commits into
microsoft:masterfrom
plantain-00:442

Conversation

@plantain-00

Copy link
Copy Markdown
Contributor

Fixes #442

Comment thread src/compiler/checker.ts Outdated
checkNameOfStaticMethodOrPropertyInClass(node);
}

function checkNameOfStaticMethodOrPropertyInClass(node: ClassElement) {

@yuit yuit Jun 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider this as grammar check -> like call it checkGrammarStaticPropertyName

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do this in a more generic fashion and not hard code the names. we should use the Function type and make sure we are not redefining any of the properties there. possibly readonly ones only. this way users can add/remove some of these checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhegazy and @yuit , thanks for the review, I will find time to fix these.

I'm wondering how to use the Function type, name === nameof Function.length is not allowed yet, and Function[name] !== undefined will not work for the case of name === "apply".

@yuit

yuit commented Jun 16, 2016

Copy link
Copy Markdown
Contributor

Thank you for the PR @plantain-00 !

Additional comment is that I believe there need to be distinction between static property and static method declaration when users target ES5, ES6

  • ES5 : all (length, name, arguments, caller) should be disallowed for both static property and static method declaration
  • ES6: all (length, name, arguments, caller) should be disallowed when declared as static property but (length, arguments, caller, name) should be allow in method declaration ("name" get special treatment definiltely should be allowed here). From trying out in node, when user defined static method declaration with those names they seems override the reserved property except "name". I can't find others in the spec :( @bterlson, could you help confirming whether in ES6(length, arguments, caller) are allowed as static method declaration

EDIT: i make an inline edit, as my original comment is incorrect

@yuit

yuit commented Jun 16, 2016

Copy link
Copy Markdown
Contributor

Regardless of the previous comment, @vladima points out that this rule should be extended to "ComputedProperty" with string literal of the value: length, name, arguments, caller

Could you add additional tests for such case?

@plantain-00

plantain-00 commented Jun 17, 2016

Copy link
Copy Markdown
Contributor Author

@yuit extended to computed property means errors in below code?

class C {
    static caller = { // error: caller
        [name]: "abc", // error: name
        [length]: 1, // error: length
    };
}

@yuit

yuit commented Jun 17, 2016

Copy link
Copy Markdown
Contributor

@plantain-00 almost but using string literal instead. In your example, it is an expression

class C {
    static caller = { // error: caller
        ["name"]: "abc", // error: name
        ["length"]: 1, // error: length
    };
}

@plantain-00 plantain-00 force-pushed the 442 branch 2 times, most recently from 6d94928 to 12e2118 Compare June 19, 2016 00:12
@plantain-00

Copy link
Copy Markdown
Contributor Author

any comments?

@bterlson

Copy link
Copy Markdown
Member

@yuit sorry I missed my ping. Confirming that this change is correct, length, arguments, caller, and name are all valid static method names in ES6.

!!! error TS2690: 'caller' is not allowed to be used as a name of a static property or method in a class.
static foo = {
["length"]: 1,
~~~~~~~~~~

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment(above) from yuit:

this rule should be extended to "ComputedProperty" with string literal of the value: length, name, arguments, caller

@plantain-00 plantain-00 force-pushed the 442 branch 2 times, most recently from dceba92 to f8366b6 Compare September 14, 2016 23:36
@plantain-00

Copy link
Copy Markdown
Contributor Author

any comments?

@mhegazy

mhegazy commented Jan 31, 2017

Copy link
Copy Markdown
Contributor

Fixed by #12065

@mhegazy mhegazy closed this Jan 31, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants