Skip to content

Report error if class uses extended type before its declaration #4343

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

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
75c143b
Add testcase for #4341
sheetalkamat Aug 17, 2015
cddf947
Reports error if class declaration appears before extended type decla…
sheetalkamat Aug 17, 2015
4de36cd
Test cases for class expressions
sheetalkamat Aug 17, 2015
9296e87
Fixed cases where base type is declared in heritage clause itself
sheetalkamat Aug 17, 2015
bf64986
Fix existing tests and baselines
sheetalkamat Aug 17, 2015
4bd17de
Adding tests for class declarations in different files
sheetalkamat Aug 17, 2015
35b6f48
Adding tests for #4244
sheetalkamat Aug 18, 2015
d29f8d4
If the use of the block scoped variable occurs inside function do not…
sheetalkamat Aug 18, 2015
6ba40ff
Add tests for class declarations with extends clause inside function
sheetalkamat Aug 18, 2015
a4c91e2
Adding tests for class declarations inside function block but differe…
sheetalkamat Aug 18, 2015
5e92286
Adding tests for imported class from module as base type
sheetalkamat Aug 18, 2015
284ea97
Fix the case when the internal alias is used in extends clause
sheetalkamat Aug 18, 2015
3009fd5
Fixed the reporting of error if extends clause uses internal alias as…
sheetalkamat Aug 18, 2015
ea2a655
Report error if alias references entity before its declaration
sheetalkamat Aug 19, 2015
9fa406c
Merge branch 'master' into baseClassDeclarationOrder
sheetalkamat Aug 19, 2015
66dc9e4
Merge branch 'master' into baseClassDeclarationOrder
sheetalkamat Aug 24, 2015
a7f85d5
Taking into account code review feedback
sheetalkamat Aug 24, 2015
bdfb7a2
Use function like to determine if it is in different scope
sheetalkamat Aug 24, 2015
ad1d8da
Took more code review feedback into account
sheetalkamat Aug 24, 2015
76fd5e4
Taken into account code review feedback
sheetalkamat Aug 27, 2015
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
66 changes: 59 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,21 +381,30 @@ namespace ts {
}

/** Returns true if node1 is defined before node 2**/
function isDefinedBefore(node1: Node, node2: Node): boolean {
let file1 = getSourceFileOfNode(node1);
let file2 = getSourceFileOfNode(node2);
if (file1 === file2) {
return node1.pos <= node2.pos;
function isDefinedBefore(referenceDeclaration: Node, locationNode: Node): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

isDefinedBeforeOrAt

if (isAliasSymbolDeclaration(referenceDeclaration) && !isInternalModuleImportEqualsDeclaration(referenceDeclaration)) {
// external aliases always declared before use
return true;
}

let referenceDeclarationFile = getSourceFileOfNode(referenceDeclaration);
let locationNodeFile = getSourceFileOfNode(locationNode);
if (referenceDeclarationFile === locationNodeFile) {
return referenceDeclaration.pos <= locationNode.pos || !isInSameContainingFunction();
}

if (!compilerOptions.outFile && !compilerOptions.out) {
return true;
}

let sourceFiles = host.getSourceFiles();
return sourceFiles.indexOf(file1) <= sourceFiles.indexOf(file2);
}
return indexOf(sourceFiles, referenceDeclarationFile) <= indexOf(sourceFiles, locationNodeFile) || !isInSameContainingFunction();

function isInSameContainingFunction() {
return getContainingFunction(referenceDeclaration) === getContainingFunction(locationNode);
}
}

// Resolve a given name for a given meaning at a given location. An error is reported if the name was not found and
// the nameNotFoundMessage argument is not undefined. Returns the resolved symbol, or undefined if no symbol with
// the given name can be found.
Expand Down Expand Up @@ -12619,6 +12628,13 @@ namespace ts {
}
}
checkKindsOfPropertyMemberOverrides(type, baseType);

// Check if base type declaration appears before heritage clause to avoid false errors for
// base type declarations in the extend clause itself
Copy link
Contributor

Choose a reason for hiding this comment

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

What would an example of this look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

class a extends class b { }  {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think isDefinedBefore takes care of that because it compares pos with <=

let baseTypeDeclaration = getLeftmostAliasDeclarationFromExpressionOrEnityName(baseTypeNode.expression) || baseType.symbol.declarations[0];
if (!isDefinedBefore(baseTypeDeclaration, baseTypeNode)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not we check that baseTypeDeclaration is not undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why/ because we get it either as alias in the expression use or base type expression's symbol's declaration which should always be there isn't it?

error(baseTypeNode, Diagnostics.Base_expression_references_type_before_it_is_declared);
}
}
}

Expand Down Expand Up @@ -12650,6 +12666,37 @@ namespace ts {
}
}

function getLeftmostAliasDeclarationFromExpressionOrEnityName(node: Expression | EntityName): Declaration {
switch (node.kind) {
case SyntaxKind.PropertyAccessExpression:
let result = getLeftmostAliasDeclarationFromExpressionOrEnityName((<PropertyAccessExpression>node).expression);
if (result) {
return result;
}
break;

case SyntaxKind.QualifiedName:
let result2 = getLeftmostAliasDeclarationFromExpressionOrEnityName((<QualifiedName>node).left);
if (result2) {
return result2;
}

case SyntaxKind.Identifier:
break;

default:
return;
}

let meaning = node.parent.kind === SyntaxKind.QualifiedName || node.parent.kind === SyntaxKind.PropertyAccessExpression?
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix spacing at the end of the line

SymbolFlags.Namespace :
Copy link
Contributor

Choose a reason for hiding this comment

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

If the parent is a property access, it could also be a value, no? I would expect SymbolFlags.Value | SymbolFlags.Namespace

SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace;
let resolvedSymbol = resolveEntityName(node, meaning | SymbolFlags.Alias, /*ignoreErrors*/ true);
if (resolvedSymbol && !!(resolvedSymbol.flags & SymbolFlags.Alias)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why can not we just retrn the first declaration of resolvedSymbol all the time? it is either an alias, or the symbol we are looking for correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it could be something like reopening modules. So in below case it should still be error to use M.B as base type expression:

module M {
}

class C2 extends M.B { // error
}
module M {
    export class B {
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't B the one we care about here? Seems like getLeftmostAliasDeclarationFromExpressionOrEnityName would return M in your example, but why is M interesting at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because: eg.

    module M {
        export module Q {
            export class C {
            }
        }

        export class C {
        }
        export import im = Q.C;
    }
    class B1 extends im1.C { // this should be reported as error
    }
    import im1 = M;

Copy link
Contributor

Choose a reason for hiding this comment

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

But does that have to do with aliases at all? I think that's an error just because the declaration for im1.C (namely the class declaration C) is before B1. Why do aliases have a fundamental role to play in the detection?

return getDeclarationOfAliasSymbol(resolvedSymbol);
}
}

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 @@ -13315,6 +13362,11 @@ namespace ts {
if (!(resolveEntityName(moduleName, SymbolFlags.Value | SymbolFlags.Namespace).flags & SymbolFlags.Namespace)) {
error(moduleName, Diagnostics.Module_0_is_hidden_by_a_local_declaration_with_the_same_name, declarationNameToString(moduleName));
}

let internalReferenceDecl = getLeftmostAliasDeclarationFromExpressionOrEnityName(<EntityName>node.moduleReference) || target.declarations[0];
if (!isDefinedBefore(internalReferenceDecl, node.moduleReference)) {
error(node.moduleReference, Diagnostics.Import_declaration_references_entity_before_it_is_declared);
}
}
if (target.flags & SymbolFlags.Type) {
checkTypeNameIsReserved(node.name, Diagnostics.Import_name_cannot_be_0);
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,8 @@ 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." },
Base_expression_references_type_before_it_is_declared: { code: 2653, category: DiagnosticCategory.Error, key: "Base expression references type before it is declared." },
Import_declaration_references_entity_before_it_is_declared: { code: 2654, category: DiagnosticCategory.Error, key: "Import declaration references entity before it is declared." },
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
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1697,6 +1697,14 @@
"category": "Error",
"code": 2652
},
"Base expression references type before it is declared.": {
"category": "Error",
"code": 2653
},
"Import declaration references entity before it is declared.": {
"category": "Error",
"code": 2654
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down
52 changes: 52 additions & 0 deletions tests/baselines/reference/aliasOutOfOrder.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
tests/cases/compiler/aliasOutOfOrder.ts(1,12): error TS2654: Import declaration references entity before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(3,12): error TS2654: Import declaration references entity before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(5,12): error TS2654: Import declaration references entity before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(6,12): error TS2654: Import declaration references entity before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(12,17): error TS2653: Base expression references type before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(22,13): error TS2654: Import declaration references entity before it is declared.
tests/cases/compiler/aliasOutOfOrder.ts(24,13): error TS2654: Import declaration references entity before it is declared.


==== tests/cases/compiler/aliasOutOfOrder.ts (7 errors) ====
import a = M.B; // error
~~~
!!! error TS2654: Import declaration references entity before it is declared.
import b = M.I; // no error
import d = c.B; // error
~~~
!!! error TS2654: Import declaration references entity before it is declared.
import e = c.I; // no error
import f = c; // error
~
!!! error TS2654: Import declaration references entity before it is declared.
import c = M; // error
~
!!! error TS2654: Import declaration references entity before it is declared.
import g = c.B; // no error
import h = c.I; // no error
import i = c; // no error
class C extends a { // no error
}
class D extends M.B { // error
~~~
!!! error TS2653: Base expression references type before it is declared.
}
module M {
export class B {
}
export interface I {
}
}
import a1 = M.B; // no error
import b1 = M.I; // no error
import d1 = c1.B; // error
~~~~
!!! error TS2654: Import declaration references entity before it is declared.
import e1 = c1.I; // no error
import f1 = c1; // error
~~
!!! error TS2654: Import declaration references entity before it is declared.
import c1 = M; // no error
import g1 = c1.B; // no error
import h1 = c1.I; // no error
import i1 = c1; // no error
71 changes: 71 additions & 0 deletions tests/baselines/reference/aliasOutOfOrder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//// [aliasOutOfOrder.ts]
import a = M.B; // error
import b = M.I; // no error
import d = c.B; // error
import e = c.I; // no error
import f = c; // error
import c = M; // error
import g = c.B; // no error
import h = c.I; // no error
import i = c; // no error
class C extends a { // no error
}
class D extends M.B { // error
}
module M {
export class B {
}
export interface I {
}
}
import a1 = M.B; // no error
import b1 = M.I; // no error
import d1 = c1.B; // error
import e1 = c1.I; // no error
import f1 = c1; // error
import c1 = M; // no error
import g1 = c1.B; // no error
import h1 = c1.I; // no error
import i1 = c1; // no error

//// [aliasOutOfOrder.js]
var __extends = (this && this.__extends) || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var a = M.B; // error
var d = c.B; // error
var f = c; // error
var c = M; // error
var g = c.B; // no error
var i = c; // no error
var C = (function (_super) {
__extends(C, _super);
function C() {
_super.apply(this, arguments);
}
return C;
})(a);
var D = (function (_super) {
__extends(D, _super);
function D() {
_super.apply(this, arguments);
}
return D;
})(M.B);
var M;
(function (M) {
var B = (function () {
function B() {
}
return B;
})();
M.B = B;
})(M || (M = {}));
var a1 = M.B; // no error
var d1 = c1.B; // error
var f1 = c1; // error
var c1 = M; // no error
var g1 = c1.B; // no error
var i1 = c1; // no error
24 changes: 24 additions & 0 deletions tests/baselines/reference/baseClassExpressionOutOfOrder.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
tests/cases/compiler/baseClassExpressionOutOfOrder.ts(1,25): error TS2653: Base expression references type before it is declared.
tests/cases/compiler/baseClassExpressionOutOfOrder.ts(3,24): error TS2653: Base expression references type before it is declared.


==== tests/cases/compiler/baseClassExpressionOutOfOrder.ts (2 errors) ====
var c = class A extends B { // error
~
!!! error TS2653: Base expression references type before it is declared.
}
var c2 = class extends B { // error
~
!!! error TS2653: Base expression references type before it is declared.
}
var c3 = class B extends class A { // No error
}{
}
var c4 =class extends class { // no error
}{
}
var c5 = class extends class B3 extends class C { // no error
}{
}{ }
class B {
}
81 changes: 81 additions & 0 deletions tests/baselines/reference/baseClassExpressionOutOfOrder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
//// [baseClassExpressionOutOfOrder.ts]
var c = class A extends B { // error
}
var c2 = class extends B { // error
}
var c3 = class B extends class A { // No error
}{
}
var c4 =class extends class { // no error
}{
}
var c5 = class extends class B3 extends class C { // no error
}{
}{ }
class B {
}

//// [baseClassExpressionOutOfOrder.js]
var __extends = (this && this.__extends) || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var c = (function (_super) {
__extends(A, _super);
function A() {
_super.apply(this, arguments);
}
return A;
})(B);
var c2 = (function (_super) {
__extends(class_1, _super);
function class_1() {
_super.apply(this, arguments);
}
return class_1;
})(B);
var c3 = (function (_super) {
__extends(B, _super);
function B() {
_super.apply(this, arguments);
}
return B;
})((function () {
function A() {
}
return A;
})());
var c4 = (function (_super) {
__extends(class_2, _super);
function class_2() {
_super.apply(this, arguments);
}
return class_2;
})((function () {
function class_3() {
}
return class_3;
})());
var c5 = (function (_super) {
__extends(class_4, _super);
function class_4() {
_super.apply(this, arguments);
}
return class_4;
})((function (_super) {
__extends(B3, _super);
function B3() {
_super.apply(this, arguments);
}
return B3;
})((function () {
function C() {
}
return C;
})()));
var B = (function () {
function B() {
}
return B;
})();
Loading