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

feat(51870): Add a Quick Fix to add an additional parameter to a method or function #56411

Merged
merged 7 commits into from Jan 19, 2024

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #51870

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 15, 2023
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #51870. If you can get it accepted, this PR will have a better chance of being reviewed.

@sandersn
Copy link
Member

I'll bring this PR up at the vscode-TS weekly meeting to decide if we want to add this.

@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Nov 29, 2023
@a-tarasyuk
Copy link
Contributor Author

@sandersn Thanks

@sandersn
Copy link
Member

Here are my notes from the meeting:

  • This codefix sounds good. It replaces something that is tedious to do by hand. C# has this codefix. It would even be nice to have a codefix that deletes parameters when you pass too few.

  • How does it handle overloads? Does it add to the longest matching one? (C# seems to do this)

  • Does it break other callers? (C# does)

  • Should the new parameter be optional? (C# gives codefixes for both; one is a new overload which is equivalent to optional)

  • Is the new parameter always added to the end? Can the new parameter be added in the middle based on which types match?

  • Does it offer the codefix for 'external' code -- basically, code from node_modules. (No idea how other codefixes handle this.)

  • There is a codefix to remove unused parameters from a function declaration. Since it's from the declaration side it shouldn't interfere. But it may edit callers to remove the parameters (Need to test this -- unused parameters should never have callers in order to be 'unused', right?)

@a-tarasyuk
Copy link
Contributor Author

@sandersn Thanks for the feedback.

This codefix sounds good. It replaces something that is tedious to do by hand. C# has this codefix. It would even be nice to have a codefix that deletes parameters when you pass too few.

Are you suggesting implementing a Quick Fix to address this case? I think fixUnusedIdentifier could be expanded to cover this new trigger case.

function f(a: number) {}
f(); // QF to remove a

How does it handle overloads? Does it add to the longest matching one? (C# seems to do this)

Yes, the Quick Fix attempts to utilize the longest overload declaration.

Does it break other callers? (C# does)

Yes, I think we can attempt to use findAllRefs and update other callers. Not sure if it's safe to do, though...

Should the new parameter be optional? (C# gives codefixes for both; one is a new overload which is equivalent to optional)

It's a topic open for discussion. The safest route is to use optional parameters (it depends on the arg position), however, I think if a user calls the function with a new argument, there's an expectation that they want this parameter to be explicitly declared in the signature.

Is the new parameter always added to the end? Can the new parameter be added in the middle based on which types match?

Yes.

Does it offer the codefix for 'external' code -- basically, code from node_modules. (No idea how other codefixes handle this.)

Need to check, I think there's a utility to handle that.

There is a codefix to remove unused parameters from a function declaration. Since it's from the declaration side it shouldn't interfere. But it may edit callers to remove the parameters (Need to test this -- unused parameters should never have callers in order to be 'unused', right?)

Could you clarify this question?

@sandersn
Copy link
Member

sandersn commented Jan 9, 2024

Here's an example for the last question:

function f(x: number, y: number, unused?: number) {
  return x + y
}
f(1, 2)
f(1, 2, 3)

unused is marked as unused whether or not f(1, 2, 3) is in the file. However, I just tested the quick fix, and it's smart enough not to offer to remove unused when f(1, 2, 3) is present. Only the "prefix unused with _" quick fix is offered. When f(1, 2, 3) is not in the file, a second quick fix to remove unused is offered.

In summary, there's nothing to do here; the existing quick fix is smart enough already.

@sandersn
Copy link
Member

sandersn commented Jan 9, 2024

Most of the other questions from the design meeting don't require changing anything, as far as I can see. The only 2 open ones are

  1. whether to offer two code fixes (optional/non-optional) -- I slightly prefer two since I think people will sometimes want non-optional to force themselves to update all callers.
  2. how to avoid offering the quick fix when the callee comes from node_modules.

src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
PR Backlog automation moved this from Waiting on reviewers to Waiting on author Jan 9, 2024
@a-tarasyuk
Copy link
Contributor Author

@sandersn

whether to offer two code fixes (optional/non-optional) -- I slightly prefer two since I think people will sometimes want non-optional to force themselves to update all callers.

Should it be provided for both overload+signature declarations and signature declarations?

function f(a: string): string;
function f(a: string, b: number): string;
function f(a: string, b?: number): string {
    return "";
}
f("", 1, "");

1. Add missing parameter to "f"
2. Add optional parameter to "f"
function f() {}
f("");
1. Add missing parameter to "f"
2. Add optional parameter to "f"

how to avoid offering the quick fix when the callee comes from node_modules.

I came across this utility in another Quick Fix. Need to check if it works correctly, especially in the fourslash context.

if (moduleSourceFile && !isSourceFileFromLibrary(program, moduleSourceFile)) {
return { kind: InfoKind.Function, token, call: parent.parent, sourceFile: moduleSourceFile, modifierFlags: ModifierFlags.Export, parentDeclaration: moduleSourceFile };
}

@sandersn
Copy link
Member

  • I don't think we need to distinguish between adding a parameter to overload signatures vs implementation signatures: to work correctly, you have to add the new parameter to the implementation signature and at least one overload signature.

  • I like the wording "Add missing parameter" vs "Add optional parameter".

@a-tarasyuk
Copy link
Contributor Author

I like the wording "Add missing parameter" vs "Add optional parameter".

@sandersn I've added Add optional parameter QF

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like the design. Just comments and suggestions on the code.

src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
src/services/codefixes/fixAddMissingParam.ts Outdated Show resolved Hide resolved
PR Backlog automation moved this from Waiting on author to Needs merge Jan 19, 2024
@sandersn sandersn merged commit 140fa7e into microsoft:main Jan 19, 2024
19 checks passed
PR Backlog automation moved this from Needs merge to Done Jan 19, 2024
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.4.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Add a Quick Fix to add an additional parameter to a method or function
4 participants