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

Merged Declarations for Classes and Interfaces #3333

Merged
merged 37 commits into from
Jul 2, 2015
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
813d227
classInterface: changed excludes flags
May 27, 2015
18abf47
Revert "classInterface: changed excludes flags"
Jun 1, 2015
05500f4
Added merged declarations for ambient class/interfaces
Jun 1, 2015
6b8d033
Merge branch 'master' into mergedDeclarationClassInterface
Jun 1, 2015
6c98c67
added conformance tests
Jun 1, 2015
b864df2
New Baselines for class-interface merging
Jun 1, 2015
ed46bc3
Fixed intendation typo
Jun 1, 2015
d25b910
fixed indentation
Jun 1, 2015
1f74b13
fixed style, added comment
Jun 2, 2015
2d77cbd
cleaner hasNonAmbientClass
Jun 2, 2015
edc4611
removed comma
Jun 2, 2015
a50ab3e
Remove checking in declareSymbol
Jun 2, 2015
90e1955
merge compatiblity now performed in checker.ts
Jun 2, 2015
c629c3f
deleted redundant tests
Jun 2, 2015
e4bc29e
Updated tests
Jun 2, 2015
b293da4
updated baselines
Jun 2, 2015
936aea8
fixed merge conflict
Jun 2, 2015
0917582
removed extra newlines
Jun 2, 2015
fa06f3e
fixed merge conflict.
Jun 2, 2015
fa9b6fc
fixed loops, merged baseline
Jun 3, 2015
9e1ab92
merged with master
Jun 3, 2015
f3278e2
fixed a grammatical issue
Jun 3, 2015
2b899f1
simplified check
Jun 3, 2015
29c9286
Updated error message
Jun 3, 2015
d000e01
updated baselines to reflect new error message
Jun 3, 2015
143890b
New test
Jun 3, 2015
323ce24
new baselines got mergeClassInterfaceAndModule
Jun 3, 2015
365ea3d
Check for ambient context instead of ambient flag
Jun 3, 2015
015c2c1
Merge branch 'master' into mergedDeclarationClassInterface
Jun 3, 2015
5ef426c
new baselines
Jun 3, 2015
3a3479d
New Test and Baseline
Jun 4, 2015
19b0c51
merged master
Jun 17, 2015
91e3a5c
updated baselines
Jun 18, 2015
851c7e4
fixed comment, spacing
Jun 18, 2015
4878cce
merged with master
Jul 2, 2015
c06e5eb
Update test
Jul 2, 2015
3af3177
update baselines
Jul 2, 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
32 changes: 26 additions & 6 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,26 @@ module ts {
return node.name ? declarationNameToString(node.name) : getDeclarationName(node);
}

/* internal */
Copy link
Member

Choose a reason for hiding this comment

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

This isn't exported, so you don't need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

/**
* Checks if the symbol contains a class declaration declaration that is non-ambient.
Copy link
Member

Choose a reason for hiding this comment

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

You should merge these "declaration"s 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks :)

*/
function hasNonAmbientClass(symbol: Symbol): boolean {
if (symbol) {
return !! forEach(symbol.declarations, (element: Declaration, index: number) => {
Copy link
Member

Choose a reason for hiding this comment

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

You don't actually need index.

Copy link
Member

Choose a reason for hiding this comment

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

No space after the !!

Copy link
Member

Choose a reason for hiding this comment

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

Also, this returns a boolean, you don't need the !!.

Copy link
Member

Choose a reason for hiding this comment

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

Consider using a for-of loop as well. Depends on your preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't include !!, the object returned by forEach is truthy/falsy (and when treated as a boolean, its value is unchanged), but !! makes the actual value returned true or false. Given the way the code is written, if symbol is falsy, then the return value is undefined. Should this be changed to a ternary operator? Does it matter from a performance/runtime safety point of view? If not, I'll remove the !!.

Also the style guidelines suggest using ts.forEach instead of for loops, so barring a strong reason to change, I'll keep it as is.

return (element.kind === SyntaxKind.ClassDeclaration && !(element.flags & NodeFlags.Ambient))
Copy link
Member

Choose a reason for hiding this comment

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

No need for parens around the return expression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

});
}
}

/**
* Declares a Symbol for the Node and add it to symbols. Reports errors for conflicting identifier names,
Copy link
Member

Choose a reason for hiding this comment

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

Period, not comma

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove trailing comma.

Thanks for adding these helpful comments!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

* @param symbols - The symbolTable which node will be added to.
* @param parent - If node is in a class, parent denotes the parent declaration.
Copy link
Member

Choose a reason for hiding this comment

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

@JsonFreeman @CyrusNajmabadi does parent only refer to classes?

Copy link
Member

Choose a reason for hiding this comment

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

And if so, just say "denotes that class"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's for any member relationship. So it can be in a class, interface, object type literal, enum or module (if it's a module the member has to be exported to have a parent).

* @param node - The declaration to be added to the symbol table
* @param includes - The SymbolFlags that node has in addition to its declaration type (eg: export, ambient, etc.)
* @param excludes - The flags which node cannot be declared alongside in a symbol table. Used to report forbidden declarations.
*/
function declareSymbol(symbols: SymbolTable, parent: Symbol, node: Declaration, includes: SymbolFlags, excludes: SymbolFlags): Symbol {
Debug.assert(!hasDynamicName(node));

Expand All @@ -144,15 +164,15 @@ module ts {
let symbol: Symbol;
if (name !== undefined) {
symbol = hasProperty(symbols, name) ? symbols[name] : (symbols[name] = createSymbol(0, name));
if (symbol.flags & excludes) {
if (symbol.flags & excludes || (node.kind === SyntaxKind.InterfaceDeclaration && hasNonAmbientClass(symbol))) {
Copy link
Member

Choose a reason for hiding this comment

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

Leave a comment for the case this covers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the comment include a reference either to the github issue or an entry in the spec?
Added something reasonable-seeming.

if (node.name) {
node.name.parent = node;
}

// Report errors every position with duplicate declaration
// Report errors on previous encountered declarations
let message = symbol.flags & SymbolFlags.BlockScopedVariable
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
: Diagnostics.Duplicate_identifier_0;

forEach(symbol.declarations, declaration => {
Expand Down Expand Up @@ -249,7 +269,7 @@ module ts {
// these cases are:
// - node has locals (symbolKind & HasLocals) !== 0
// - node is a source file
setBlockScopeContainer(node, /*cleanLocals*/ (symbolKind & SymbolFlags.HasLocals) === 0 && node.kind !== SyntaxKind.SourceFile);
setBlockScopeContainer(node, /*cleanLocals*/ (symbolKind & SymbolFlags.HasLocals) === 0 && node.kind !== SyntaxKind.SourceFile);
}

forEachChild(node, bind);
Expand Down Expand Up @@ -403,7 +423,7 @@ module ts {
declareModuleMember(node, symbolKind, symbolExcludes);
break;
}
// fall through.
// fall through.
Copy link
Member

Choose a reason for hiding this comment

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

Re-indent this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

default:
if (!blockScopeContainer.locals) {
blockScopeContainer.locals = {};
Expand All @@ -424,7 +444,7 @@ module ts {

function bind(node: Node) {
node.parent = parent;

switch (node.kind) {
case SyntaxKind.TypeParameter:
bindDeclaration(<Declaration>node, SymbolFlags.TypeParameter, SymbolFlags.TypeParameterExcludes, /*isBlockScopeContainer*/ false);
Expand Down Expand Up @@ -516,7 +536,7 @@ module ts {
bindCatchVariableDeclaration(<CatchClause>node);
break;
case SyntaxKind.ClassDeclaration:
bindBlockScopedDeclaration(<Declaration>node, SymbolFlags.Class, SymbolFlags.ClassExcludes);
bindBlockScopedDeclaration(<Declaration>node, SymbolFlags.Class, isAmbientContext(node) ? SymbolFlags.AmbientClassExcludes : SymbolFlags.ClassExcludes);
break;
case SyntaxKind.InterfaceDeclaration:
bindBlockScopedDeclaration(<Declaration>node, SymbolFlags.Interface, SymbolFlags.InterfaceExcludes);
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1473,7 +1473,8 @@ module ts {
EnumMemberExcludes = Value,
FunctionExcludes = Value & ~(Function | ValueModule),
ClassExcludes = (Value | Type) & ~ValueModule,
InterfaceExcludes = Type & ~Interface,
AmbientClassExcludes = (Value | Type) & ~(ValueModule | Interface),
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do the other change I suggested (https://github.com/Microsoft/TypeScript/pull/3333/files#r31552915), then this goes away, and ClassExcludes becomes (Value | Type) & ~(ValueModule | Interface)

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just letting classes and interfaces merge, and reporting an error later on in CheckClassDeclaration for instance if the class is not ambient? just like we do with merging order for module/class and module/class being in different files.

InterfaceExcludes = Type & ~(Interface | Class),
RegularEnumExcludes = (Value | Type) & ~(RegularEnum | ValueModule), // regular enums merge only with regular enums and modules
ConstEnumExcludes = (Value | Type) & ~ConstEnum, // const enums merge only with const enums
ValueModuleExcludes = Value & ~(Function | Class | RegularEnum | ValueModule),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(2,12): error TS2300: Duplicate identifier 'x'.
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(6,5): error TS2300: Duplicate identifier 'x'.
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(10,15): error TS2300: Duplicate identifier 'x'.
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(14,5): error TS2300: Duplicate identifier 'x'.
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(18,13): error TS2300: Duplicate identifier 'x'.
tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts(22,5): error TS2300: Duplicate identifier 'x'.


==== tests/cases/conformance/classes/classDeclarations/classAndInterfaceMergeConflictingMembers.ts (6 errors) ====
declare class C1 {
public x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}

interface C1 {
x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}

declare class C2 {
protected x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}

interface C2 {
x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}

declare class C3 {
private x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}

interface C3 {
x : number;
~
!!! error TS2300: Duplicate identifier 'x'.
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//// [classAndInterfaceMergeConflictingMembers.ts]
declare class C1 {
public x : number;
}

interface C1 {
x : number;
}

declare class C2 {
protected x : number;
}

interface C2 {
x : number;
}

declare class C3 {
private x : number;
}

interface C3 {
x : number;
}

//// [classAndInterfaceMergeConflictingMembers.js]
27 changes: 27 additions & 0 deletions tests/baselines/reference/declaredClassMergedwithSelf.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts(1,15): error TS2300: Duplicate identifier 'C1'.
tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts(3,15): error TS2300: Duplicate identifier 'C1'.
tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts(5,15): error TS2300: Duplicate identifier 'C2'.
tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts(7,11): error TS2300: Duplicate identifier 'C2'.
tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts(9,15): error TS2300: Duplicate identifier 'C2'.


==== tests/cases/conformance/classes/classDeclarations/declaredClassMergedwithSelf.ts (5 errors) ====
declare class C1 {}
~~
!!! error TS2300: Duplicate identifier 'C1'.

declare class C1 {}
~~
!!! error TS2300: Duplicate identifier 'C1'.

declare class C2 {}
~~
!!! error TS2300: Duplicate identifier 'C2'.

interface C2 {}
~~
!!! error TS2300: Duplicate identifier 'C2'.

declare class C2 {}
~~
!!! error TS2300: Duplicate identifier 'C2'.
12 changes: 12 additions & 0 deletions tests/baselines/reference/declaredClassMergedwithSelf.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [declaredClassMergedwithSelf.ts]
declare class C1 {}

declare class C1 {}

declare class C2 {}

interface C2 {}

declare class C2 {}

//// [declaredClassMergedwithSelf.js]
14 changes: 14 additions & 0 deletions tests/baselines/reference/mergedClassInterface.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [mergedClassInterface.ts]
declare class C1 {}

interface C1 {}

interface C2 {}

declare class C2 {}

interface C2 {}

interface C2 {}

//// [mergedClassInterface.js]
19 changes: 19 additions & 0 deletions tests/baselines/reference/mergedClassInterface.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/classes/classDeclarations/mergedClassInterface.ts ===
declare class C1 {}
>C1 : Symbol(C1, Decl(mergedClassInterface.ts, 0, 0), Decl(mergedClassInterface.ts, 0, 19))

interface C1 {}
>C1 : Symbol(C1, Decl(mergedClassInterface.ts, 0, 0), Decl(mergedClassInterface.ts, 0, 19))

interface C2 {}
>C2 : Symbol(C2, Decl(mergedClassInterface.ts, 2, 15), Decl(mergedClassInterface.ts, 4, 15), Decl(mergedClassInterface.ts, 6, 19), Decl(mergedClassInterface.ts, 8, 15))

declare class C2 {}
>C2 : Symbol(C2, Decl(mergedClassInterface.ts, 2, 15), Decl(mergedClassInterface.ts, 4, 15), Decl(mergedClassInterface.ts, 6, 19), Decl(mergedClassInterface.ts, 8, 15))

interface C2 {}
>C2 : Symbol(C2, Decl(mergedClassInterface.ts, 2, 15), Decl(mergedClassInterface.ts, 4, 15), Decl(mergedClassInterface.ts, 6, 19), Decl(mergedClassInterface.ts, 8, 15))

interface C2 {}
>C2 : Symbol(C2, Decl(mergedClassInterface.ts, 2, 15), Decl(mergedClassInterface.ts, 4, 15), Decl(mergedClassInterface.ts, 6, 19), Decl(mergedClassInterface.ts, 8, 15))

19 changes: 19 additions & 0 deletions tests/baselines/reference/mergedClassInterface.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/classes/classDeclarations/mergedClassInterface.ts ===
declare class C1 {}
>C1 : C1

interface C1 {}
>C1 : C1

interface C2 {}
>C2 : C2

declare class C2 {}
>C2 : C2

interface C2 {}
>C2 : C2

interface C2 {}
>C2 : C2

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
declare class C1 {
public x : number;
}

interface C1 {
x : number;
}

declare class C2 {
protected x : number;
}

interface C2 {
x : number;
}

declare class C3 {
private x : number;
}

interface C3 {
x : number;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
declare class C1 {}

declare class C1 {}

declare class C2 {}

interface C2 {}

declare class C2 {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
declare class C1 {}

interface C1 {}

interface C2 {}

declare class C2 {}

interface C2 {}

interface C2 {}