-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Changes from 32 commits
813d227
18abf47
05500f4
6b8d033
6c98c67
b864df2
ed46bc3
d25b910
1f74b13
2d77cbd
edc4611
a50ab3e
90e1955
c629c3f
e4bc29e
b293da4
936aea8
0917582
fa06f3e
fa9b6fc
9e1ab92
f3278e2
2b899f1
29c9286
d000e01
143890b
323ce24
365ea3d
015c2c1
5ef426c
3a3479d
19b0c51
91e3a5c
851c7e4
4878cce
c06e5eb
3af3177
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,14 @@ namespace ts { | |
return node.name ? declarationNameToString(node.name) : getDeclarationName(node); | ||
} | ||
|
||
/** | ||
* Declares a Symbol for the node and adds it to symbols. Reports errors for conflicting identifier names. | ||
* @param symbolTable - The symbol table which node will be added to. | ||
* @param parent - If node is in a class, parent denotes the parent declaration. | ||
* @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(symbolTable: SymbolTable, parent: Symbol, node: Declaration, includes: SymbolFlags, excludes: SymbolFlags): Symbol { | ||
Debug.assert(!hasDynamicName(node)); | ||
|
||
|
@@ -181,10 +189,11 @@ namespace ts { | |
|
||
let symbol: Symbol; | ||
if (name !== undefined) { | ||
|
||
// Check and see if the symbol table already has a symbol with this name. If not, | ||
// create a new symbol with this name and add it to the table. Note that we don't | ||
// give the new symbol any flags *yet*. This ensures that it will not conflict | ||
// witht he 'excludes' flags we pass in. | ||
// with the 'excludes' flags we pass in. | ||
// | ||
// If we do get an existing symbol, see if it conflicts with the new symbol we're | ||
// creating. For example, a 'var' symbol and a 'class' symbol will conflict within | ||
|
@@ -202,11 +211,11 @@ namespace ts { | |
symbol = hasProperty(symbolTable, name) | ||
? symbolTable[name] | ||
: (symbolTable[name] = createSymbol(SymbolFlags.None, name)); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the leading space if you get the chance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. |
||
if (name && (includes & SymbolFlags.Classifiable)) { | ||
classifiableNames[name] = name; | ||
} | ||
|
||
} | ||
if (symbol.flags & excludes) { | ||
if (node.name) { | ||
node.name.parent = node; | ||
|
@@ -314,6 +323,7 @@ namespace ts { | |
|
||
addToContainerChain(container); | ||
} | ||
|
||
else if (containerFlags & ContainerFlags.IsBlockScopedContainer) { | ||
blockScopeContainer = node; | ||
blockScopeContainer.locals = undefined; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10641,6 +10641,11 @@ namespace ts { | |
}); | ||
} | ||
|
||
// Interfaces cannot be merged with non-ambient classes. | ||
if (symbol.flags & SymbolFlags.Interface && !isInAmbientContext(node)) { | ||
error(node, Diagnostics.Only_an_ambient_class_can_be_merged_with_an_interface) | ||
} | ||
|
||
forEach(node.members, checkSourceElement); | ||
if (produceDiagnostics) { | ||
checkIndexConstraints(type); | ||
|
@@ -10820,13 +10825,24 @@ namespace ts { | |
checkIndexConstraints(type); | ||
} | ||
} | ||
|
||
// Interfaces cannot merge with non-ambient classes. | ||
if (symbol && symbol.declarations) { | ||
for (let declaration of symbol.declarations) { | ||
if (declaration.kind === SyntaxKind.ClassDeclaration && !isInAmbientContext(declaration)) { | ||
error(node, Diagnostics.Only_an_ambient_class_can_be_merged_with_an_interface); | ||
break; | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, you can just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, need to check if the class is in an ambient context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work for interface I {}
declare class I {}
module I {} Since the module occupies the value space, the blanket check looking at the symbol will trigger an error, but the merge should be able to go through. |
||
} | ||
forEach(getInterfaceBaseTypeNodes(node), heritageElement => { | ||
if (!isSupportedExpressionWithTypeArguments(heritageElement)) { | ||
error(heritageElement.expression, Diagnostics.An_interface_can_only_extend_an_identifier_Slashqualified_name_with_optional_type_arguments); | ||
} | ||
checkTypeReferenceNode(heritageElement); | ||
}); | ||
|
||
forEach(node.members, checkSourceElement); | ||
|
||
if (produceDiagnostics) { | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
=== tests/cases/conformance/classes/classDeclarations/classAndInterfaceMerge.d.ts === | ||
|
||
interface C { } | ||
>C : Symbol(C, Decl(classAndInterfaceMerge.d.ts, 0, 0), Decl(classAndInterfaceMerge.d.ts, 1, 15), Decl(classAndInterfaceMerge.d.ts, 3, 19), Decl(classAndInterfaceMerge.d.ts, 5, 15)) | ||
|
||
declare class C { } | ||
>C : Symbol(C, Decl(classAndInterfaceMerge.d.ts, 0, 0), Decl(classAndInterfaceMerge.d.ts, 1, 15), Decl(classAndInterfaceMerge.d.ts, 3, 19), Decl(classAndInterfaceMerge.d.ts, 5, 15)) | ||
|
||
interface C { } | ||
>C : Symbol(C, Decl(classAndInterfaceMerge.d.ts, 0, 0), Decl(classAndInterfaceMerge.d.ts, 1, 15), Decl(classAndInterfaceMerge.d.ts, 3, 19), Decl(classAndInterfaceMerge.d.ts, 5, 15)) | ||
|
||
interface C { } | ||
>C : Symbol(C, Decl(classAndInterfaceMerge.d.ts, 0, 0), Decl(classAndInterfaceMerge.d.ts, 1, 15), Decl(classAndInterfaceMerge.d.ts, 3, 19), Decl(classAndInterfaceMerge.d.ts, 5, 15)) | ||
|
||
declare module M { | ||
>M : Symbol(M, Decl(classAndInterfaceMerge.d.ts, 7, 15), Decl(classAndInterfaceMerge.d.ts, 20, 1)) | ||
|
||
interface C1 { } | ||
>C1 : Symbol(C1, Decl(classAndInterfaceMerge.d.ts, 9, 18), Decl(classAndInterfaceMerge.d.ts, 11, 20), Decl(classAndInterfaceMerge.d.ts, 13, 16), Decl(classAndInterfaceMerge.d.ts, 15, 20)) | ||
|
||
class C1 { } | ||
>C1 : Symbol(C1, Decl(classAndInterfaceMerge.d.ts, 9, 18), Decl(classAndInterfaceMerge.d.ts, 11, 20), Decl(classAndInterfaceMerge.d.ts, 13, 16), Decl(classAndInterfaceMerge.d.ts, 15, 20)) | ||
|
||
interface C1 { } | ||
>C1 : Symbol(C1, Decl(classAndInterfaceMerge.d.ts, 9, 18), Decl(classAndInterfaceMerge.d.ts, 11, 20), Decl(classAndInterfaceMerge.d.ts, 13, 16), Decl(classAndInterfaceMerge.d.ts, 15, 20)) | ||
|
||
interface C1 { } | ||
>C1 : Symbol(C1, Decl(classAndInterfaceMerge.d.ts, 9, 18), Decl(classAndInterfaceMerge.d.ts, 11, 20), Decl(classAndInterfaceMerge.d.ts, 13, 16), Decl(classAndInterfaceMerge.d.ts, 15, 20)) | ||
|
||
export class C2 { } | ||
>C2 : Symbol(C2, Decl(classAndInterfaceMerge.d.ts, 17, 20), Decl(classAndInterfaceMerge.d.ts, 22, 18)) | ||
} | ||
|
||
declare module M { | ||
>M : Symbol(M, Decl(classAndInterfaceMerge.d.ts, 7, 15), Decl(classAndInterfaceMerge.d.ts, 20, 1)) | ||
|
||
export interface C2 { } | ||
>C2 : Symbol(C2, Decl(classAndInterfaceMerge.d.ts, 17, 20), Decl(classAndInterfaceMerge.d.ts, 22, 18)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
=== tests/cases/conformance/classes/classDeclarations/classAndInterfaceMerge.d.ts === | ||
|
||
interface C { } | ||
>C : C | ||
|
||
declare class C { } | ||
>C : C | ||
|
||
interface C { } | ||
>C : C | ||
|
||
interface C { } | ||
>C : C | ||
|
||
declare module M { | ||
>M : typeof M | ||
|
||
interface C1 { } | ||
>C1 : C1 | ||
|
||
class C1 { } | ||
>C1 : C1 | ||
|
||
interface C1 { } | ||
>C1 : C1 | ||
|
||
interface C1 { } | ||
>C1 : C1 | ||
|
||
export class C2 { } | ||
>C2 : C2 | ||
} | ||
|
||
declare module M { | ||
>M : typeof M | ||
|
||
export interface C2 { } | ||
>C2 : C2 | ||
} |
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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
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).