Avoid generating implementation signatures in ambient contexts. #19708
Conversation
src/services/codefixes/helpers.ts
Outdated
| @@ -93,14 +93,16 @@ namespace ts.codefix { | |||
| // (eg: an abstract method or interface declaration), there is a 1-1 | |||
| // correspondence of declarations and signatures. | |||
| const signatures = checker.getSignaturesOfType(type, SignatureKind.Call); | |||
| const needsImplementation = !isInAmbientContext(enclosingDeclaration); | |||
| @@ -119,7 +121,7 @@ namespace ts.codefix { | |||
| signatureDeclarations.push(methodDeclaration); | |||
| } | |||
| } | |||
| else { | |||
| else if (needsImplementation) { | |||
There was a problem hiding this comment.
Is it possible that declarations.length > signatures.length but we don't want to add an implementation?
While I'm here, TypeChecker.signatureToSignatureDeclaration has a non-nullable result. Then in signatureToMethodDeclaration it's cast to another non-nullable type. Then it's tested for existence!
There was a problem hiding this comment.
Is it possible that
declarations.length > signatures.lengthbut we don't want to add an implementation?
Good catch, but I don't even understand what that check is for and what it's doing. @aozgaa?
There was a problem hiding this comment.
See this comment (a few lines above): https://github.com/Microsoft/TypeScript/pull/19708/files#diff-6d6034083b71c2ed75493db0fe725ec5R88.
An example (haven't checked by actual debugging) might be
class C {
foo(x: number): void;
foo(x: string): void;
foo(x: number | string) { console.log(x); }
}
declare class D implements C {
}Then I think adding foo to D would be an example.
There was a problem hiding this comment.
I think this check should be inside createMethodImplementingSignatures, which will just omit the body if we are in an ambient context.
EDIT: no, we do not need an implementation signature in this case. Doh.
| optional: boolean, | ||
| typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined, | ||
| parameters: ReadonlyArray<ParameterDeclaration>, | ||
| returnType: TypeNode | undefined) { | ||
| return createMethod( | ||
| /*decorators*/ undefined, | ||
| modifiers, | ||
| /*asteriskToken*/ undefined, | ||
| name, | ||
| optional ? createToken(SyntaxKind.QuestionToken) : undefined, |
There was a problem hiding this comment.
It's wierd to create an optional method with a body even if that's legal.
If the method is optional, though, we don't want to make it look like it must be implemented; maybe just add an // optional, delete if you don't want to implement this comment and let the user decide?
| parameters, | ||
| returnType, | ||
| createStubbedMethodBody()); | ||
| /*returnType*/ undefined, |
There was a problem hiding this comment.
We might as well give them the return type from the signature, right?
There was a problem hiding this comment.
Technically it's unsound to union the return types in most cases, but we could do that in a different PR.
There was a problem hiding this comment.
Intersection instead? It's better that they get an error when trying to return a value from the method, than that they get an error that the class failed to implement the interface, which are always hard to read..
There was a problem hiding this comment.
The intersection will be impossible to fulfill most of the time.
function foo(x: number): number;
function foo(x: string): string;
function foo(x: string | number): ??? {
// An intersection is impossible to return
}All I'm saying is that it's unsound to do the union, but we do allow it because it's just convenient. You're right that we should probably still do it. Just felt weird.
DanielRosenwasser
force-pushed
the
quickFixImplIntAmbientContexts
branch
from
aeeeee5 to
8a211ae
Compare
|
@DanielRosenwasser can you please refresh this PR and address the test failures.. |
|
@DanielRosenwasser can we get these in? |
|
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
Fixes #19703