-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Allow visibility on constructors #4174
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
Conversation
Hi @AbubakerB, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
For this to work properly, the visibility needs to be part of the construct signature rather than being a declaration-based check that happens during signature resolution. Otherwise it's going to be too difficult to enforce assignability constraints based on constructor visibility. A relevant testcase to add would be: class Foo {
private constructor() { }
}
class Bar {
protected constructor() { }
}
let x: new() => any;
x = Foo; // Should be an error
x = Bar; // Should be an error |
@RyanCavanaugh SignaturesRelatedTo now checks visibility of constructor. |
Should i close this off? or is this in the pipeline? |
|
||
function checkConstructorVisibility(signature: Signature, node: NewExpression) { | ||
let constructor = result.declaration; | ||
if (!constructor || !node.expression) return; |
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.
Can you wrap these return
statements with curlies and place them on their own line?
if (!constructor || !node.expression) {
return;
}
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
The declaration emitter will need to be amended - add |
The following test cases would be useful too: class Base {
protected constructor() {
}
}
class Derived extends Base {
private constructor() {
super();
}
} class Base {
protected constructor() {
}
}
class Derived extends Base {
protected constructor() {
super();
}
}
var baseCtor = Base;
baseCtor = Derived; |
@AbubakerB sorry this fell off of our radar, we've been hard at work on TypeScript 1.6. @ahejlsberg @mhegazy @vladima any thoughts? |
@DanielRosenwasser I see, that's fine :)
|
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(10,5): error TS1089: 'protected' modifier cannot appear on a constructor declaration. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(23,9): error TS1089: 'private' modifier cannot appear on a constructor declaration. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(27,9): error TS1089: 'protected' modifier cannot appear on a constructor declaration. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(20,9): error TS2655: Constructor '(x: number): D' is private and only accessible within class 'D'. |
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.
Shouldn't this be new (x: number): D
?
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.
What would be the best way of adding the new
before the signature
output?
I can't seem to find any method that writes 'new' + signature
.
@AbubakerB can you get this up-to-date and resubmit in a new PR? Very sorry we left this hanging for so long. |
Sure. I've opened a new PR for this. |
Fixes #2341.
Changes:
1 - Allows private, protected and public accessibilities on constructors.
2 - Disallows classes with private constructors from being extended.
3 - Now throws an error on constructor overloads with different access modifiers (than the constructors implementation).