Skip to content

Alternative proposal for grouping exported and local declarations #197

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.

Sign up for GitHub

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 7 commits into from
Jul 28, 2014
Merged

Alternative proposal for grouping exported and local declarations #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d165708
check groups of local symbols
vladima Jul 22, 2014
a490eb2
Merge pull request #217 from Microsoft/strict_mode
vladima Jul 24, 2014
d7f6708
check groups of local symbols
vladima Jul 22, 2014
bb193fe
augment check for colliding declaration spaces
vladima Jul 24, 2014
8d523f9
merged with upstream
vladima Jul 25, 2014
81da2cb
update comments
vladima Jul 25, 2014
42df260
use getEffectiveDeclarationFlags to get Export flag
vladima Jul 25, 2014
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
9 changes: 3 additions & 6 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,30 +136,27 @@ module ts {
// 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.
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);
var local = declareSymbol(container.locals, undefined, node, exportKind, symbolExcludes);
local.exportSymbol = declareSymbol(container.symbol.exports, container.symbol, node, symbolKind, symbolExcludes);
node.localSymbol = local;
}
else {
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
132 changes: 109 additions & 23 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4793,24 +4793,21 @@ module ts {
error(signatureDeclarationNode, Diagnostics.Specialized_overload_signature_is_not_assignable_to_any_non_specialized_signature);
}

function checkFunctionOrConstructorSymbol(symbol: Symbol) {
function getEffectiveFlagsForFunctionCheck(n: Node) {
var flags = n.flags;
// We want to determine if an overload is effectively ambient, which can happen if it
// is nested in an ambient context. However, do not treat members of interfaces differently
// based on whether the interface itself is in an ambient context. Interfaces should never
// be considered ambient for purposes of comparing overload attributes.
if (n.parent.kind !== SyntaxKind.InterfaceDeclaration && isInAmbientContext(n)) {
if (!(flags & NodeFlags.Ambient)) {
// It is nested in an ambient context, which means it is automatically exported
flags |= NodeFlags.Export;
}
flags |= NodeFlags.Ambient;
function getEffectiveDeclarationFlags(n: Node, flagsToCheck: NodeFlags) {
var flags = n.flags;
if (n.parent.kind !== SyntaxKind.InterfaceDeclaration && isInAmbientContext(n)) {
if (!(flags & NodeFlags.Ambient)) {
// It is nested in an ambient context, which means it is automatically exported
flags |= NodeFlags.Export;
}

return flags & flagsToCheck;
flags |= NodeFlags.Ambient;
}

return flags & flagsToCheck;
}

function checkFunctionOrConstructorSymbol(symbol: Symbol) {

function checkFlagAgreementBetweenOverloads(overloads: Declaration[], implementation: FunctionDeclaration, flagsToCheck: NodeFlags, someOverloadFlags: NodeFlags, allOverloadFlags: NodeFlags): void {
// Error if some overloads have a flag that is not shared by all overloads. To find the
// deviations, we XOR someOverloadFlags with allOverloadFlags
Expand All @@ -4823,10 +4820,10 @@ module ts {
// the canonical signature only if it is in the same container as the first overload
var implementationSharesContainerWithFirstOverload = implementation !== undefined && implementation.parent === overloads[0].parent;
var canonicalFlags = implementationSharesContainerWithFirstOverload
? getEffectiveFlagsForFunctionCheck(implementation)
: getEffectiveFlagsForFunctionCheck(overloads[0]);
? getEffectiveDeclarationFlags(implementation, flagsToCheck)
: getEffectiveDeclarationFlags(overloads[0], flagsToCheck);
forEach(overloads, o => {
var deviation = getEffectiveFlagsForFunctionCheck(o) ^ canonicalFlags;
var deviation = getEffectiveDeclarationFlags(o, flagsToCheck) ^ canonicalFlags;
if (deviation & NodeFlags.Export) {
error(o.name, Diagnostics.Overload_signatures_must_all_be_exported_or_not_exported);
}
Expand Down Expand Up @@ -4854,7 +4851,7 @@ module ts {
for (var i = 0; i < declarations.length; i++) {
var node = <FunctionDeclaration>declarations[i];
if (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.Method || node.kind === SyntaxKind.Constructor) {
var currentNodeFlags = getEffectiveFlagsForFunctionCheck(node);
var currentNodeFlags = getEffectiveDeclarationFlags(node, flagsToCheck);
someNodeFlags |= currentNodeFlags;
allNodeFlags &= currentNodeFlags;

Expand Down Expand Up @@ -4925,14 +4922,97 @@ module ts {
}
}

function checkExportsOnMergedDeclarations(node: Node) {
var symbol: Symbol;

// Exports should be checked only if enclosing module contains both exported and non exported declarations.
// In case if all declarations are non-exported check is unnecesary.

// if localSymbol is defined on node then node itself is exported - check is required
var symbol = node.localSymbol;
if (!symbol) {
// local symbol is undefined => this declaration is non-exported.
// however symbol might contain other declarations that are exported
symbol = getSymbolOfNode(node);
if (!(symbol.flags & SymbolFlags.Export)) {
// this is a pure local symbol (all declarations are non-exported) - no need to check anything
return;
}
}

// run the check only for the first declaration in the list
if (getDeclarationOfKind(symbol, node.kind) !== node) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment to explain what is going on here. From what I can understand

// Get the appropriate declaration for the local symbol
// (e.g. if we are checking a function, get the function declaration).
// If this is not the root of the export, then we have no reason to check.

could work.

return;
}

// we use SymbolFlags.ExportValue, SymbolFlags.ExportType and SymbolFlags.ExportNamespace
// to denote disjoint declarationSpaces (without making new enum type).
var exportedDeclarationSpaces: SymbolFlags = 0;
var nonExportedDeclarationSpaces: SymbolFlags = 0;
forEach(symbol.declarations, d => {
var declarationSpaces = getDeclarationSpaces(d);
if (getEffectiveDeclarationFlags(d, NodeFlags.Export)) {
exportedDeclarationSpaces |= declarationSpaces;
}
else {
nonExportedDeclarationSpaces |= declarationSpaces;
}
});

var commonDeclarationSpace = exportedDeclarationSpaces & nonExportedDeclarationSpaces

if (commonDeclarationSpace) {
// declaration spaces for exported and non-exported declarations intersect
forEach(symbol.declarations, d => {
if (getDeclarationSpaces(d) & commonDeclarationSpace) {
error(d.name, Diagnostics.Individual_declarations_in_merged_declaration_0_must_be_all_exported_or_all_local, identifierToString(d.name));
}
});
}

function getDeclarationSpaces(d: Declaration): SymbolFlags {
switch (d.kind) {
case SyntaxKind.InterfaceDeclaration:
return SymbolFlags.ExportType;
case SyntaxKind.ModuleDeclaration:
return (<ModuleDeclaration>d).name.kind === SyntaxKind.StringLiteral || isInstantiated(d)
? SymbolFlags.ExportNamespace | SymbolFlags.ExportValue
: SymbolFlags.ExportNamespace;
case SyntaxKind.ClassDeclaration:
case SyntaxKind.EnumDeclaration:
return SymbolFlags.ExportType | SymbolFlags.ExportValue;
case SyntaxKind.ImportDeclaration:
var result: SymbolFlags = 0;
var target = resolveImport(getSymbolOfNode(d));
forEach(target.declarations, d => { result |= getDeclarationSpaces(d); } )
return result;
default:
return SymbolFlags.ExportValue;
}
}
}

function checkFunctionDeclaration(node: FunctionDeclaration) {
checkSignatureDeclaration(node);

var symbol = getSymbolOfNode(node);
var firstDeclaration = getDeclarationOfKind(symbol, node.kind);
var symbol = getSymbolOfNode(node)
// first we want to check the local symbol that contain this declaration
// - if node.localSymbol !== undefined - this is current declaration is exported and localSymbol points to the local symbol
// - if node.localSymbol === undefined - this node is non-exported so we can just pick the result of getSymbolOfNode
var localSymbol = node.localSymbol || symbol;

var firstDeclaration = getDeclarationOfKind(localSymbol, node.kind);
// Only type check the symbol once
if (node === firstDeclaration) {
checkFunctionOrConstructorSymbol(symbol);
checkFunctionOrConstructorSymbol(localSymbol);
}

if (symbol.parent) {
// run check once for the first declaration
if (getDeclarationOfKind(symbol, node.kind) === node) {
// run check on export symbol to check that modifiers agree across all exported declarations
checkFunctionOrConstructorSymbol(symbol);
}
}

checkSourceElement(node.body);
Expand Down Expand Up @@ -5129,8 +5209,10 @@ module ts {
function checkVariableDeclaration(node: VariableDeclaration) {
checkSourceElement(node.type);

var symbol = getSymbolOfNode(node);
checkExportsOnMergedDeclarations(node);

var symbol = getSymbolOfNode(node);

var typeOfValueDeclaration = getTypeOfVariableOrParameterOrProperty(symbol);
var type: Type;
var useTypeFromValueDeclaration = node === symbol.valueDeclaration;
Expand Down Expand Up @@ -5418,6 +5500,7 @@ module ts {
checkTypeNameIsReserved(node.name, Diagnostics.Class_name_cannot_be_0);
checkTypeParameters(node.typeParameters);
checkCollisionWithCapturedThisVariable(node, node.name);
checkExportsOnMergedDeclarations(node);
var symbol = getSymbolOfNode(node);
var type = <InterfaceType>getDeclaredTypeOfSymbol(symbol);
var staticType = <ObjectType>getTypeOfSymbol(symbol);
Expand Down Expand Up @@ -5572,6 +5655,7 @@ module ts {
function checkInterfaceDeclaration(node: InterfaceDeclaration) {
checkTypeNameIsReserved(node.name, Diagnostics.Interface_name_cannot_be_0);
checkTypeParameters(node.typeParameters);
checkExportsOnMergedDeclarations(node);
var symbol = getSymbolOfNode(node);
var firstInterfaceDecl = <InterfaceDeclaration>getDeclarationOfKind(symbol, SyntaxKind.InterfaceDeclaration);
if (symbol.declarations.length > 1) {
Expand Down Expand Up @@ -5617,6 +5701,7 @@ module ts {
function checkEnumDeclaration(node: EnumDeclaration) {
checkTypeNameIsReserved(node.name, Diagnostics.Enum_name_cannot_be_0);
checkCollisionWithCapturedThisVariable(node, node.name);
checkExportsOnMergedDeclarations(node);
var enumSymbol = getSymbolOfNode(node);
var enumType = getDeclaredTypeOfSymbol(enumSymbol);
var autoValue = 0;
Expand Down Expand Up @@ -5688,6 +5773,7 @@ module ts {

function checkModuleDeclaration(node: ModuleDeclaration) {
checkCollisionWithCapturedThisVariable(node, node.name);
checkExportsOnMergedDeclarations(node);
var symbol = getSymbolOfNode(node);
if (symbol.flags & SymbolFlags.ValueModule && symbol.declarations.length > 1 && !isInAmbientContext(node)) {
var classOrFunc = getFirstNonAmbientClassOrFunctionDeclaration(symbol);
Expand Down
1 change: 1 addition & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,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." },
Individual_declarations_in_merged_declaration_0_must_be_all_exported_or_all_local: { code: 2192, category: DiagnosticCategory.Error, key: "Individual declarations in merged declaration {0} must be all exported or all local." },
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,10 @@
"category": "Error",
"code": 2190
},
"Individual declarations in merged declaration {0} must be all exported or all local.": {
"category": "Error",
"code": 2192
},
"'super' cannot be referenced in constructor arguments.":{
"category": "Error",
"code": 2193
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ module ts {
symbol?: Symbol; // Symbol declared by node (initialized by binding)
locals?: SymbolTable; // Locals associated with node (initialized by binding)
nextContainer?: Node; // Next container in declaration order (initialized by binding)
localSymbol?: Symbol; // Local symbol declared by node (initialized by binding only for exported nodes)
}

export interface NodeArray<T> extends Array<T>, TextRange { }
Expand Down Expand Up @@ -699,6 +700,7 @@ module ts {

IsContainer = HasLocals | HasExports | HasMembers,
PropertyOrAccessor = Property | Accessor,
Export = ExportNamespace | ExportType | ExportValue,
}

export interface Symbol {
Expand Down
6 changes: 4 additions & 2 deletions tests/baselines/reference/anonymousModules.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
==== tests/cases/compiler/anonymousModules.ts (12 errors) ====
==== tests/cases/compiler/anonymousModules.ts (13 errors) ====
module {
~
!!! ';' expected.
Expand All @@ -18,13 +18,15 @@
export var bar = 1;
~~~~~~
!!! Statement expected.
~~~
!!! Individual declarations in merged declaration bar must be all exported or all local.
}
~
!!! Declaration or statement expected.

var bar = 2;
~~~
!!! Duplicate identifier 'bar'.
!!! Individual declarations in merged declaration bar must be all exported or all local.

module {
~
Expand Down
Loading