-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Constructor Visibility #6885
Constructor Visibility #6885
Conversation
Hi @AbubakerB, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility3.ts(26,1): error TS2322: Type 'typeof Foo' is not assignable to type 'typeof Baz'. | ||
Cannot assign a 'public' constructor type to a 'protected' constructor type. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility3.ts(27,1): error TS2322: Type 'typeof Bar' is not assignable to type 'typeof Baz'. | ||
Cannot assign a 'public' constructor type to a 'protected' constructor type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a one-directional relationship: public
is assignable to protected
and private
, and protected
is assignable to private
(but none of the reverse)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"category": "Error", | ||
"code": 2674 | ||
}, | ||
"Cannot extend private class '{0}'.": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
Cannot extend a class '{0}'. Class constructor is marked as private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
return Ternary.False; | ||
} | ||
if (!constructorRelatedTo(sourceSignatures[0], targetSignatures[0], reportErrors)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it just comparing the first of each? Why was this change made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the first has to be checked because the rest must have identical visibility (it's an error for this not to be the case)
Is there anything left for me to do on this? |
@ahejlsberg - take a look? |
Sorry for the delay, we will be discussing this in the TS design meeting tomorrow. should get back to you shortly after. |
@AbubakerB, we have no more comments on the design. Please update for merge conflicts. Just one comment a private constrictor should be visible within nested classes, we do not handle this for private and protected members today (see #7058). |
const enclosingClass = enclosingClassDeclaration ? <InterfaceType>getDeclaredTypeOfSymbol(getSymbolOfNode(enclosingClassDeclaration)) : undefined; | ||
|
||
// A private or protected constructor can only be instantiated within it's own class | ||
if (declaringClass !== enclosingClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as noted in , we #7059, if here is not sufficient, it should be a while loop walking up, looking at all enclosing classes, so that something like this should be legal:
class B {
private constructor() { }
method() {
class C {
method() {
new B(); // should be fine
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
So now, a I also, hopefully, implemented it such that it'll be easy for #7058 😃 |
Build is failing due to gitignore ignoring the tests/**/*.js. |
@AbubakerB thanks! This was a three-day weekend but we're back on Tuesday and should be able to take a last look. |
@AbubakerB, one last merge conflict. but we should be good to go after that. |
Conflicts: src/compiler/checker.ts
Sure :) |
thanks! |
#7058 should be a 1-line fix as well if you are interested. |
@AbubakerB, Since you implemented this feature, do you mind adding some documentation at https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/Classes.md? |
Thanks guys ⛄ ! Sure, no problem |
This is a great change, thanks very much @AbubakerB and all involved |
Was point 4 regarding signature assignability completed as part of this PR? If so, is there documentation with an example? I couldn't find anything that seemed connected or related in the TypeScript Handbook. |
Constructor Visibility
(This is a updated PR from #4174)
There wasn't any detailed proposal, so I'm just going to list what this PR implements.
1) Constructor Visibility
private
orprotected
keyword on its signature.private
,protected
, orpublic
)2) Private Constructors
A class with a private constructor:
3) Protected Constructors
A class with a protected constructor:
4) Signature Assignability