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 3 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 : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

do not use null; use undefined

if (valueDecl && valueDecl.flags & NodeFlags.Abstract) {
error(node, Diagnostics.Cannot_create_an_instance_of_the_abstract_class_1, 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_1: { code: 2501, category: DiagnosticCategory.Error, key: "Cannot create an instance of the abstract class '{1}'" },
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 '{1}'": {
Copy link
Contributor

Choose a reason for hiding this comment

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

"cannot use new on an abstract class {1}."

@JsonFreeman may have better message though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oke, Will change this (but I will wait for feedback from JsonFreeman)
The message I got now is more or less the same as the one from C#.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1, and not 0 (for the substitution slot)?

I think I like the "Cannot create an instance" message better.

"category": "Error",
"code": 2501
Copy link
Member

Choose a reason for hiding this comment

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

Codes should be distinct

},

"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 @@ -83,6 +83,7 @@ module ts {
"private": SyntaxKind.PrivateKeyword,
"protected": SyntaxKind.ProtectedKeyword,
"public": SyntaxKind.PublicKeyword,
"abstract": SyntaxKind.AbstractKeyword,
"require": SyntaxKind.RequireKeyword,
"return": SyntaxKind.ReturnKeyword,
"set": SyntaxKind.SetKeyword,
Expand Down
20 changes: 11 additions & 9 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ module ts {
PrivateKeyword,
ProtectedKeyword,
PublicKeyword,
AbstractKeyword,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a future reserved word in ES6, should move to the contextual keywords block. you should also add a test for variables named "abstract" with "use strict";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jup will do, didn't wrote a (full) test-suite yet because I wanted to know if the code was a bit in the right places

StaticKeyword,
YieldKeyword,
// Contextual keywords
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
47 changes: 47 additions & 0 deletions tests/baselines/reference/abstractClass1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
tests/cases/compiler/abstractClass1.ts(15,9): error TS2501: Cannot create an instance of the abstract class 'undefined'
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 TS2501: Cannot create an instance of the abstract class 'undefined'
tests/cases/compiler/abstractClass1.ts(28,1): error TS2346: Supplied parameters do not match any signature of call target.
tests/cases/compiler/abstractClass1.ts(28,1): error TS2501: Cannot create an instance of the abstract class 'undefined'


==== tests/cases/compiler/abstractClass1.ts (5 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 TS2501: Cannot create an instance of the abstract class 'undefined'
var b = new Foo(); // Error because of invalid constructor arguments
~~~~~~~~~
!!! error TS2346: Supplied parameters do not match any signature of call target.
~~~~~~~~~
!!! error TS2501: Cannot create an instance of the abstract class 'undefined'


// 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 TS2501: Cannot create an instance of the abstract class 'undefined'

76 changes: 76 additions & 0 deletions tests/baselines/reference/abstractClass1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//// [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


// 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
// 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 var c: Bar;
declare var Copy: typeof Foo;
29 changes: 29 additions & 0 deletions tests/cases/compiler/abstractClass1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// @declaration: true

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


// 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();