Skip to content
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

Closed
wants to merge 14 commits into from
103 changes: 95 additions & 8 deletions src/compiler/checker.ts
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ===?

Copy link
Contributor Author

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.

return result;
}
}

let sourceErasedSignature = getErasedSignature(sourceSig);
let targetErasedSignature = getErasedSignature(targetSig);

Expand Down Expand Up @@ -5136,6 +5143,32 @@ 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_0_constructor_to_a_1_constructor, visibilityToText(sourceVisibility), visibilityToText(targetVisibility));
}
return Ternary.False;
}
}
}
return Ternary.True;

function visibilityToText(flag: NodeFlags) {
if (flag === NodeFlags.Private) {
return "private";
}
if (flag === NodeFlags.Protected) {
return "protected";
}
return "public";
}
}
}

function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary {
Expand Down Expand Up @@ -8641,6 +8674,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;
}

Expand Down Expand Up @@ -8697,6 +8733,45 @@ namespace ts {
}

return resolveErrorCall(node);

function checkConstructorVisibility(signature: Signature, node: NewExpression) {
let constructor = result.declaration;
if (!constructor || !node.expression) {
return;
}
// if constructor is public, we don't need to check visibility
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_of_type_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_of_type_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;
Expand Down Expand Up @@ -10738,7 +10813,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);
Expand Down Expand Up @@ -14778,12 +14853,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");
}
Expand Down Expand Up @@ -15003,7 +15072,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)) {
return grammarErrorOnFirstToken(expression, Diagnostics.Cannot_extend_private_class_0, (<Identifier>expression).text);
}
}
}
}
}

seenExtendsClause = true;
}
else {
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Expand Up @@ -427,6 +427,10 @@ namespace ts {
Cannot_emit_namespaced_JSX_elements_in_React: { code: 2650, category: DiagnosticCategory.Error, key: "Cannot emit namespaced JSX elements in React" },
A_member_initializer_in_a_const_enum_declaration_cannot_reference_members_declared_after_it_including_members_defined_in_other_const_enums: { code: 2651, category: DiagnosticCategory.Error, key: "A member initializer in a 'const' enum declaration cannot reference members declared after it, including members defined in other 'const' enums." },
Merged_declaration_0_cannot_include_a_default_export_declaration_Consider_adding_a_separate_export_default_0_declaration_instead: { code: 2652, category: DiagnosticCategory.Error, key: "Merged declaration '{0}' cannot include a default export declaration. Consider adding a separate 'export default {0}' declaration instead." },
Cannot_extend_private_class_0: { code: 2653, category: DiagnosticCategory.Error, key: "Cannot extend private class '{0}'." },
Constructor_of_type_0_is_protected_and_only_accessible_within_class_1_and_its_subclasses: { code: 2654, category: DiagnosticCategory.Error, key: "Constructor of type '{0}' is protected and only accessible within class '{1}' and its subclasses." },
Constructor_of_type_0_is_private_and_only_accessible_within_class_1: { code: 2655, category: DiagnosticCategory.Error, key: "Constructor of type '{0}' is private and only accessible within class '{1}'." },
Cannot_assign_a_0_constructor_to_a_1_constructor: { code: 2656, category: DiagnosticCategory.Error, key: "Cannot assign a {0} constructor to a {1} constructor." },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
16 changes: 16 additions & 0 deletions src/compiler/diagnosticMessages.json
Expand Up @@ -1697,6 +1697,22 @@
"category": "Error",
"code": 2652
},
"Cannot extend private class '{0}'.": {
"category": "Error",
"code": 2653
},
"Constructor of type '{0}' is protected and only accessible within class '{1}' and its subclasses.": {
"category": "Error",
"code": 2654
},
"Constructor of type '{0}' is private and only accessible within class '{1}'.": {
"category": "Error",
"code": 2655
},
"Cannot assign a {0} constructor to a {1} constructor.": {
"category": "Error",
"code": 2656
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/types.ts
Expand Up @@ -360,9 +360,9 @@ namespace ts {
export const enum NodeFlags {
Export = 0x00000001, // Declarations
Ambient = 0x00000002, // Declarations
Public = 0x00000010, // Property/Method
Private = 0x00000020, // Property/Method
Protected = 0x00000040, // Property/Method
Public = 0x00000010, // Property/Method/Constructor
Private = 0x00000020, // Property/Method/Constructor
Protected = 0x00000040, // Property/Method/Constructor
Static = 0x00000080, // Property/Method
Abstract = 0x00000100, // Class/Method/ConstructSignature
Async = 0x00000200, // Property/Method/Function
Expand Down
9 changes: 0 additions & 9 deletions tests/baselines/reference/Protected3.errors.txt

This file was deleted.

6 changes: 6 additions & 0 deletions tests/baselines/reference/Protected3.symbols
@@ -0,0 +1,6 @@
=== tests/cases/conformance/parser/ecmascript5/Protected/Protected3.ts ===
class C {
>C : Symbol(C, Decl(Protected3.ts, 0, 0))

protected constructor() { }
}
6 changes: 6 additions & 0 deletions tests/baselines/reference/Protected3.types
@@ -0,0 +1,6 @@
=== tests/cases/conformance/parser/ecmascript5/Protected/Protected3.ts ===
class C {
>C : C

protected constructor() { }
}
67 changes: 45 additions & 22 deletions tests/baselines/reference/classConstructorAccessibility.errors.txt
@@ -1,49 +1,72 @@
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(20,9): error TS2655: Constructor of type '(x: number): D' is private and only accessible within class 'D'.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(21,9): error TS2654: Constructor of type '(x: number): E' is protected and only accessible within class 'E' and its subclasses.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(38,13): error TS2655: Constructor of type '(x: number): D<number>' is private and only accessible within class 'D<T>'.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(39,13): error TS2654: Constructor of type '(x: number): E<number>' is protected and only accessible within class 'E<T>' and its subclasses.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(46,1): error TS2322: Type 'typeof D' is not assignable to type 'new (x: number) => any'.
Cannot assign a private constructor to a public constructor.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(47,1): error TS2322: Type 'typeof E' is not assignable to type 'new (x: number) => any'.
Cannot assign a protected constructor to a public constructor.


==== 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 of type '(x: number): D' is private and only accessible within class 'D'.
var e = new E(1); // error - E is protected
~~~~~~~~
!!! error TS2654: Constructor of type '(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 of type '(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 of type '(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 - private to public
~~~
!!! error TS2322: Type 'typeof D' is not assignable to type 'new (x: number) => any'.
!!! error TS2322: Cannot assign a private constructor to a public constructor.
sig = E; // error - protected to public
~~~
!!! error TS2322: Type 'typeof E' is not assignable to type 'new (x: number) => any'.
!!! error TS2322: Cannot assign a protected constructor to a public constructor.