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

Review PR: Abstract classes #2946

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7092,6 +7092,11 @@ module ts {
return resolveErrorCall(node);
}

var valueDecl = (expressionType.symbol ? expressionType.symbol.valueDeclaration : undefined);
if (valueDecl && valueDecl.flags & NodeFlags.Abstract) {
error(node, Diagnostics.Cannot_create_an_instance_of_the_abstract_class_0, declarationNameToString(valueDecl.name));
}

// Technically, this signatures list may be incomplete. We are taking the apparent type,
// but we are not including construct signatures that may have been added to the Object or
// Function interface, since they have none by default. This is a bit of a leap of faith
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,10 @@ module ts {

emitJsDocComments(node);
emitModuleElementDeclarationFlags(node);
if (node.flags & NodeFlags.Abstract) {
write("abstract ");
}

write("class ");
writeTextOfNode(currentSourceFile, node.name);
let prevEnclosingDeclaration = enclosingDeclaration;
Expand Down
1 change: 1 addition & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ module ts {
An_interface_can_only_extend_an_identifier_Slashqualified_name_with_optional_type_arguments: { code: 2499, category: DiagnosticCategory.Error, key: "An interface can only extend an identifier/qualified-name with optional type arguments." },
A_class_can_only_implement_an_identifier_Slashqualified_name_with_optional_type_arguments: { code: 2500, category: DiagnosticCategory.Error, key: "A class can only implement an identifier/qualified-name with optional type arguments." },
A_rest_element_cannot_contain_a_binding_pattern: { code: 2501, category: DiagnosticCategory.Error, key: "A rest element cannot contain a binding pattern." },
Cannot_create_an_instance_of_the_abstract_class_0: { code: 2502, category: DiagnosticCategory.Error, key: "Cannot create an instance of the abstract class '{0}'" },
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
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1447,6 +1447,10 @@
"category": "Error",
"code": 2501
},
"Cannot create an instance of the abstract class '{0}'": {
"category": "Error",
"code": 2502
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
1 change: 1 addition & 0 deletions src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ module ts {
}

let textToToken: Map<SyntaxKind> = {
"abstract": SyntaxKind.AbstractKeyword,
"any": SyntaxKind.AnyKeyword,
"as": SyntaxKind.AsKeyword,
"boolean": SyntaxKind.BooleanKeyword,
Expand Down
20 changes: 11 additions & 9 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ module ts {
StaticKeyword,
YieldKeyword,
// Contextual keywords
AbstractKeyword,
AsKeyword,
AnyKeyword,
BooleanKeyword,
Expand Down Expand Up @@ -307,15 +308,16 @@ module ts {
Protected = 0x00000040, // Property/Method
Static = 0x00000080, // Property/Method
Default = 0x00000100, // Function/Class (export default declaration)
MultiLine = 0x00000200, // Multi-line array or object literal
Synthetic = 0x00000400, // Synthetic node (for full fidelity)
DeclarationFile = 0x00000800, // Node is a .d.ts file
Let = 0x00001000, // Variable declaration
Const = 0x00002000, // Variable declaration
OctalLiteral = 0x00004000,
ExportContext = 0x00008000, // Export context (initialized by binding)

Modifier = Export | Ambient | Public | Private | Protected | Static | Default,
Abstract = 0x00000200,
MultiLine = 0x00000400, // Multi-line array or object literal
Synthetic = 0x00000800, // Synthetic node (for full fidelity)
DeclarationFile = 0x00001000, // Node is a .d.ts file
Let = 0x00002000, // Variable declaration
Const = 0x00004000, // Variable declaration
OctalLiteral = 0x00008000,
ExportContext = 0x00010000, // Export context (initialized by binding)

Modifier = Export | Ambient | Public | Private | Protected | Static | Default | Abstract,
AccessibilityModifier = Public | Private | Protected,
BlockScoped = Let | Const
}
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,7 @@ module ts {

export function isModifier(token: SyntaxKind): boolean {
switch (token) {
case SyntaxKind.AbstractKeyword:
case SyntaxKind.PublicKeyword:
case SyntaxKind.PrivateKeyword:
case SyntaxKind.ProtectedKeyword:
Expand Down Expand Up @@ -1626,6 +1627,7 @@ module ts {
case SyntaxKind.PublicKeyword: return NodeFlags.Public;
case SyntaxKind.ProtectedKeyword: return NodeFlags.Protected;
case SyntaxKind.PrivateKeyword: return NodeFlags.Private;
case SyntaxKind.AbstractKeyword: return NodeFlags.Abstract;
case SyntaxKind.ExportKeyword: return NodeFlags.Export;
case SyntaxKind.DeclareKeyword: return NodeFlags.Ambient;
case SyntaxKind.ConstKeyword: return NodeFlags.Const;
Expand Down
20 changes: 10 additions & 10 deletions tests/baselines/reference/APISample_linter.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,26 +75,26 @@ function delint(sourceFile) {
delintNode(sourceFile);
function delintNode(node) {
switch (node.kind) {
case 186 /* ForStatement */:
case 187 /* ForInStatement */:
case 185 /* WhileStatement */:
case 184 /* DoStatement */:
if (node.statement.kind !== 179 /* Block */) {
case 187 /* ForStatement */:
case 188 /* ForInStatement */:
case 186 /* WhileStatement */:
case 185 /* DoStatement */:
if (node.statement.kind !== 180 /* Block */) {
report(node, "A looping statement's contents should be wrapped in a block body.");
}
break;
case 183 /* IfStatement */:
case 184 /* IfStatement */:
var ifStatement = node;
if (ifStatement.thenStatement.kind !== 179 /* Block */) {
if (ifStatement.thenStatement.kind !== 180 /* Block */) {
report(ifStatement.thenStatement, "An if statement's contents should be wrapped in a block body.");
}
if (ifStatement.elseStatement &&
ifStatement.elseStatement.kind !== 179 /* Block */ &&
ifStatement.elseStatement.kind !== 183 /* IfStatement */) {
ifStatement.elseStatement.kind !== 180 /* Block */ &&
ifStatement.elseStatement.kind !== 184 /* IfStatement */) {
report(ifStatement.elseStatement, "An else statement's contents should be wrapped in a block body.");
}
break;
case 169 /* BinaryExpression */:
case 170 /* BinaryExpression */:
var op = node.operatorToken.kind;
if (op === 28 /* EqualsEqualsToken */ || op == 29 /* ExclamationEqualsToken */) {
report(node, "Use '===' and '!=='.");
Expand Down
57 changes: 57 additions & 0 deletions tests/baselines/reference/abstractClass1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
tests/cases/compiler/abstractClass1.ts(15,9): error TS2502: Cannot create an instance of the abstract class 'Foo'
tests/cases/compiler/abstractClass1.ts(16,9): error TS2346: Supplied parameters do not match any signature of call target.
tests/cases/compiler/abstractClass1.ts(16,9): error TS2502: Cannot create an instance of the abstract class 'Foo'
tests/cases/compiler/abstractClass1.ts(25,1): error TS2502: Cannot create an instance of the abstract class 'Qux'
tests/cases/compiler/abstractClass1.ts(35,1): error TS2346: Supplied parameters do not match any signature of call target.
tests/cases/compiler/abstractClass1.ts(35,1): error TS2502: Cannot create an instance of the abstract class 'Foo'


==== tests/cases/compiler/abstractClass1.ts (6 errors) ====

abstract class Foo {
constructor(f: any) { }
public static bar(): void { }

public empty() { }
}

class Bar extends Foo {
constructor(f: any) {
super(f);
}
}

var a = new Foo(1); // Error
~~~~~~~~~~
!!! error TS2502: Cannot create an instance of the abstract class 'Foo'
var b = new Foo(); // Error because of invalid constructor arguments
~~~~~~~~~
!!! error TS2346: Supplied parameters do not match any signature of call target.
~~~~~~~~~
!!! error TS2502: Cannot create an instance of the abstract class 'Foo'

module baz {
export abstract class Qux {
}
export class Quz extends Qux {
}
}

new baz.Qux();
~~~~~~~~~~~~~
!!! error TS2502: Cannot create an instance of the abstract class 'Qux'

// Valid
var c = new Bar(1);
c.empty();

// Calling a static method on a abstract class is valid
Foo.bar();

var Copy = Foo;
new Copy();
Copy link
Member

Choose a reason for hiding this comment

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

We want this to be allowed

Copy link
Contributor

Choose a reason for hiding this comment

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

what about

abstract class A {

}

import alias = A;

new alias();

I would argue this has to be an error.. else import {abstractClass} from "module" is not going to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is the case, the check should not use type.symbol, it should use resolveName at this location, and only be restricted to identifiers and qualified names.

this looks like a more correct approach though. i think you can always cast to something that is not abstract to get away with it.. it feels a lot like privates in classes that we preserve on the type.

~~~~~~~~~~
!!! error TS2346: Supplied parameters do not match any signature of call target.
~~~~~~~~~~
!!! error TS2502: Cannot create an instance of the abstract class 'Foo'

107 changes: 107 additions & 0 deletions tests/baselines/reference/abstractClass1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
//// [abstractClass1.ts]

abstract class Foo {
constructor(f: any) { }
public static bar(): void { }

public empty() { }
}

class Bar extends Foo {
constructor(f: any) {
super(f);
}
}

var a = new Foo(1); // Error
var b = new Foo(); // Error because of invalid constructor arguments

module baz {
export abstract class Qux {
}
export class Quz extends Qux {
}
}

new baz.Qux();

// Valid
var c = new Bar(1);
c.empty();

// Calling a static method on a abstract class is valid
Foo.bar();

var Copy = Foo;
new Copy();


//// [abstractClass1.js]
var __extends = this.__extends || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
function __() { this.constructor = d; }
__.prototype = b.prototype;
d.prototype = new __();
};
var Foo = (function () {
function Foo(f) {
}
Foo.bar = function () { };
Foo.prototype.empty = function () { };
return Foo;
})();
var Bar = (function (_super) {
__extends(Bar, _super);
function Bar(f) {
_super.call(this, f);
}
return Bar;
})(Foo);
var a = new Foo(1); // Error
var b = new Foo(); // Error because of invalid constructor arguments
var baz;
(function (baz) {
var Qux = (function () {
function Qux() {
}
return Qux;
})();
baz.Qux = Qux;
var Quz = (function (_super) {
__extends(Quz, _super);
function Quz() {
_super.apply(this, arguments);
}
return Quz;
})(Qux);
baz.Quz = Quz;
})(baz || (baz = {}));
new baz.Qux();
// Valid
var c = new Bar(1);
c.empty();
// Calling a static method on a abstract class is valid
Foo.bar();
var Copy = Foo;
new Copy();


//// [abstractClass1.d.ts]
declare abstract class Foo {
constructor(f: any);
static bar(): void;
empty(): void;
}
declare class Bar extends Foo {
constructor(f: any);
}
declare var a: Foo;
declare var b: any;
declare module baz {
abstract class Qux {
}
class Quz extends Qux {
}
}
declare var c: Bar;
declare var Copy: typeof Foo;
14 changes: 14 additions & 0 deletions tests/baselines/reference/abstractClassIdentifierName.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [abstractClassIdentifierName.ts]
class abstract {

abstract(): void { }
}


//// [abstractClassIdentifierName.js]
var abstract = (function () {
function abstract() {
}
abstract.prototype.abstract = function () { };
return abstract;
})();
8 changes: 8 additions & 0 deletions tests/baselines/reference/abstractClassIdentifierName.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/compiler/abstractClassIdentifierName.ts ===
class abstract {
>abstract : Symbol(abstract, Decl(abstractClassIdentifierName.ts, 0, 0))

abstract(): void { }
>abstract : Symbol(abstract, Decl(abstractClassIdentifierName.ts, 0, 16))
}

8 changes: 8 additions & 0 deletions tests/baselines/reference/abstractClassIdentifierName.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/compiler/abstractClassIdentifierName.ts ===
class abstract {
>abstract : abstract

abstract(): void { }
>abstract : () => void
}

14 changes: 14 additions & 0 deletions tests/baselines/reference/abstractIdentifierName.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [abstractIdentifierName.ts]
var abstract = true;

function foo() {
"use strict";
var abstract = true;
}

//// [abstractIdentifierName.js]
var abstract = true;
function foo() {
"use strict";
var abstract = true;
}
11 changes: 11 additions & 0 deletions tests/baselines/reference/abstractIdentifierName.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/compiler/abstractIdentifierName.ts ===
var abstract = true;
>abstract : Symbol(abstract, Decl(abstractIdentifierName.ts, 0, 3))

function foo() {
>foo : Symbol(foo, Decl(abstractIdentifierName.ts, 0, 20))

"use strict";
var abstract = true;
>abstract : Symbol(abstract, Decl(abstractIdentifierName.ts, 4, 7))
}
15 changes: 15 additions & 0 deletions tests/baselines/reference/abstractIdentifierName.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
=== tests/cases/compiler/abstractIdentifierName.ts ===
var abstract = true;
>abstract : boolean
>true : boolean

function foo() {
>foo : () => void

"use strict";
>"use strict" : string

var abstract = true;
>abstract : boolean
>true : boolean
}
8 changes: 8 additions & 0 deletions tests/baselines/reference/abstractInterfaceIdentifierName.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//// [abstractInterfaceIdentifierName.ts]

interface abstract {
abstract(): void;
}


//// [abstractInterfaceIdentifierName.js]
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
=== tests/cases/compiler/abstractInterfaceIdentifierName.ts ===

interface abstract {
>abstract : Symbol(abstract, Decl(abstractInterfaceIdentifierName.ts, 0, 0))

abstract(): void;
>abstract : Symbol(abstract, Decl(abstractInterfaceIdentifierName.ts, 1, 20))
}

Loading