-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Allow functions and ambient classes to merge #32584
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
Changes from all commits
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 |
---|---|---|
|
@@ -26276,8 +26276,9 @@ namespace ts { | |
|
||
let duplicateFunctionDeclaration = false; | ||
let multipleConstructorImplementation = false; | ||
let hasNonAmbientClass = false; | ||
for (const current of declarations) { | ||
const node = <SignatureDeclaration>current; | ||
const node = <SignatureDeclaration | ClassDeclaration | ClassExpression>current; | ||
const inAmbientContext = node.flags & NodeFlags.Ambient; | ||
const inAmbientContextOrInterface = node.parent.kind === SyntaxKind.InterfaceDeclaration || node.parent.kind === SyntaxKind.TypeLiteral || inAmbientContext; | ||
if (inAmbientContextOrInterface) { | ||
|
@@ -26291,6 +26292,10 @@ namespace ts { | |
previousDeclaration = undefined; | ||
} | ||
|
||
if ((node.kind === SyntaxKind.ClassDeclaration || node.kind === SyntaxKind.ClassExpression) && !inAmbientContext) { | ||
hasNonAmbientClass = true; | ||
} | ||
|
||
if (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.MethodDeclaration || node.kind === SyntaxKind.MethodSignature || node.kind === SyntaxKind.Constructor) { | ||
const currentNodeFlags = getEffectiveDeclarationFlags(node, flagsToCheck); | ||
someNodeFlags |= currentNodeFlags; | ||
|
@@ -26339,6 +26344,16 @@ namespace ts { | |
}); | ||
} | ||
|
||
if (hasNonAmbientClass && !isConstructor && symbol.flags & SymbolFlags.Function) { | ||
// A non-ambient class cannot be an implementation for a non-constructor function/class merge | ||
// TODO: The below just replicates our older error from when classes and functions were | ||
// entirely unable to merge - a more helpful message like "Class declaration cannot implement overload list" | ||
// might be warranted. :shrug: | ||
forEach(declarations, declaration => { | ||
addDuplicateDeclarationError(getNameOfDeclaration(declaration) || declaration, Diagnostics.Duplicate_identifier_0, symbolName(symbol), filter(declarations, d => d !== declaration)); | ||
}); | ||
} | ||
|
||
// Abstract methods can't have an implementation -- in particular, they don't need one. | ||
if (lastSeenNonAmbientDeclaration && !lastSeenNonAmbientDeclaration.body && | ||
!hasModifier(lastSeenNonAmbientDeclaration, ModifierFlags.Abstract) && !lastSeenNonAmbientDeclaration.questionToken) { | ||
|
@@ -31650,7 +31665,7 @@ namespace ts { | |
if (!symbol || !(symbol.flags & SymbolFlags.Function)) { | ||
return false; | ||
} | ||
return !!forEachEntry(getExportsOfSymbol(symbol), p => p.flags & SymbolFlags.Value && isPropertyAccessExpression(p.valueDeclaration)); | ||
return !!forEachEntry(getExportsOfSymbol(symbol), p => p.flags & SymbolFlags.Value && p.valueDeclaration && isPropertyAccessExpression(p.valueDeclaration)); | ||
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. so an ambient class sets SymbolFlags.Value too? I assume that's why we have to 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. Ambient classes do, in fact, count as values, since they do, in fact, resolve in an expression. A declaration being ambient just means it's missing a body - doesn't change its meaning in any other way, really. 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. (The value without a value declaration thing shouldn't happen anymore - it's what caused me to do the "Assignment no longer implies Type or Value" in the other PR for 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. So it sounds like you can revert if it you want, or keep it for future safety. Either way is fine with me. 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. I'd rather keep it - I always feel assuming |
||
} | ||
|
||
function getPropertiesOfContainerFunction(node: Declaration): Symbol[] { | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
=== tests/cases/compiler/ambientClassOverloadForFunction.ts === | ||
declare class foo{}; | ||
>foo : Symbol(foo, Decl(ambientClassOverloadForFunction.ts, 0, 0)) | ||
>foo : Symbol(foo, Decl(ambientClassOverloadForFunction.ts, 0, 20), Decl(ambientClassOverloadForFunction.ts, 0, 0)) | ||
|
||
function foo() { return null; } | ||
>foo : Symbol(foo, Decl(ambientClassOverloadForFunction.ts, 0, 20)) | ||
>foo : Symbol(foo, Decl(ambientClassOverloadForFunction.ts, 0, 20), Decl(ambientClassOverloadForFunction.ts, 0, 0)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,6 @@ declare class foo{}; | |
>foo : foo | ||
|
||
function foo() { return null; } | ||
>foo : () => any | ||
>foo : typeof foo | ||
>null : null | ||
|
Uh oh!
There was an error while loading. Please reload this page.