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 2 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
101 changes: 85 additions & 16 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5889,16 +5889,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 (!constructorRelatedTo(sourceSignatures[0], targetSignatures[0], reportErrors)) {
Copy link
Member

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?

Copy link
Member

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)

return Ternary.False;
}
return Ternary.False;
}

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

function constructorRelatedTo(sourceSignature: Signature, targetSignature: Signature, reportErrors: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like a better name would be constructorVisibilitiesAreCompatible

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 (sourceSignature && targetSignature && sourceSignature.declaration && targetSignature.declaration) {
Copy link
Member

Choose a reason for hiding this comment

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

When would sourceSignature and targetSignature be undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Same with declaration - those appear to be non-optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are cases where the declarations are undefined. I checked and ran the tests, which caused lots of errors.
But I've removed sourceSignature and targetSignature.

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

const isRelated = sourceAccessibility === targetAccessibility;
if (!isRelated && reportErrors) {
reportError(Diagnostics.Cannot_assign_a_0_constructor_type_to_a_1_constructor_type, flagsToString(sourceAccessibility), flagsToString(targetAccessibility));
}

return isRelated;
}

return true;

function flagsToString(flags: NodeFlags) {
Copy link
Member

Choose a reason for hiding this comment

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

Move this outside the function and rename (visibilityToString ?) (most runtimes allocate a new closure per nested function on invocation, which is bad). You can also try factoring out the logic in lines ~16190-16199 to use this new function if you like.

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) {
return "private";
}
if (flags === NodeFlags.Protected) {
return "protected";
}
return "public";
}
}
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 +10133,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 +10156,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_type_0_is_private_and_only_accessible_within_class_1, signatureToString(signature), typeToString(declaringClass));
}
if (flags & NodeFlags.Protected) {
error(node, Diagnostics.Constructor_of_type_0_is_protected_and_only_accessible_within_class_1, signatureToString(signature), 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 +12123,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 +13992,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 +14048,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_private_class_0, (<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 @@ -16348,12 +16423,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
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 type '{0}' is private and only accessible within class '{1}'.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Constructor of class '{0}' is private and only accessible within the class declaration.

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

"category": "Error",
"code": 2673
},
"Constructor of type '{0}' is protected and only accessible within class '{1}'.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about Constructor of class '{0}' is protected and only accessible within the class '{0}' or a class derived from it.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or a class derived from it.

I have currently implemented it such that a class with a protected constructor cannot be instantiated within a derived class (only extended - thus only from it's super call).
Doesn't this error message go against that? Or do I need to reimplement it such that it can be accessible within derived classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind. my bad.

"category": "Error",
"code": 2674
},
"Cannot extend private class '{0}'.": {
Copy link
Contributor

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.

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

"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 type '(x: number): D' is private and only accessible within class 'D'.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(16,9): error TS2674: Constructor of type '(x: number): E' is protected and only accessible within class 'E'.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(32,13): error TS2673: Constructor of type '<T>(x: T): D<T>' is private and only accessible within class 'D<T>'.
tests/cases/conformance/classes/constructorDeclarations/classConstructorAccessibility.ts(33,13): error TS2674: Constructor of type '<T>(x: T): E<T>' is protected and only accessible within class 'E<T>'.


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

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

53 changes: 37 additions & 16 deletions tests/baselines/reference/classConstructorAccessibility.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,37 @@
//// [classConstructorAccessibility.ts]

class C {
public constructor(public x: number) { }
}

class D {
private constructor(public x: number) { } // error
private constructor(public x: number) { }
}

class E {
protected constructor(public x: number) { } // error
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
var e = new E(1); // error

module Generic {
class C<T> {
public constructor(public x: T) { }
}

class D<T> {
private constructor(public x: T) { } // error
private constructor(public x: T) { }
}

class E<T> {
protected constructor(public x: T) { } // error
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
var e = new E(1); // error
}


Expand All @@ -44,18 +45,18 @@ var C = (function () {
var D = (function () {
function D(x) {
this.x = x;
} // error
}
return D;
}());
var E = (function () {
function E(x) {
this.x = x;
} // error
}
return E;
}());
var c = new C(1);
var d = new D(1);
var e = new E(1);
var d = new D(1); // error
var e = new E(1); // error
var Generic;
(function (Generic) {
var C = (function () {
Expand All @@ -67,16 +68,36 @@ var Generic;
var D = (function () {
function D(x) {
this.x = x;
} // error
}
return D;
}());
var E = (function () {
function E(x) {
this.x = x;
} // error
}
return E;
}());
var c = new C(1);
var d = new D(1);
var e = new E(1);
var d = new D(1); // error
var e = new E(1); // error
})(Generic || (Generic = {}));


//// [classConstructorAccessibility.d.ts]
declare class C {
x: number;
constructor(x: number);
}
declare class D {
x: number;
constructor(x);
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to update the declaration emitter to emit private / protected appropriately

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

}
declare class E {
x: number;
constructor(x: number);
}
declare var c: C;
declare var d: any;
declare var e: any;
declare module Generic {
}
Loading