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

[cjbarre]

Proposed patch for issue #442.

Introducing a set of contextually reserved identifier names seems like a big deal, so I'll happily go back and set that up in whatever way seems best, if anyone has suggestions. I'm new at this so that's what I came up with. I think I added the test correctly and the rest of the baselines look good. I had to modify one test because it was using 'name' as a static identifier name. I don't believe the test was specifically testing that bug, but possibly one that was actually being caused by this issue, I changed the property name from 'name' arbitrarily to 'beep' and I don't believe it will affect the test.

I look forward to a review of this and suggestions from everyone. I've never done a PR so if I did this incorrectly please let me know 👍

[msftclas]

Hi @cjbarre, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@cjbarre, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[DanielRosenwasser]

Hey @cjbarre, thanks for the PR!

Right now your checks are in the grammar checking code, whereas I would suggest you should perform the check in checkVariableLikeDeclaration.

I feel like a better approach might be to check whether or not the members exist within the global Function interface.

That might prompt a question as to whether or not we should enforce the same restrictions on an interface with a call/construct signature. My feeling is no, but maybe others could weigh in. @ahejlsberg @RyanCavanaugh @JsonFreeman

[mhegazy]

this makes static declarations with name "toString" for instance illegal, if you are using a map, use ts.hasProperty to check if the property exist on the object it self and not its prototype chain.

[DanielRosenwasser]

@mhegazy what do you think of checking the Function interface?

[mhegazy]

I think we need a lit bit more information here; preferably something that will point that classes constructors are inherently functions. something like:

Duplicate identifier 'name'. Class constructor functions implicitly have a property with the same name

[mhegazy]

@mhegazy what do you think of checking the Function interface?

i agree this is a cleaner approach.

[JsonFreeman]

Agreed about the Function interface. But to clarify something, overriding properties inherited from Object.prototype works, but from Function.prototype does not work? Is that the general rule?

@DanielRosenwasser for your question, I think interfaces with c/c signatures should not have this check. The reason is because interfaces are merely descriptive. My understanding of this check is that it is erroring on code that will try to override these properties, which is not what an interface is doing.

[cjbarre]

Right now your checks are in the grammar checking code, whereas I would suggest you should perform the check in checkVariableLikeDeclaration.

@DanielRosenwasser I agree that sounds like a much better place for something like this.

I feel like a better approach might be to check whether or not the members exist within the global Function interface.

This will ensure that every member on the global Function interface will be checked and centralize the location of these reserved identifier names. It also makes the intention of the code more clear. Sounds good to me!

I think we need a lit bit more information here; preferably something that will point that classes constructors are inherently functions. something like:

Duplicate identifier 'name'. Class constructor functions implicitly have a property with the same name

Absolutely @mhegazy, would you mind explaining this

this makes static declarations with name "toString" for instance illegal, if you are using a map, use ts.hasProperty to check if the property exist on the object it self and not its prototype chain.

a little further?

@DanielRosenwasser for your question, I think interfaces with c/c signatures should not have this check. The reason is because interfaces are merely descriptive. My understanding of this check is that it is erroring on code that will try to override these properties, which is not what an interface is doing.

@JsonFreeman Doesn't defining an interface imply that most of the time it will be implemented at some point? Could it be confusing if someone tries to do this:

interface A {
  static name; // No error
}

class B implements A {
   static name; // Error
}

I think I understand your position, that TypeScript interfaces can be useful without implementations and aren't the same as typical language interfaces like in C# where an interface will always have some sort of implementation, right?

@DanielRosenwasser @mhegazy @JsonFreeman Thanks a lot for the review guys I appreciate it! I have a lot to learn about the compiler :P

[DanielRosenwasser]

@mhegazy meant that if you index into the object with the symbol name like you're currently doing, it will perform a lookup in the object's prototype as well. Since that object's prototype is Object, there will be existing properties like toString, so if your symbol's name is "toString", you'll accidentally say that the lookup succeeded.

We have a function called hasProperty which takes care of this issue, so you could use that.

If you end up using the Function type, I think we have other helper functions. Something like getPropertyOfType.

[mhegazy]

@cjbarre in your change you have a check like:

  if (node.kind === SyntaxKind.Identifier && reservedStaticPropertyNames[node.symbol.name])

checking if the object reservedStaticPropertyNames has a property with value node.symbol.name is not correct, as it will check properties on the object it self (e.g. length, name, arguments, caller) as well as on its prototype, Object in this case. Object has a set of predefined members like toString and valueOf, so now your look up will return true if your symbol is toString, what you should use instead is hasOwnProperty to make sure you are not looking up prototype properties.

Here is a sample i ran in my browser F12 console.

var map = {"a" : "a"};
> undefined
!!map["a"];
> true
!!map["toString"];
> true
map.hasOwnProperty("a");
> true
map.hasOwnProperty("toString");
> false

[cjbarre]

@mhegazy @DanielRosenwasser Thanks guys I understand now! I'm going to use the Function type, so I'll be sure to check out those helper functions.

Any opinions on @JsonFreeman's comments on whether or not interfaces with c/c signatures should have to follow the rules?

[mhegazy]

I agree with @JsonFreeman. we should only do this checking for static properties on classes (and merged namespaces* as well), but not on anything with a call/construct signature.

• here is a test:

class C {
}

namespace C {
     export var name: string;  // should be a duplicate property declaration as C already have a "name" property from the Function interface.
}

[JsonFreeman]

@mhegazy Good point about the merged declarations. What about when the class or namespace is ambient? Again, ambients are just descriptive. So should these properties be allowed for ambients?

Here are some interesting examples:

declare class C {
    static length(): number;
}

class C { }
declare namespace C {
    export function length(): number;
}

declare class C { }
declare namespace C {
    export function length(): number;
}

declare class C { }
namespace C {
    export function length(): number { return 0 }
}

[mhegazy]

I think you are right. ambient should not be errors. we do not know how it was implemented.

[JsonFreeman]

@mhegazy but what about the very last example?

[mhegazy]

i would say error. you are trying to add a function with the same name to an existing class, which possibly will break some functionality, so i would assume you want to know about it.

[JsonFreeman]

Yes, so the rule would be that if the property declaration itself is non-ambient, it is an error.

[mhegazy]

correct

[mhegazy]

@cjbarre please let us know if you have any questions.

[cjbarre]

@mhegazy I definitely will, having a busy week. I think the situation has been complicated a little more than the original request. I'm not yet familiar with the concept checking merged namespaces and checking for c/c signatures.

[mhegazy]

Feel free to ping @DanielRosenwasser, myself, or @JsonFreeman if you have any questions. here is some documentation about declaration merging in general https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/Declaration%20Merging.md

[cjbarre]

@mhegazy @JsonFreeman @DanielRosenwasser

Opinions on a different diagnostic message for namespace merge conflicts? I think it's less obvious than when trying to declare a conflicted static property on a class.

Cannot export an identifier named 'name' in a namespace declaration that is being merged with a class declaration. Class constructor functions implicitly have a property with the same name.

Something to that effect?

[mhegazy]

"Duplicate property declaration" error is fine along with some explanation of "Class constructors implicitly has a property 'name`"

[JsonFreeman]

I am not a fan of the word "implicit" being used in an error message because I think it confuses most users (except implicit any errors). I would say something like "Static property 'name' inherited from type Function cannot be overriden". @cjbarre while I think it is nice to distinguish the merged declaration case, I can't think of a good wording that won't sound confusing.

[mhegazy]

@cjbarre are you still perusing this PR? if so please refresh it and resolve merge conflicts.

[mhegazy]

Closing in favor of #12065