Fixing #442: Impossible to define static 'length' function on class #12065

about-code · 2016-11-05T17:36:53Z

[x] There is an associated issue that is labelled 'Bug' or 'Accepting PRs' or is in the Community milestone
[x] Code is up-to-date with the master branch
[x] You've successfully run jake runtests locally
[x] You've signed the CLA
[x] There are new or updated unit tests validating the change

Fixes #442

By accepting this pull request when a static property name conflict is detected a new error message

TS2699: Static property 'X' conflicts with built-in property 'Function.X' of constructor function 'Y'

will be issued.

Conflicting method declarations

For example

class Foo {
    static name() {
        return "Bar";
    }
}

produces

TS2699: Static property 'name' conflicts with built-in property 'Function.name' of constructor function 'Foo'

for compile target es5 or lower. Because ES2015 runtimes seem to be able to handle potentially conflicting static method declarations natively (apart from prototype), by internally setting the native function property to writable: true, there won't be an error for target es6 in the current implementation. The absence of an error in this case just indicates, that it will be handled without a runtime error. It may still lead to unexpected results if a user is not aware of overwriting the class constructor's name property which he might want to use elsewhere to read the class name (see examples on MDN). So I leave open to discussions if we should also issue an error for these cases when targeting es6.

Conflicting property declarations

class Foo {
    static name = "Bar";
}

is impossible to fix for target es5 because of Function.name being non-configurable and non-writable by spec. and at least in some es5 runtimes. In es6 targets Function.name is configurable but non-writable by default. Without changes to the emitter above value would not be assigned. It seems like it could be made possible for es6 runtimes and above, using an emit like

Object.defineProperty(Foo, "name", {
   writable: true,
   value: "Bar"
});

Unfortunately I am not able to contribute such change myself. The pragmatic solution will be to also raise a transpile error which some may favour over a transpile, anyway.

Eventually, here's a matrix of the various static member declarations and when a transpile error will be raised:

Target `es3`, `es5`:

member declaration	configurable	writable	How handled
`static prototype = ""`	no	no	error
`static arguments = ""`	no	no	error
`static caller = ""`	no	no	error
`static length = ""`	no	no	error
`static name = ""`	no	no	error

method declaration	configurable	writable	How handled
`static prototype(){...}`	no	no	error
`static arguments(){...}`	no	no	error
`static caller(){...}`	no	no	error
`static length(){...}`	no	no	error
`static name(){...}`	no	no	error

Target `es6` and higher:

member declaration	configurable	writable	How handled
`static prototype = ""`	no	no	error
`static arguments = ""`	yes	no	error*
`static caller = ""`	yes	no	error*
`static length = ""`	yes	no	error*
`static name = ""`	yes	no	error*

method declaration	configurable	writable	How handled
`static prototype(){...}`	no	no	error
`static arguments(){...}`	yes	no**	ok
`static caller(){...}`	yes	no**	ok
`static length(){...}`	yes	no**	ok
`static name(){...}`	yes	no**	ok

* could be enabled by changing emitter output
** property made writable internally by the runtime, once a method declaration exists

Prior work

TypeScript has been preventing the declaration of a static class member prototype since the first public commit by @mhegazy and since then issued a TS2300: Duplicate identifier 'prototype' error.

class Foo {
    static property: any;
}

There were no changes made to the existing implementation. However, having a static prototype property falls into the same category of errors as to be handled for fixing #442. Hence when running the tests for this pull request the baseline for test case tests/cases/conformance/classes/propertyMemberDeclarations/propertyNamedPrototype.ts changed and now also contains a static property conflict error. I accepted this as a new baseline.

Another attempt to fix #442 is PR GH-5396

msftclas · 2016-11-05T17:36:56Z

Hi @about-code, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

… not included in prior commit).

basarat · 2016-11-05T21:18:11Z

Not on the TypeScript dev team. That said, the error message looks fantastic 🌹❤️

…tionInStrictMode1.errors.txt after rebuilding and testing compiler.

about-code · 2016-11-06T00:28:10Z

@basarat you're as visible and active in the TS community as if you were part of the dev team 😄 Have got in touch with TS thanks to you and atom-typescript. So this 🌹 from 🇩🇪 is for you. Glad to here, you like the message.

Disallow non-function properties `static arguments` and `static caller`, though.

…rties.

sandersn

Overall the change looks good. I am not sure the method/property distinction is worthwhile to make in ES6. Not only would it be simpler to explain to people, it would simplify the code as well.

@rbuckton, can you take a look at the proposed semantics? Do you have an opinion about whether we should follow ES6 semantics exactly or simplify them so that using these names is always an error?

sandersn · 2016-12-15T16:46:22Z

src/compiler/checker.ts

@@ -14771,6 +14771,49 @@ namespace ts {
            }
        }

+        // Static members may conflict with non-configurable non-writable built-in Function object properties 


Please use jsdoc format.

sandersn · 2016-12-15T16:47:25Z

src/compiler/checker.ts

@@ -14771,6 +14771,49 @@ namespace ts {
            }
        }

+        // Static members may conflict with non-configurable non-writable built-in Function object properties 
+        // see https://github.com/microsoft/typescript/issues/442.


instead of a github issue number, can you reference the spec the way you do below instead?

sandersn · 2016-12-15T16:49:43Z

src/compiler/checker.ts

+            const message = Diagnostics.Static_property_0_conflicts_with_built_in_property_Function_0_of_constructor_function_1;
+            const className = getSymbolOfNode(node).name;
+            for (const member of node.members) {
+                const isStatic = forEach(member.modifiers, (m: Modifier) => m.kind  === SyntaxKind.StaticKeyword);


should be getModifierFlags(node) & ModifierFlags.Static -- this calculates the modifiers once and then caches them

Yikes, looks like checkClassForDuplicateDeclarations also uses this code. We should change that pretty soon.

sandersn · 2016-12-15T16:51:53Z

src/compiler/checker.ts

+        // see https://github.com/microsoft/typescript/issues/442.
+        function checkClassForStaticPropertyNameConflicts(node: ClassLikeDeclaration) {
+            const message = Diagnostics.Static_property_0_conflicts_with_built_in_property_Function_0_of_constructor_function_1;
+            const className = getSymbolOfNode(node).name;


getNameOfSymbol(getSymbolOfNode(node)) would be better here. You may have to move getNameOfSymbol to make it accessible outside getSymbolDisplayBuilder.

Or just node.name, but that doesn't work for anonymous classes, unfortunately. I think that means some of the other error reporting in checkClassLikeDeclaration is broken, because it uses node.name.

about-code · 2016-12-15T22:17:42Z

@sandersn Thanks for your review and comments. I'll try to implement them as soon as possible. I fully agree with your concerns regarding potential confusion about member/method distinction for es6.

@sandersn

…cker.ts` as proposed by @sandersn

sandersn · 2017-01-03T19:45:58Z

I just talked to @rbuckton and we both agree that we should simplify ES6' semantics and get rid of the method/property distinction. Using these names should always be an error.

DanielRosenwasser · 2017-01-04T05:50:21Z

src/compiler/checker.ts

+         */
+        function checkClassForStaticPropertyNameConflicts(node: ClassLikeDeclaration) {
+            const message = Diagnostics.Static_property_0_conflicts_with_built_in_property_Function_0_of_constructor_function_1;
+            const className = getNameOfSymbol(getSymbolOfNode(node));


Can you add a test for default-exported classes and anonymous class expressions?

Also, I would also only get the information you need when you actually report the error. Consider extracting that into a helper function.

DanielRosenwasser · 2017-01-04T05:54:56Z

src/compiler/checker.ts

+                const isStatic = getModifierFlags(member) & ModifierFlags.Static;
+                const isMethod = member.kind === SyntaxKind.MethodDeclaration;
+                const memberNameNode = member.name;
+                if (isStatic && memberNameNode) {


Reduce nesting by negating this:

if (!isStatic || !memberNameNode) { continue; }

DanielRosenwasser · 2017-01-04T05:58:21Z

src/compiler/checker.ts

+                            memberName === "name" ||
+                            memberName === "length" ||
+                            memberName === "caller" ||
+                            memberName === "arguments"


Consider a switch/case here instead.

DanielRosenwasser · 2017-01-04T06:00:37Z

src/compiler/checker.ts

+                        }
+                    }
+                    else { // ES6+
+                        if (memberName === "prototype") {


switch (memberName) { case "name": case "length": case "caller": case "arguments": if (isMethod) { continue; } // fall through case "prototype": error(/*...*/);

DanielRosenwasser · 2017-01-04T06:02:29Z

Just saw @sandersn's feedback - feel free to simplify on top of any style nits I left behind based on that.

Adding tests with default export and anonymous class expressions.

about-code · 2017-01-14T16:17:55Z

Like @sandersn noted, by omitting es6 method/property distinction we eventually resolve any differences between the currently supported targets. So I changed my proposal to implement the following semantics:

All currently supported targets:

member declaration	method declaration	How handled
`static prototype = ""`	`static prototype(){...}`	error
`static arguments = ""`	`static arguments(){...}`	error
`static caller = ""`	`static caller(){...}`	error
`static length = ""`	`static length(){...}`	error
`static name = ""`	`static name(){...}`	error

Future targets:

I recently found out about a tc39 stage2 proposal about class-public-fields. From your reading of the draft, do you think it correctly addresses potential conflicts? For example would a [[PublicFields]] internal slot as defined in the proposal prevent collisions in future versions of ECMAScript - at least for public static fields?

about-code · 2017-01-14T16:34:39Z

There's also a tc39 proposal for private fields.

msftclas · 2017-01-14T16:34:52Z

Hi @about-code, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

sandersn · 2017-01-17T19:08:32Z

I think it's fine to put the error in as-is, and if/when the class-public-fields proposal moves to stage 3, we can remove the error for the --esnext target (or whatever edition the proposal makes it into the official spec).

You might raise your concerns with the spec authors, though. During class initialisation, everything in [[PublicFields]] gets passed to DefinePropertyOrThrow, which should have the same behaviour as ES6. I think. I'm not sure I'm reading it correctly.

sandersn · 2017-01-17T19:25:44Z

Thanks @about-code!

about-code added 5 commits October 16, 2016 18:34

Fix for issue #442

fc5b2e5

Fix for issue #442

8a37a16

Merge remote-tracking branch 'upstream/master'

9122f62

Accepting new baseline. Format code to fit linter rules.

45e8a5b

Merge remote-tracking branch 'upstream/master'

8fa07d6

msftclas added the cla-signed label Nov 5, 2016

Accept baseline tests. Fixing diagnosticMessages.json (merge result…

189dbdd

… not included in prior commit).

Fixing wrong line number in tests/baselines/reference/variableDeclara…

d58b5c8

…tionInStrictMode1.errors.txt after rebuilding and testing compiler.

Allowing static arguments() and static caller() for target "es6".

d9a46e1

Disallow non-function properties `static arguments` and `static caller`, though.

about-code closed this Nov 6, 2016

about-code added 2 commits November 9, 2016 21:08

Fix #442: (es3, es5, es6+) Show compiler errors for conflicting prope…

b623f37

…rties.

Merge remote-tracking branch 'upstream/master'

02ee7c7

about-code reopened this Nov 9, 2016

sandersn self-assigned this Dec 15, 2016

sandersn requested changes Dec 15, 2016

View reviewed changes

about-code added 2 commits December 30, 2016 12:06

Merge remote-tracking branch 'upstream/master'

ff828fd

Incorporate changes proposed by @sandersn

544d0a4

mhegazy mentioned this pull request Dec 30, 2016

Fixed bug - Certain static member names cannot be used due to non overridable member names on the constructor function type. #5396

Closed

about-code added 2 commits January 3, 2017 14:06

Incorporating changes to checkClassForDuplicateDeclarations in `che…

244db70

…cker.ts` as proposed by @sandersn

Merge remote-tracking branch 'upstream/master'

d78247a

DanielRosenwasser requested changes Jan 4, 2017

View reviewed changes

about-code added 3 commits January 14, 2017 12:24

Merge remote-tracking branch 'upstream/master'

66263b3

Removing es6 method/property distinction.

4718efd

Adding tests with default export and anonymous class expressions.

Removing es6 method/property distinction.

9b217e3

Adding tests with default export and anonymous class expressions.

about-code closed this Jan 14, 2017

about-code reopened this Jan 14, 2017

msftclas added the cla-already-signed label Jan 14, 2017

sandersn approved these changes Jan 17, 2017

View reviewed changes

sandersn merged commit 899d512 into microsoft:master Jan 17, 2017

tinganho mentioned this pull request Jan 21, 2017

Loosen static reserved identifiers errors on ambient declarations #13615

Closed

mhegazy mentioned this pull request Jan 31, 2017

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

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing #442: Impossible to define static 'length' function on class #12065

Fixing #442: Impossible to define static 'length' function on class #12065

about-code commented Nov 5, 2016 •

edited

msftclas commented Nov 5, 2016

basarat commented Nov 5, 2016

about-code commented Nov 6, 2016

sandersn left a comment

sandersn Dec 15, 2016

sandersn Dec 15, 2016

sandersn Dec 15, 2016

sandersn Dec 15, 2016

sandersn Dec 15, 2016

sandersn Dec 15, 2016

about-code commented Dec 15, 2016

sandersn commented Jan 3, 2017

DanielRosenwasser Jan 4, 2017

DanielRosenwasser Jan 4, 2017

DanielRosenwasser Jan 4, 2017

DanielRosenwasser Jan 4, 2017

DanielRosenwasser Jan 4, 2017

DanielRosenwasser commented Jan 4, 2017

about-code commented Jan 14, 2017

about-code commented Jan 14, 2017

msftclas commented Jan 14, 2017

sandersn commented Jan 17, 2017

sandersn commented Jan 17, 2017

Fixing #442: Impossible to define static 'length' function on class #12065

Fixing #442: Impossible to define static 'length' function on class #12065

Conversation

about-code commented Nov 5, 2016 • edited

Conflicting method declarations

Conflicting property declarations

Target es3, es5:

Target es6 and higher:

Prior work

msftclas commented Nov 5, 2016

basarat commented Nov 5, 2016

about-code commented Nov 6, 2016

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

about-code commented Dec 15, 2016

sandersn commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Jan 4, 2017

about-code commented Jan 14, 2017

All currently supported targets:

Future targets:

about-code commented Jan 14, 2017

msftclas commented Jan 14, 2017

sandersn commented Jan 17, 2017

sandersn commented Jan 17, 2017

about-code commented Nov 5, 2016 •

edited

Target `es3`, `es5`:

Target `es6` and higher: