Skip to content

exported declarations now don't take the exclusive slot #37

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 5 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
33 changes: 20 additions & 13 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,40 +125,47 @@ module ts {
function declareModuleMember(node: Declaration, symbolKind: SymbolFlags, symbolExcludes: SymbolFlags) {
// Exported module members are given 2 symbols: A local symbol that is classified with an ExportValue,
// ExportType, or ExportContainer flag, and an associated export symbol with all the correct flags set
// on it. There are 2 main reasons:
//
// 1. We treat locals and exports of the same name as mutually exclusive within a container.
// That means the binder will issue a Duplicate Identifier error if you mix locals and exports
// with the same name in the same container.
// TODO: Make this a more specific error and decouple it from the exclusion logic.
// on it. Reasons:
// 1. During name resolution i.e for types we need to distingush local and exported symbols.
// All members are added to the exported one so local should never be returned.
// 2. When we checkIdentifier in the checker, we set its resolved symbol to the local symbol,
// but return the export symbol (by calling getExportSymbolOfValueSymbolIfExported). That way
// when the emitter comes back to it, it knows not to qualify the name if it was found in a containing scope.
// Exported members will share local symbol with non-exported members
var exportKind = 0;
var exportExcludes = 0;
if (symbolKind & SymbolFlags.Value) {
exportKind |= SymbolFlags.ExportValue;
exportExcludes |= SymbolFlags.Value;
}
if (symbolKind & SymbolFlags.Type) {
exportKind |= SymbolFlags.ExportType;
exportExcludes |= SymbolFlags.Type;
}
if (symbolKind & SymbolFlags.Namespace) {
exportKind |= SymbolFlags.ExportNamespace;
exportExcludes |= SymbolFlags.Namespace;
}
if (node.flags & NodeFlags.Export || (node.kind !== SyntaxKind.ImportDeclaration && isAmbientContext(container))) {
if (exportKind) {
var local = declareSymbol(container.locals, undefined, node, exportKind, exportExcludes);
local.exportSymbol = declareSymbol(container.symbol.exports, container.symbol, node, symbolKind, symbolExcludes);
var exportSymbol = declareSymbol(container.symbol.exports, container.symbol, node, symbolKind, symbolExcludes);
var localSymbol = declareSymbol(container.locals, undefined, node, exportKind, symbolExcludes);
node.symbol = exportSymbol;
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 you don't need to set the symbol explicitly if you swap the order of the previous two lines

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to be explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

// export function foo(); // 1
// export function foo() {} // 2
// at (1) we'll create two symbols: local and exported and link then together
// at (2) the same symbols will be returned as exported and local.
// make sure that local symbol is added to the list of local symbols just once.
if (!localSymbol.exportSymbol) {
if (!exportSymbol.localSymbols) {
exportSymbol.localSymbols = [];
}
exportSymbol.localSymbols.push(localSymbol);
localSymbol.exportSymbol = exportSymbol;
}
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this else block for?

Copy link
Member

Choose a reason for hiding this comment

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

It is for import aliases. An exported import alias doesn't have a local symbol.

declareSymbol(container.symbol.exports, container.symbol, node, symbolKind, symbolExcludes);
}
}
else {
declareSymbol(container.locals, undefined, node, symbolKind, symbolExcludes | exportKind);
declareSymbol(container.locals, undefined, node, symbolKind, symbolExcludes);
}
}

Expand Down
320 changes: 231 additions & 89 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ module ts {
A_signature_with_an_implementation_cannot_use_a_string_literal_type: { code: 2163, category: DiagnosticCategory.Error, key: "A signature with an implementation cannot use a string literal type." },
Interface_0_cannot_simultaneously_extend_types_1_and_2_Colon: { code: 2189, category: DiagnosticCategory.Error, key: "Interface '{0}' cannot simultaneously extend types '{1}' and '{2}':" },
Initializer_of_parameter_0_cannot_reference_identifier_1_declared_after_it: { code: 2190, category: DiagnosticCategory.Error, key: "Initializer of parameter '{0}' cannot reference identifier '{1}' declared after it." },
All_declarations_of_merged_declaration_0_must_be_exported_or_not_exported: { code: 2192, category: DiagnosticCategory.Error, key: "All declarations of merged declaration '{0}' must be exported or not exported." },
super_cannot_be_referenced_in_constructor_arguments: { code: 2193, category: DiagnosticCategory.Error, key: "'super' cannot be referenced in constructor arguments." },
Return_type_of_constructor_signature_must_be_assignable_to_the_instance_type_of_the_class: { code: 2194, category: DiagnosticCategory.Error, key: "Return type of constructor signature must be assignable to the instance type of the class" },
Ambient_external_module_declaration_cannot_specify_relative_module_name: { code: 2196, category: DiagnosticCategory.Error, key: "Ambient external module declaration cannot specify relative module name." },
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,10 @@
"category": "Error",
"code": 2190
},
"All declarations of merged declaration '{0}' must be exported or not exported.": {
"category": "Error",
"code": 2192
},
"'super' cannot be referenced in constructor arguments.":{
"category": "Error",
"code": 2193
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,6 @@ module ts {
IndexSignature = 0x00020000, // Index signature
TypeParameter = 0x00040000, // Type parameter

// Export markers (see comment in declareModuleMember in binder)
ExportValue = 0x00080000, // Exported value marker
ExportType = 0x00100000, // Exported type marker
ExportNamespace = 0x00200000, // Exported namespace marker
Expand Down Expand Up @@ -707,7 +706,8 @@ module ts {
exports?: SymbolTable; // Module exports
exportSymbol?: Symbol; // Exported symbol associated with this symbol
exportAssignSymbol?: Symbol; // Symbol exported from external module
valueDeclaration?: Declaration // First value declaration of the symbol
valueDeclaration?: Declaration;// First value declaration of the symbol
localSymbols?: Symbol[]; // List of local symbols that contribute to the export symbol
}

export interface SymbolLinks {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/anonymousModules.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

var bar = 2;
~~~
!!! Duplicate identifier 'bar'.
!!! All declarations of merged declaration 'bar' must be exported or not exported.

module {
~
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@
interface I { }
export interface I { } // error
~
!!! Duplicate identifier 'I'.
!!! All declarations of merged declaration 'I' must be exported or not exported.
export interface E { }
interface E { } // error
~
!!! Duplicate identifier 'E'.
!!! All declarations of merged declaration 'E' must be exported or not exported.
}

// Should report error only once for instantiated module
Expand All @@ -39,7 +39,7 @@
}
export module inst { // one error
~~~~
!!! Duplicate identifier 'inst'.
!!! All declarations of merged declaration 'inst' must be exported or not exported.
var t;
}
}
Expand All @@ -49,28 +49,28 @@
var v: string;
export var v: string; // one error (visibility)
~
!!! Duplicate identifier 'v'.
!!! All declarations of merged declaration 'v' must be exported or not exported.
var w: number;
export var w: string; // two errors (visibility and type mismatch)
~
!!! Duplicate identifier 'w'.
!!! All declarations of merged declaration 'w' must be exported or not exported.
}

module M {
module F {
~
!!! A module declaration cannot be located prior to a class or function with which it is merged
var t;
}
export function F() { } // Only one error for duplicate identifier (don't consider visibility)
~
!!! Duplicate identifier 'F'.
}

module M {
class C { }
module C { }
export module C { // Two visibility errors (one for the clodule symbol, and one for the merged container symbol)
~
!!! Duplicate identifier 'C'.
!!! All declarations of merged declaration 'C' must be exported or not exported.
var t;
}
}
Expand All @@ -79,4 +79,4 @@
interface D { }
export interface D { }
~
!!! Duplicate identifier 'D'.
!!! All declarations of merged declaration 'D' must be exported or not exported.
20 changes: 5 additions & 15 deletions tests/baselines/reference/functionOverloadErrors.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
==== tests/cases/conformance/functions/functionOverloadErrors.ts (19 errors) ====
==== tests/cases/conformance/functions/functionOverloadErrors.ts (14 errors) ====
//Function overload signature with initializer
function fn1(x = 3);
~~~~~
Expand Down Expand Up @@ -86,26 +86,16 @@
//Function overloads with differing export
module M {
export function fn1();
~~~~~~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Overload signatures must all be exported or not exported.
function fn1(n: string);
~~~~~~~~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Duplicate identifier 'fn1'.
function fn1() { }
~~~
!!! Duplicate identifier 'fn1'.

function fn2(n: string);
~~~~~~~~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Overload signatures must all be exported or not exported.
export function fn2();
~~~
!!! Duplicate identifier 'fn2'.
export function fn2() { }
~~~
!!! Duplicate identifier 'fn2'.
}

//Function overloads with differing ambience
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/multivar.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
declare var d1, d2;
var b2;
~~
!!! Duplicate identifier 'b2'.
!!! All declarations of merged declaration 'b2' must be exported or not exported.

declare var v1;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
==== tests/cases/compiler/overloadModifiersMustAgree.ts (5 errors) ====
==== tests/cases/compiler/overloadModifiersMustAgree.ts (4 errors) ====
class baz {
public foo();
~~~
Expand All @@ -10,10 +10,8 @@
~~~
!!! Overload signatures must all be ambient or non-ambient.
export function bar(s: string);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
~~~
!!! Duplicate identifier 'bar'.
!!! Overload signatures must all be exported or not exported.
function bar(s?: string) { }

interface I {
Expand Down