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

Constructor Visibility #6885

Merged
merged 18 commits into from
Feb 17, 2016
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 103 additions & 24 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1711,6 +1711,16 @@ namespace ts {
return result;
}

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

function getTypeAliasForTypeLiteral(type: Type): Symbol {
if (type.symbol && type.symbol.flags & SymbolFlags.TypeLiteral) {
let node = type.symbol.declarations[0].parent;
Expand Down Expand Up @@ -5889,16 +5899,20 @@ namespace ts {

const sourceSignatures = getSignaturesOfType(source, kind);
const targetSignatures = getSignaturesOfType(target, kind);
if (kind === SignatureKind.Construct && sourceSignatures.length && targetSignatures.length &&
isAbstractConstructorType(source) && !isAbstractConstructorType(target)) {
// An abstract constructor type is not assignable to a non-abstract constructor type
// as it would otherwise be possible to new an abstract class. Note that the assignablity
// check we perform for an extends clause excludes construct signatures from the target,
// so this check never proceeds.
if (reportErrors) {
reportError(Diagnostics.Cannot_assign_an_abstract_constructor_type_to_a_non_abstract_constructor_type);
if (kind === SignatureKind.Construct && sourceSignatures.length && targetSignatures.length) {
if (isAbstractConstructorType(source) && !isAbstractConstructorType(target)) {
// An abstract constructor type is not assignable to a non-abstract constructor type
// as it would otherwise be possible to new an abstract class. Note that the assignablity
// check we perform for an extends clause excludes construct signatures from the target,
// so this check never proceeds.
if (reportErrors) {
reportError(Diagnostics.Cannot_assign_an_abstract_constructor_type_to_a_non_abstract_constructor_type);
}
return Ternary.False;
}
if (!constructorVisibilitiesAreCompatible(sourceSignatures[0], targetSignatures[0], reportErrors)) {
return Ternary.False;
}
return Ternary.False;
}

let result = Ternary.True;
Expand Down Expand Up @@ -6052,6 +6066,36 @@ namespace ts {
}
return Ternary.True;
}

function constructorVisibilitiesAreCompatible(sourceSignature: Signature, targetSignature: Signature, reportErrors: boolean) {
if (!sourceSignature.declaration || !targetSignature.declaration) {
return true;
}

const sourceAccessibility = sourceSignature.declaration.flags & (NodeFlags.Private | NodeFlags.Protected);
const targetAccessibility = targetSignature.declaration.flags & (NodeFlags.Private | NodeFlags.Protected);

// A public, protected and private signature is assignable to a private signature.
if (targetAccessibility === NodeFlags.Private) {
return true;
}

// A public and protected signature is assignable to a protected signature.
if (targetAccessibility === NodeFlags.Protected && sourceAccessibility !== NodeFlags.Private) {
return true;
}

// Only a public signature is assignable to public signature.
if (targetAccessibility !== NodeFlags.Protected && !sourceAccessibility) {
return true;
}

if (reportErrors) {
reportError(Diagnostics.Cannot_assign_a_0_constructor_type_to_a_1_constructor_type, visibilityToString(sourceAccessibility), visibilityToString(targetAccessibility));
}

return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

If you don't get rid of the if, please add an explicit return statement.

}

// Return true if the given type is the constructor type for an abstract class
Expand Down Expand Up @@ -10103,6 +10147,9 @@ namespace ts {
// that the user will not add any.
const constructSignatures = getSignaturesOfType(expressionType, SignatureKind.Construct);
if (constructSignatures.length) {
if (!isConstructorAccessible(node, constructSignatures[0])) {
return resolveErrorCall(node);
}
return resolveCall(node, constructSignatures, candidatesOutArray);
}

Expand All @@ -10123,6 +10170,37 @@ namespace ts {
return resolveErrorCall(node);
}

function isConstructorAccessible(node: NewExpression, signature: Signature) {
if (!signature || !signature.declaration) {
return true;
}

const declaration = signature.declaration;
const flags = declaration.flags;

// Public constructor is accessible.
if (!(flags & (NodeFlags.Private | NodeFlags.Protected))) {
return true;
}

const declaringClass = <InterfaceType>getDeclaredTypeOfSymbol(declaration.parent.symbol);
const enclosingClassDeclaration = getContainingClass(node);
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) {
Copy link
Contributor

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
            }
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (flags & NodeFlags.Private) {
error(node, Diagnostics.Constructor_of_class_0_is_private_and_only_accessible_within_the_class_declaration, typeToString(declaringClass));
}
if (flags & NodeFlags.Protected) {
error(node, Diagnostics.Constructor_of_class_0_is_protected_and_only_accessible_within_the_class_declaration, typeToString(declaringClass));
}
return false;
}

return true;
}

function resolveTaggedTemplateExpression(node: TaggedTemplateExpression, candidatesOutArray: Signature[]): Signature {
const tagType = checkExpression(node.tag);
const apparentType = getApparentType(tagType);
Expand Down Expand Up @@ -12059,7 +12137,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 @@ -13928,6 +14006,7 @@ namespace ts {
if (baseTypes.length && produceDiagnostics) {
const baseType = baseTypes[0];
const staticBaseType = getBaseConstructorTypeOfClass(type);
checkBaseTypeAccessibility(staticBaseType, baseTypeNode);
checkSourceElement(baseTypeNode.expression);
if (baseTypeNode.typeArguments) {
forEach(baseTypeNode.typeArguments, checkSourceElement);
Expand Down Expand Up @@ -13983,6 +14062,16 @@ namespace ts {
}
}

function checkBaseTypeAccessibility(type: ObjectType, node: ExpressionWithTypeArguments) {
const signatures = getSignaturesOfType(type, SignatureKind.Construct);
if (signatures.length) {
const declaration = signatures[0].declaration;
if (declaration && declaration.flags & NodeFlags.Private) {
error(node, Diagnostics.Cannot_extend_a_class_0_Class_constructor_is_marked_as_private, (<Identifier>node.expression).text);
}
}
}

function getTargetSymbol(s: Symbol) {
// if symbol is instantiated its flags are not copied from the 'target'
// so we'll need to get back original 'target' symbol to work with correct set of flags
Expand Down Expand Up @@ -16188,16 +16277,12 @@ namespace ts {
case SyntaxKind.PublicKeyword:
case SyntaxKind.ProtectedKeyword:
case SyntaxKind.PrivateKeyword:
let text: string;
if (modifier.kind === SyntaxKind.PublicKeyword) {
text = "public";
}
else if (modifier.kind === SyntaxKind.ProtectedKeyword) {
text = "protected";
let text = visibilityToString(modifierToFlag(modifier.kind));

if (modifier.kind === SyntaxKind.ProtectedKeyword) {
lastProtected = modifier;
}
else {
text = "private";
else if (modifier.kind === SyntaxKind.PrivateKeyword) {
lastPrivate = modifier;
}

Expand Down Expand Up @@ -16348,12 +16433,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
2 changes: 1 addition & 1 deletion src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,7 @@ namespace ts {
if (node.kind === SyntaxKind.FunctionDeclaration) {
emitModuleElementDeclarationFlags(node);
}
else if (node.kind === SyntaxKind.MethodDeclaration) {
else if (node.kind === SyntaxKind.MethodDeclaration || node.kind === SyntaxKind.Constructor) {
emitClassMemberDeclarationFlags(node.flags);
}
if (node.kind === SyntaxKind.FunctionDeclaration) {
Expand Down
16 changes: 16 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,22 @@
"category": "Error",
"code": 2671
},
"Cannot assign a '{0}' constructor type to a '{1}' constructor type.": {
"category": "Error",
"code": 2672
},
"Constructor of class '{0}' is private and only accessible within the class declaration.": {
"category": "Error",
"code": 2673
},
"Constructor of class '{0}' is protected and only accessible within the class declaration.": {
"category": "Error",
"code": 2674
},
"Cannot extend a class '{0}'. Class constructor is marked as private.": {
"category": "Error",
"code": 2675
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
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
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() { }
}
6 changes: 6 additions & 0 deletions tests/baselines/reference/Protected3.types
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() { }
}
41 changes: 21 additions & 20 deletions tests/baselines/reference/classConstructorAccessibility.errors.txt
Original file line number Diff line number Diff line change
@@ -1,49 +1,50 @@
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(15,9): error TS2673: Constructor of class 'D' is private and only accessible within the class declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(16,9): error TS2674: Constructor of class 'E' is protected and only accessible within the class declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(32,13): error TS2673: Constructor of class 'D<T>' is private and only accessible within the class declaration.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(33,13): error TS2674: Constructor of class 'E<T>' is protected and only accessible within the class declaration.


==== tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts (4 errors) ====

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 c = new C(1);
var d = new D(1);
var e = new E(1);
var d = new D(1); // error
~~~~~~~~
!!! error TS2673: Constructor of class 'D' is private and only accessible within the class declaration.
var e = new E(1); // error
~~~~~~~~
!!! error TS2674: Constructor of class 'E' is protected and only accessible within the class declaration.

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 c = new C(1);
var d = new D(1);
var e = new E(1);
var d = new D(1); // error
~~~~~~~~
!!! error TS2673: Constructor of class 'D<T>' is private and only accessible within the class declaration.
var e = new E(1); // error
~~~~~~~~
!!! error TS2674: Constructor of class 'E<T>' is protected and only accessible within the class declaration.
}

Loading