Skip to content

"Import" suggestions should be before "Change spelling" suggestions #38743

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

Closed
lstkz opened this issue May 23, 2020 · 15 comments
Closed

"Import" suggestions should be before "Change spelling" suggestions #38743

lstkz opened this issue May 23, 2020 · 15 comments
Labels
Experience Enhancement Noncontroversial enhancements External Relates to another program, environment, or user action which we cannot control. Good First Issue Well scoped, documented and has the green light PursuitFellowship Help wanted from Pursuit fellowship; others please avoid until Dec 19
Milestone

Comments

@lstkz
Copy link

lstkz commented May 23, 2020

I face the following situation multiple times per day.

  • Type some code.
  • Variable is not defined/imported.
  • Press ctrl+enter to trigger the quick fixes menu.
  • "Change spelling" is always the first suggestion.
  • Press down arrow and select the import suggestion.

image

TypeScript Version: 3.9.2, 4.0.0-dev

Search Terms:
Import, suggestion, spelling

I tried to google it, but I couldn't find any solutions.

Code

See the screenshot above.

Expected behavior:
The import suggestion should be the first entry in quick fixes.

Actual behavior:
The import suggestion is the last entry in quick fixes.

Related Issues:
#23258 I think it broke it. Also please see the last comment, @krryan mentions the same problem.

@lstkz
Copy link
Author

lstkz commented May 23, 2020

Another example. I must press unnecessary 'down arrow' multiple times.

image

@DanielRosenwasser
Copy link
Member

I guess if you've got an exact match, that is more desirable.

Side note @mjbvz, I wonder if there's a way to group some of these "fix all"s better so that the list doesn't get too long.

@DanielRosenwasser DanielRosenwasser added Experience Enhancement Noncontroversial enhancements Good First Issue Well scoped, documented and has the green light Help Wanted You can do this labels May 26, 2020
@DanielRosenwasser
Copy link
Member

The fix should simply be a matter of reordering when each codefix gets registered by updating the tsconfig.json in services/

@DanielRosenwasser DanielRosenwasser added this to the Backlog milestone May 26, 2020
@mjbvz
Copy link
Contributor

mjbvz commented May 26, 2020

@DanielRosenwasser Opened microsoft/vscode#98613 for 'add all missing imports' coming in the middle of the other import actions

@Gum-Joe
Copy link

Gum-Joe commented Jun 14, 2020

@DanielRosenwasser if it's that simple to fix I'm happy to submit a PR here? Or is there something I am missing, given @mjbvz's comments?

@arunaaabh95
Copy link

arunaaabh95 commented Jun 14, 2020

Hi @Gum-Joe ,
wanted to give a head's up. I can see that in services/tsconfig.json we are already calling importFixes.ts before fixSpelling.ts
I also changed the test case CodeFixSpellingVsImport.ts to check if import fix is being called before change spelling suggestion and I see it passing(it should not according to current behavior)

This test case is similar to what op has posted

/// <reference path='fourslash.ts' />

// Tests that the import fix is returned first.

// @Filename: /abc/a.ts
////export const ChallengeLoader = 0;

// @Filename: /b.ts
////const ChallengeHeader = 0;
////ChallengeLoader;

goTo.file("/b.ts");
verify.codeFixAvailable([
    { description: `Import 'ChallengeLoader' from module "./abc/a"` },
    { description: "Change spelling to 'ChallengeHeader'" },
]);

I might be missing something here.

According to fix for issue #23258 the given problem was handled
TLDR: We are missing something here, task needs more investigation

@Gum-Joe
Copy link

Gum-Joe commented Jun 14, 2020

Thank for the head's up @arunaaabh95
So if this is fixed, presumably it's in TS 3.9, which is bundled in VSCode.
Yet, having just checked this myself, the issue is still there.
My assumption at the moment is that there is some code in VSCode (or whatever handles this -> Language server? Extension?) changing the order of fixes.
A few avenues to explore:

  • Is there another IDE we can test with (besides VSCode Insiders, which might be worth testing)? Does Atom or IntelliJ use the same TS services?
  • Track down how the suggestions are generated and walk through the code. Probably some good old fashioned console.log()ing?
    • We're probably looking at creating some custom VSCode release using a modified version of TS? This could take a while.

Also, where is CodeFixSpellingVsImport.ts BTW?

@arunaaabh95
Copy link

Update: After reading this similar issue #27614. These changes need to be handled on the editor side

@arunaaabh95
Copy link

@Gum-Joe
you can find CodeFixSpellingVsImport.ts in test/cases/fourslahses directory

@Gum-Joe
Copy link

Gum-Joe commented Jun 14, 2020

Thank you @arunaaabh95. I'll probably have a look tonight or at some point myself. It seems the fix for this is quite large and could be a major refactor? I agree with the above comment saying this is not a good first issue, since the fix looks quite big? (still going to work/help with it though)

In the mean time opening a new issue on vscode is probably a good idea?

@arunaaabh95
Copy link

@Gum-Joe I think @DanielRosenwasser and @mjbvz are better judge of this,
I started looking at this issue today itself and this is the first time I have investigated this code base but opening an issue in vscode seems to the right direction to me

@zachkirsch
Copy link

Glad to see there's already an issue files for this! Any update here?

@sandersn sandersn added the PursuitFellowship Help wanted from Pursuit fellowship; others please avoid until Dec 19 label Sep 18, 2020
@snigui
Copy link

snigui commented Oct 1, 2020

Took a look at this issue! Changed the code error number in diagnosticsMessages.json just in case to make sure import comes before check spelling. Don't think this is an issue within TypeScript, but more so with VS Code's suggestions ordering?

@sandersn sandersn added External Relates to another program, environment, or user action which we cannot control. and removed Help Wanted You can do this labels Oct 2, 2020
@sandersn
Copy link
Member

sandersn commented Oct 2, 2020

@mjbvz can you give us an idea of what might be re-ordering suggestions? Emacs has the correct order so I think that eliminates the language service as the cause of this.

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements External Relates to another program, environment, or user action which we cannot control. Good First Issue Well scoped, documented and has the green light PursuitFellowship Help wanted from Pursuit fellowship; others please avoid until Dec 19
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants