Skip to content

Disallow statements between overloads #317

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

Merged
merged 2 commits into from
Jul 31, 2014
Merged
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
96 changes: 75 additions & 21 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4872,47 +4872,101 @@ module ts {
var hasOverloads = false;
var bodyDeclaration: FunctionDeclaration;
var lastSeenNonAmbientDeclaration: FunctionDeclaration;
var previousDeclaration: FunctionDeclaration;

var declarations = symbol.declarations;
var isConstructor = (symbol.flags & SymbolFlags.Constructor) !== 0;

function reportImplementationExpectedError(node: FunctionDeclaration): void {
var seen = false;
var subsequentNode = forEachChild(node.parent, c => {
if (seen) {
return c;
}
else {
seen = c === node;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't make a difference, but my preference is

if (c === node) {
    seen = true;
}

Feel free to keep it the same.

}
});
if (subsequentNode) {
if (subsequentNode.kind === node.kind) {
var errorNode: Node = (<FunctionDeclaration>subsequentNode).name || subsequentNode;
if (node.name && (<FunctionDeclaration>subsequentNode).name && node.name.text === (<FunctionDeclaration>subsequentNode).name.text) {
// the only situation when this is possible (same kind\same name but different symbol) - mixed static and instance class members
Debug.assert(node.kind === SyntaxKind.Method);
Debug.assert((node.flags & NodeFlags.Static) !== (subsequentNode.flags & NodeFlags.Static));
var diagnostic = node.flags & NodeFlags.Static ? Diagnostics.Function_overload_must_be_static : Diagnostics.Function_overload_must_not_be_static;
error(errorNode, diagnostic);
return;
}
else if ((<FunctionDeclaration>subsequentNode).body) {
error(errorNode, Diagnostics.Function_implementation_name_must_be_0, identifierToString(node.name));
return;
}
}
}
var errorNode: Node = node.name || node;
if (isConstructor) {
error(errorNode, Diagnostics.Constructor_implementation_is_missing);
}
else {
error(errorNode, Diagnostics.Function_implementation_is_missing_or_not_immediately_following_the_declaration);
}
}

// when checking exported function declarations across modules check only duplicate implementations
// names and consistensy of modifiers are verified when we check local symbol
var isExportSymbolInsideModule = symbol.parent && symbol.parent.flags & SymbolFlags.Module;
for (var i = 0; i < declarations.length; i++) {
var node = <FunctionDeclaration>declarations[i];
var inAmbientContext = isInAmbientContext(node);
var inAmbientContextOrInterface = node.parent.kind === SyntaxKind.InterfaceDeclaration || node.parent.kind === SyntaxKind.TypeLiteral || inAmbientContext;
if (inAmbientContextOrInterface) {
// check if declarations are consecutive only if they are non-ambient
// 1. ambient declarations can be interleaved
// i.e. this is legal
// declare function foo();
// declare function bar();
// declare function foo();
// 2. mixing ambient and non-ambient declarations is a separate error that will be reported - do not want to report an extra one
previousDeclaration = undefined;
}

if (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.Method || node.kind === SyntaxKind.Constructor) {
var currentNodeFlags = getEffectiveDeclarationFlags(node, flagsToCheck);
someNodeFlags |= currentNodeFlags;
allNodeFlags &= currentNodeFlags;

var inAmbientContext = isInAmbientContext(node);
var inAmbientContextOrInterface = node.parent.kind === SyntaxKind.InterfaceDeclaration || node.parent.kind === SyntaxKind.TypeLiteral || inAmbientContext;
if (!inAmbientContextOrInterface) {
lastSeenNonAmbientDeclaration = node;
if (node.body && bodyDeclaration) {
if (isConstructor) {
error(node, Diagnostics.Multiple_constructor_implementations_are_not_allowed);
}
else {
error(node, Diagnostics.Duplicate_function_implementation);
}
}
else if (!isExportSymbolInsideModule && previousDeclaration && previousDeclaration.parent === node.parent && previousDeclaration.end !== node.pos) {
reportImplementationExpectedError(previousDeclaration);
}

if (node.body) {
if (bodyDeclaration) {
if (isConstructor) {
error(node, Diagnostics.Multiple_constructor_implementations_are_not_allowed);
}
else {
error(node, Diagnostics.Duplicate_function_implementation);
}
}
else {
if (!bodyDeclaration) {
bodyDeclaration = node;
}
}
else {
hasOverloads = true;
}

previousDeclaration = node;

if (!inAmbientContextOrInterface) {
lastSeenNonAmbientDeclaration = node;
}
}
}

if (lastSeenNonAmbientDeclaration && !lastSeenNonAmbientDeclaration.body) {
if (isConstructor) {
error(lastSeenNonAmbientDeclaration, Diagnostics.Constructor_implementation_expected);
}
else {
error(lastSeenNonAmbientDeclaration, Diagnostics.Function_implementation_expected);
}
if (!isExportSymbolInsideModule && lastSeenNonAmbientDeclaration && !lastSeenNonAmbientDeclaration.body) {
reportImplementationExpectedError(lastSeenNonAmbientDeclaration);
}

if (hasOverloads) {
Expand Down Expand Up @@ -4985,7 +5039,7 @@ module ts {
}
});

var commonDeclarationSpace = exportedDeclarationSpaces & nonExportedDeclarationSpaces
var commonDeclarationSpace = exportedDeclarationSpaces & nonExportedDeclarationSpaces;

if (commonDeclarationSpace) {
// declaration spaces for exported and non-exported declarations intersect
Expand Down
7 changes: 5 additions & 2 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,12 @@ module ts {
Duplicate_number_index_signature: { code: 2233, category: DiagnosticCategory.Error, key: "Duplicate number index signature." },
All_declarations_of_an_interface_must_have_identical_type_parameters: { code: 2234, category: DiagnosticCategory.Error, key: "All declarations of an interface must have identical type parameters." },
Expression_resolves_to_variable_declaration_i_that_compiler_uses_to_initialize_rest_parameter: { code: 2235, category: DiagnosticCategory.Error, key: "Expression resolves to variable declaration '_i' that compiler uses to initialize rest parameter." },
Constructor_implementation_expected: { code: 2240, category: DiagnosticCategory.Error, key: "Constructor implementation expected." },
Function_implementation_name_must_be_0: { code: 2239, category: DiagnosticCategory.Error, key: "Function implementation name must be '{0}'." },
Constructor_implementation_is_missing: { code: 2240, category: DiagnosticCategory.Error, key: "Constructor implementation is missing." },
An_export_assignment_cannot_be_used_in_a_module_with_other_exported_elements: { code: 2245, category: DiagnosticCategory.Error, key: "An export assignment cannot be used in a module with other exported elements." },
A_parameter_property_is_only_allowed_in_a_constructor_implementation: { code: 2246, category: DiagnosticCategory.Error, key: "A parameter property is only allowed in a constructor implementation." },
Function_overload_must_be_static: { code: 2247, category: DiagnosticCategory.Error, key: "Function overload must be static." },
Function_overload_must_not_be_static: { code: 2248, category: DiagnosticCategory.Error, key: "Function overload must not be static." },
Circular_definition_of_import_alias_0: { code: 3000, category: DiagnosticCategory.Error, key: "Circular definition of import alias '{0}'." },
Cannot_find_name_0: { code: 3001, category: DiagnosticCategory.Error, key: "Cannot find name '{0}'." },
Module_0_has_no_exported_member_1: { code: 3002, category: DiagnosticCategory.Error, key: "Module '{0}' has no exported member '{1}'." },
Expand Down Expand Up @@ -276,7 +279,7 @@ module ts {
Types_of_parameters_0_and_1_are_incompatible_Colon: { code: -9999999, category: DiagnosticCategory.Error, key: "Types of parameters '{0}' and '{1}' are incompatible:" },
Unknown_identifier_0: { code: -9999999, category: DiagnosticCategory.Error, key: "Unknown identifier '{0}'." },
Property_0_is_inaccessible: { code: -9999999, category: DiagnosticCategory.Error, key: "Property '{0}' is inaccessible." },
Function_implementation_expected: { code: -9999999, category: DiagnosticCategory.Error, key: "Function implementation expected." },
Function_implementation_is_missing_or_not_immediately_following_the_declaration: { code: -9999999, category: DiagnosticCategory.Error, key: "Function implementation is missing or not immediately following the declaration." },
Property_0_of_type_1_is_not_assignable_to_string_index_type_2: { code: -9999999, category: DiagnosticCategory.Error, key: "Property '{0}' of type '{1}' is not assignable to string index type '{2}'." },
Property_0_of_type_1_is_not_assignable_to_numeric_index_type_2: { code: -9999999, category: DiagnosticCategory.Error, key: "Property '{0}' of type '{1}' is not assignable to numeric index type '{2}'." },
Numeric_index_type_0_is_not_assignable_to_string_index_type_1: { code: -9999999, category: DiagnosticCategory.Error, key: "Numeric index type '{0}' is not assignable to string index type '{1}'." },
Expand Down
19 changes: 15 additions & 4 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -623,8 +623,12 @@
"Expression resolves to variable declaration '_i' that compiler uses to initialize rest parameter.": {
"category": "Error",
"code": 2235
},
"Constructor implementation expected.": {
},
"Function implementation name must be '{0}'.": {
"category": "Error",
"code": 2239
},
"Constructor implementation is missing.": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error message was changed from "Constructor implementation expected." to "Constructor implementation is missing.". Given that error is usually reported on the line that contains the last overload wording "Constructor implementation expected." is not clear enough to explain what went wrong (should I create implementation instead of this overloads? But what if I need this overloads? etc...). In opposite "Constructor implementation is missing." implies that this overload itself is ok but it lacks implementation and thus error is reported

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems too generic, the problem may not be a missing implementation per se but an implementation that is separated from the declaration.. so something like "Function implementation is missing or not immediately following the declaration", or "Function implementation is expected to follow the declaration" .. etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, makes sense. I'll change it to "Function implementation is missing or not immediately following the declaration"

"category": "Error",
"code": 2240
},
Expand All @@ -636,6 +640,14 @@
"category": "Error",
"code": 2246
},
"Function overload must be static.": {
"category": "Error",
"code": 2247
},
"Function overload must not be static.": {
Copy link
Member

Choose a reason for hiding this comment

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

Also possible: "Function overload cannot be static" or "Function overload must be non-static." - I'm a bigger fan of the first.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for "Function overload cannot be static"

"category": "Error",
"code": 2248
},
"Circular definition of import alias '{0}'.": {
"category": "Error",
"code": 3000
Expand Down Expand Up @@ -900,7 +912,6 @@
"category": "Error",
"code": 7020
},

"Variable declaration list cannot be empty.": {
"category": "Error",
"code": -9999999
Expand Down Expand Up @@ -1121,7 +1132,7 @@
"category": "Error",
"code": -9999999
},
"Function implementation expected.": {
"Function implementation is missing or not immediately following the declaration.": {
"category": "Error",
"code": -9999999
},
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/ClassDeclaration10.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
class C {
constructor();
~~~~~~~~~~~~~~
!!! Constructor implementation expected.
!!! Constructor implementation is missing.
foo();
~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
}
2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration11.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
class C {
constructor();
~~~~~~~~~~~~~~
!!! Constructor implementation expected.
!!! Constructor implementation is missing.
foo() { }
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/ClassDeclaration13.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/ClassDeclaration13.ts (1 errors) ====
class C {
foo();
~~~~~~
!!! Function implementation expected.
bar() { }
~~~
!!! Function implementation name must be 'foo'.
}
6 changes: 3 additions & 3 deletions tests/baselines/reference/ClassDeclaration14.errors.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
==== tests/cases/compiler/ClassDeclaration14.ts (2 errors) ====
class C {
foo();
~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
constructor();
~~~~~~~~~~~~~~
!!! Constructor implementation expected.
!!! Constructor implementation is missing.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/ClassDeclaration15.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/ClassDeclaration15.ts (1 errors) ====
class C {
foo();
~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
constructor() { }
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/ClassDeclaration21.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/ClassDeclaration21.ts (1 errors) ====
class C {
0();
~~~~
!!! Function implementation expected.
1() { }
~
!!! Function implementation name must be '0'.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/ClassDeclaration22.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/ClassDeclaration22.ts (1 errors) ====
class C {
"foo"();
~~~~~~~~
!!! Function implementation expected.
"bar"() { }
~~~~~
!!! Function implementation name must be '"foo"'.
}
8 changes: 4 additions & 4 deletions tests/baselines/reference/ClassDeclaration25.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
}
class List<U> implements IList<U> {
data(): U;
~~~~~~~~~~
!!! Function implementation expected.
~~~~
!!! Function implementation is missing or not immediately following the declaration.
next(): string;
~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~~
!!! Function implementation is missing or not immediately following the declaration.
}

2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration8.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
class C {
constructor();
~~~~~~~~~~~~~~
!!! Constructor implementation expected.
!!! Constructor implementation is missing.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/ClassDeclaration9.errors.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
==== tests/cases/compiler/ClassDeclaration9.ts (1 errors) ====
class C {
foo();
~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/FunctionDeclaration3.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
==== tests/cases/compiler/FunctionDeclaration3.ts (1 errors) ====
function foo();
~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
6 changes: 3 additions & 3 deletions tests/baselines/reference/FunctionDeclaration4.errors.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
==== tests/cases/compiler/FunctionDeclaration4.ts (1 errors) ====
function foo();
~~~~~~~~~~~~~~~
!!! Function implementation expected.
function bar() { }
function bar() { }
~~~
!!! Function implementation name must be 'foo'.
4 changes: 2 additions & 2 deletions tests/baselines/reference/FunctionDeclaration6.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/FunctionDeclaration6.ts (1 errors) ====
{
function foo();
~~~~~~~~~~~~~~~
!!! Function implementation expected.
function bar() { }
~~~
!!! Function implementation name must be 'foo'.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/FunctionDeclaration7.errors.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
==== tests/cases/compiler/FunctionDeclaration7.ts (1 errors) ====
module M {
function foo();
~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
==== tests/cases/compiler/assignmentCompatFunctionsWithOptionalArgs.ts (3 errors) ====
function foo(x: { id: number; name?: string; }): void;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
foo({ id: 1234 }); // Ok
foo({ id: 1234, name: "hello" }); // Ok
foo({ id: 1234, name: false }); // Error, name of wrong type
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/callOverloads1.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
}

function Foo(); // error
~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Function implementation is missing or not immediately following the declaration.
~~~
!!! Duplicate identifier 'Foo'.
function F1(s:string);
Expand Down
Loading