Skip to content

Make getSupportedCodeFixes plugin test such that we dont need to update its baseline for every new error #53070

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.

Sign up for GitHub

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
sheetalkamat merged 3 commits into from
Mar 2, 2023
Merged

Make getSupportedCodeFixes plugin test such that we dont need to update its baseline for every new error #53070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57fe782
Make getSupportedCodeFixes plugin test such that we dont need to upda…
sheetalkamat Mar 2, 2023
42ccb16
Add assert
sheetalkamat Mar 2, 2023
fc5f60f
reset errorCodeToFixesArray
sheetalkamat Mar 2, 2023
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
4 changes: 3 additions & 1 deletion src/services/codeFixProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function createCodeFixActionWorker(fixName: string, description: string, changes
/** @internal */
export function registerCodeFix(reg: CodeFixRegistration) {
for (const error of reg.errorCodes) {
errorCodeToFixesArray = undefined;
errorCodeToFixes.add(String(error), reg);
}
if (reg.fixIds) {
Expand All @@ -59,9 +60,10 @@ export function registerCodeFix(reg: CodeFixRegistration) {
}
}

let errorCodeToFixesArray: readonly string[] | undefined;
/** @internal */
export function getSupportedErrorCodes(): readonly string[] {
return arrayFrom(errorCodeToFixes.keys());
return errorCodeToFixesArray ??= arrayFrom(errorCodeToFixes.keys());
Copy link
Member

@jakebailey jakebailey Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Codefixes are registered at runtime, so if this call happens too early it may get locked in forever with a reduced list.

Can we add something like errorCodeToFixesArray = undefined to registerCodeFix above just to make sure this function is always accurate? It's unlikely, but, seems like it'd be a bit annoying to have to debug if we did accidentally cause this to happen. 😅

(I want to get rid of this registration mechanism at some point, as it's side-effect based and could be redone post-modules, but, that's another change.)

Copy link
Member Author

@sheetalkamat sheetalkamat Mar 2, 2023

Choose a reason for hiding this comment

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

right now how this is setup is that this is immutable list and is correct when called. I will go ahead and add assert in register code fix though

Copy link
Member

@jakebailey jakebailey Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You'd be surprised; I know of a few projects which add more on:

Copy link
Member Author

@sheetalkamat sheetalkamat Mar 2, 2023

Choose a reason for hiding this comment

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

wow .. thats just misusing it.. their plugin instead needs to implement getSupportedCodeFixes on LS

Copy link
Member

@jakebailey jakebailey Mar 2, 2023

Choose a reason for hiding this comment

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

An assert would stop those cases from functioning, right?

Copy link
Member Author

@sheetalkamat sheetalkamat Mar 2, 2023

Choose a reason for hiding this comment

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

An assert would stop those cases from functioning, right?

Yeah for now i have just gone ahead with resetting the list.

Copy link
Member

@jakebailey jakebailey Mar 2, 2023

Choose a reason for hiding this comment

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

To be clear I'm not against breaking this particular mechanism of adding code fixes. I'm curious why they did this; hopefully it was just because they didn't know about getSupportedCodeFixes, but maybe there's a restriction there I'm not thinking of. tsec is one that I think is actively maintained. The roblox one is... confusing because it doesn't add anything.

Copy link

@ArnaudBarre ArnaudBarre Jun 29, 2024

Choose a reason for hiding this comment

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

The restriction is that IDEs don't use the feature so the function is never called and the proxy is useless. I created an issue: microsoft/vscode#219273

Copy link
Member

@jakebailey jakebailey Jun 30, 2024

Choose a reason for hiding this comment

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

Isn't making more people depend on this an even worse direction?

Copy link

@ArnaudBarre ArnaudBarre Jun 30, 2024

Choose a reason for hiding this comment

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

Which direction?
I don't have much opinion on the design, but I think the language server should have an official API to extend code actions.

}

function removeFixIdIfFixAllUnavailable(registration: CodeFixRegistration, diagnostics: Diagnostic[]) {
Expand Down
2 changes: 1 addition & 1 deletion src/testRunner/unittests/tsserver/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ export class TestSession extends ts.server.Session {
}
const response = super.executeCommand(request);
if (this.logger.hasLevel(ts.server.LogLevel.verbose)) {
this.logger.info(`response:${ts.server.indent(JSON.stringify(response, undefined, 2))}`);
this.logger.info(`response:${ts.server.indent(JSON.stringify(response.response === ts.getSupportedCodeFixes() ? { ...response, response: "ts.getSupportedCodeFixes()" } : response, undefined, 2))}`);
this.testhost.baselineHost("After request");
}
return response;
Expand Down
2 changes: 1 addition & 1 deletion src/testRunner/unittests/tsserver/plugins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe("unittests:: tsserver:: plugins overriding getSupportedCodeFixes", () =
return ["b"];
default:
// Make this stable list of single item so we dont have to update the baseline for every additional error
return [info.languageService.getSupportedCodeFixes(fileName)[0]];
return info.languageService.getSupportedCodeFixes(fileName);
}
};
return proxy;
Expand Down
Loading