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
Allow visibility on constructors #4174
Changes from 8 commits
708f423
9dfa9c7
6d01766
7105a41
51bf395
c87c3c5
f66e66d
5421ac8
ffec744
0e8ecbd
3732fcd
f2d3b29
35fceb9
f6d8a0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5097,6 +5097,13 @@ namespace ts { | |
let targetSig = targetSignatures[0]; | ||
|
||
if (sourceSig && targetSig) { | ||
if (kind === SignatureKind.Construct) { | ||
result &= signatureVisibilityRelatedTo(sourceSig, targetSig); | ||
if (result !== Ternary.True) { | ||
return result; | ||
} | ||
} | ||
|
||
let sourceErasedSignature = getErasedSignature(sourceSig); | ||
let targetErasedSignature = getErasedSignature(targetSig); | ||
|
||
|
@@ -5136,6 +5143,22 @@ namespace ts { | |
} | ||
} | ||
return result; | ||
|
||
function signatureVisibilityRelatedTo(sourceSig: Signature, targetSig: Signature) { | ||
if (sourceSig && targetSig && sourceSig.declaration && sourceSig.declaration.kind === SyntaxKind.Constructor) { | ||
if (sourceSig.declaration && targetSig.declaration) { | ||
let sourceVisibility = sourceSig.declaration.modifiers ? sourceSig.declaration.modifiers.flags : 0; | ||
let targetVisibility = targetSig.declaration.modifiers ? targetSig.declaration.modifiers.flags : 0; | ||
if (sourceVisibility !== targetVisibility && (!((sourceVisibility | targetVisibility) & NodeFlags.Public))) { | ||
if (reportErrors) { | ||
reportError(Diagnostics.Cannot_assign_a_non_public_constructor_type_to_a_public_constructor_type); | ||
} | ||
return Ternary.False; | ||
} | ||
} | ||
} | ||
return Ternary.True; | ||
} | ||
} | ||
|
||
function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary { | ||
|
@@ -8641,6 +8664,9 @@ namespace ts { | |
result = chooseOverload(candidates, assignableRelation); | ||
} | ||
if (result) { | ||
// Check to see if constructor accessibility is valid for this call | ||
checkConstructorVisibility(result, <NewExpression>node); | ||
|
||
return result; | ||
} | ||
|
||
|
@@ -8697,6 +8723,37 @@ namespace ts { | |
} | ||
|
||
return resolveErrorCall(node); | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Can you wrap these if (!constructor || !node.expression) {
return;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// if constructor is public, we dont need to check visibility | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont -> don't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
if (constructor.flags & NodeFlags.Public) return; | ||
|
||
let expressionType = checkExpression(node.expression); | ||
expressionType = getApparentType(expressionType); | ||
if (expressionType === unknownType) return; | ||
|
||
let declaration = expressionType.symbol && getDeclarationOfKind(expressionType.symbol, SyntaxKind.ClassDeclaration); | ||
if (!declaration) return; | ||
|
||
// Get the declaring and enclosing class instance types | ||
let enclosingClassDeclaration = getContainingClass(node); | ||
let enclosingClass = enclosingClassDeclaration ? <InterfaceType>getDeclaredTypeOfSymbol(getSymbolOfNode(enclosingClassDeclaration)) : undefined; | ||
let declaringClass = <InterfaceType>getDeclaredTypeOfSymbol(getSymbolOfNode(<ClassDeclaration>declaration)); | ||
if (constructor.flags & NodeFlags.Private) { | ||
// A private constructor is only accessible in the declaring class | ||
if (declaringClass !== enclosingClass) { | ||
reportError(Diagnostics.Constructor_0_is_private_and_only_accessible_within_class_1, signatureToString(result), typeToString(declaringClass)); | ||
} | ||
} | ||
else if (constructor.flags & NodeFlags.Protected) { | ||
// A protected constructor is only accessible in the declaring class and classes derived from it | ||
if (!enclosingClass || !hasBaseType(enclosingClass, declaringClass)) { | ||
reportError(Diagnostics.Constructor_0_is_protected_and_only_accessible_within_class_1_and_its_subclasses, signatureToString(result), typeToString(declaringClass)); | ||
} | ||
} | ||
} | ||
|
||
function reportError(message: DiagnosticMessage, arg0?: string, arg1?: string, arg2?: string): void { | ||
let errorInfo: DiagnosticMessageChain; | ||
|
@@ -10738,7 +10795,7 @@ namespace ts { | |
error(o.name, Diagnostics.Overload_signatures_must_all_be_ambient_or_non_ambient); | ||
} | ||
else if (deviation & (NodeFlags.Private | NodeFlags.Protected)) { | ||
error(o.name, Diagnostics.Overload_signatures_must_all_be_public_private_or_protected); | ||
error(o.name || o, Diagnostics.Overload_signatures_must_all_be_public_private_or_protected); | ||
} | ||
else if (deviation & NodeFlags.Abstract) { | ||
error(o.name, Diagnostics.Overload_signatures_must_all_be_abstract_or_not_abstract); | ||
|
@@ -14778,12 +14835,6 @@ namespace ts { | |
if (flags & NodeFlags.Abstract) { | ||
return grammarErrorOnNode(lastStatic, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "abstract"); | ||
} | ||
else if (flags & NodeFlags.Protected) { | ||
return grammarErrorOnNode(lastProtected, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "protected"); | ||
} | ||
else if (flags & NodeFlags.Private) { | ||
return grammarErrorOnNode(lastPrivate, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "private"); | ||
} | ||
else if (flags & NodeFlags.Async) { | ||
return grammarErrorOnNode(lastAsync, Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration, "async"); | ||
} | ||
|
@@ -15003,7 +15054,25 @@ namespace ts { | |
if (heritageClause.types.length > 1) { | ||
return grammarErrorOnFirstToken(heritageClause.types[1], Diagnostics.Classes_can_only_extend_a_single_class); | ||
} | ||
|
||
|
||
// if the base class (the class to extend) has a private constructor, | ||
// then the derived class should not be allowed to extend it. | ||
if (heritageClause.types.length == 1) { | ||
let expression = heritageClause.types[0].expression; | ||
if (expression) { | ||
let baseType = getApparentType(checkExpression(expression)); | ||
if (baseType !== unknownType) { | ||
let signatures = getSignaturesOfType(baseType, SignatureKind.Construct); | ||
for (let signature of signatures) { | ||
let constuctor = signature.declaration; | ||
if (constuctor && constuctor.flags & NodeFlags.Private) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
return grammarErrorOnFirstToken(expression, Diagnostics.Cannot_extend_private_class_0, (<Identifier>expression).text); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
seenExtendsClause = true; | ||
} | ||
else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1697,6 +1697,22 @@ | |
"category": "Error", | ||
"code": 2652 | ||
}, | ||
"Cannot extend private class '{0}'.": { | ||
"category": "Error", | ||
"code": 2653 | ||
}, | ||
"Constructor '{0}' is protected and only accessible within class '{1}' and its subclasses.": { | ||
"category": "Error", | ||
"code": 2654 | ||
}, | ||
"Constructor '{0}' is private and only accessible within class '{1}'.": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be "of type '{0}'" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
"category": "Error", | ||
"code": 2655 | ||
}, | ||
"Cannot assign a non-public constructor type to a public constructor type.": { | ||
"category": "Error", | ||
"code": 2656 | ||
}, | ||
"Import declaration '{0}' is using private name '{1}'.": { | ||
"category": "Error", | ||
"code": 4000 | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
=== tests/cases/conformance/parser/ecmascript5/Protected/Protected3.ts === | ||
class C { | ||
>C : Symbol(C, Decl(Protected3.ts, 0, 0)) | ||
|
||
protected constructor() { } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
=== tests/cases/conformance/parser/ecmascript5/Protected/Protected3.ts === | ||
class C { | ||
>C : C | ||
|
||
protected constructor() { } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,71 @@ | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(6,5): error TS1089: 'private' modifier cannot appear on a constructor declaration. | ||
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(19,9): error TS2655: Constructor '(x: number): D' is private and only accessible within class 'D'. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(20,9): error TS2654: Constructor '(x: number): E' is protected and only accessible within class 'E' and its subclasses. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(37,13): error TS2655: Constructor '(x: number): D<number>' is private and only accessible within class 'D<T>'. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(38,13): error TS2654: Constructor '(x: number): E<number>' is protected and only accessible within class 'E<T>' and its subclasses. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(45,1): error TS2322: Type 'typeof D' is not assignable to type 'new (x: number) => any'. | ||
Cannot assign a non-public constructor type to a public constructor type. | ||
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(46,1): error TS2322: Type 'typeof E' is not assignable to type 'new (x: number) => any'. | ||
Cannot assign a non-public constructor type to a public constructor type. | ||
|
||
|
||
==== tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts (4 errors) ==== | ||
==== tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts (6 errors) ==== | ||
class B { | ||
constructor(public x: number) { } | ||
} | ||
|
||
class C { | ||
public constructor(public x: number) { } | ||
} | ||
|
||
class D { | ||
private constructor(public x: number) { } // error | ||
~~~~~~~ | ||
!!! error TS1089: 'private' modifier cannot appear on a constructor declaration. | ||
private constructor(public x: number) { } | ||
} | ||
|
||
class E { | ||
protected constructor(public x: number) { } // error | ||
~~~~~~~~~ | ||
!!! error TS1089: 'protected' modifier cannot appear on a constructor declaration. | ||
protected constructor(public x: number) { } | ||
} | ||
|
||
var b = new B(1); | ||
var c = new C(1); | ||
var d = new D(1); | ||
var e = new E(1); | ||
var d = new D(1); // error - D is private | ||
~~~~~~~~ | ||
!!! error TS2655: Constructor '(x: number): D' is private and only accessible within class 'D'. | ||
var e = new E(1); // error - E is protected | ||
~~~~~~~~ | ||
!!! error TS2654: Constructor '(x: number): E' is protected and only accessible within class 'E' and its subclasses. | ||
|
||
module Generic { | ||
class C<T> { | ||
public constructor(public x: T) { } | ||
} | ||
|
||
class D<T> { | ||
private constructor(public x: T) { } // error | ||
~~~~~~~ | ||
!!! error TS1089: 'private' modifier cannot appear on a constructor declaration. | ||
private constructor(public x: T) { } | ||
} | ||
|
||
class E<T> { | ||
protected constructor(public x: T) { } // error | ||
~~~~~~~~~ | ||
!!! error TS1089: 'protected' modifier cannot appear on a constructor declaration. | ||
protected constructor(public x: T) { } | ||
} | ||
|
||
var b = new B(1); | ||
var c = new C(1); | ||
var d = new D(1); | ||
var e = new E(1); | ||
var d = new D(1); // error - D is private | ||
~~~~~~~~ | ||
!!! error TS2655: Constructor '(x: number): D<number>' is private and only accessible within class 'D<T>'. | ||
var e = new E(1); // error - E is protected | ||
~~~~~~~~ | ||
!!! error TS2654: Constructor '(x: number): E<number>' is protected and only accessible within class 'E<T>' and its subclasses. | ||
} | ||
|
||
|
||
// make sure signatures are covered. | ||
let sig: new(x: number) => any; | ||
sig = B; | ||
sig = C; | ||
sig = D; // error - non-public to public | ||
~~~ | ||
!!! error TS2322: Type 'typeof D' is not assignable to type 'new (x: number) => any'. | ||
!!! error TS2322: Cannot assign a non-public constructor type to a public constructor type. | ||
sig = E; // error - non-public to public | ||
~~~ | ||
!!! error TS2322: Type 'typeof E' is not assignable to type 'new (x: number) => any'. | ||
!!! error TS2322: Cannot assign a non-public constructor type to a public 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.
Shouldn't this be
===
?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.
I thought I should follow the same structure as here.
It checks to see if its not assignable (resulted from
signatureVisibilityRelatedTo
) and returns if not.