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

x/tools/gopls: add a setting to control removal of unused imports, and a quickfix #54362

Open
findleyr opened this issue Aug 10, 2022 · 14 comments
Assignees
Labels
FeatureRequest gopls/imports gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Aug 10, 2022

We see feedback in survey results that users don't want unused imports to be removed by the organizeImports code action. I myself sometimes get annoyed by this behavior: it can be frustrating to manually type out an import path, save, and have the import disappear.

Surprisingly, I don't think we have an open (or closed) request for this feature, unless my search failed me.

I think we should consider a configuration option removeUnusedImports to control the removal of unused imports, and the default should probably be to not to remove unused imports EDIT: I take that back, the default behavior should not change.

@findleyr findleyr added this to the gopls/v0.10.0 milestone Aug 10, 2022
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 10, 2022
@muirdm
Copy link

muirdm commented Aug 10, 2022

When do you need to manually type an import?

This would mean after removing the final use of a package and saving, the import wouldn’t be removed automatically by default? When would the unused imports be cleaned up?

@findleyr findleyr added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 10, 2022
@findleyr findleyr changed the title x/tools/gopls: add a settting to control removal of unused imports x/tools/gopls: consider adding a settting to control removal of unused imports Aug 10, 2022
@findleyr findleyr modified the milestones: gopls/v0.10.0, gopls/later Aug 10, 2022
@findleyr
Copy link
Contributor Author

When do you need to manually type an import?

I sometimes do this to improve completion results, actually, which may suggest an alternative solution.

This would mean after removing the final use of a package and saving, the import wouldn’t be removed automatically by default? When would the unused imports be cleaned up?

Err yes these are good points. I filed this issue (rather nearsightedly) based on consistent feedback we see that this is a point of confusion / frustration. But of course the alternative has significant downsides, which you point out, thanks. I think we need more research into whether we can mitigate this pain point by other means, such as improving unimported completion. I've removed the conviction in the original post, and removed this from the milestone.

@skelouse
Copy link

@findleyr When trying to improve autocomplete rather than typing it into the imports type it into go get -u github.com/..... This will add it to the auto-import mechanism.

@adonovan adonovan changed the title x/tools/gopls: consider adding a settting to control removal of unused imports x/tools/gopls: consider adding a setting to control removal of unused imports Aug 17, 2022
@adonovan
Copy link
Member

An unused import generates a compile error, so we shouldn't leave them in as a convenience. If better completions is the motivation for this feature, we should improve the completion algorithm so that it generates candidates from unimported packages.

The only other case I can think of for wanting this is when temporarily commenting out the last use of an imported package, but usually goimports correctly restores the import when the code us uncommented. In any case, if that's not sufficient, I think the fix should be in goimports, not in suppressing the removal in the first place.

@Major2000

This comment was marked as off-topic.

@findleyr

This comment was marked as off-topic.

@hyangah
Copy link
Contributor

hyangah commented Oct 5, 2022

VS Code users who'd rather like to fix this compilation error manually can turn it off from the settings:

    "[go]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports": false
        }
    },

And explicitly invoke "Organize Import command" manually from the Command Palette when you think your code is ready closer to buildable state.
Screen Shot 2022-10-05 at 3 55 40 PM
As you can see, there is a key binding already too.

(Caveat: it will not automatically add imports either)

@findleyr
Copy link
Contributor Author

We discussed this again, as it consistently comes up, particularly from new users.

I think it would be OK to add a new (non-default) mode to gopls' goimports integration that doesn't delete imports. Particularly with a quickfix to clean up the unused import error, I can see an alternate workflow that works better for some users, where cleaning up unused imports is performed as a separate step.

I personally would continue to use the default behavior, but don't have a problem with supporting this alternate behavior.

@muirdm
Copy link

muirdm commented Aug 11, 2023

Is the main use case manually typing import statements and then losing them on save? Maybe we could detect that and automatically suppress the goimports deletion.

If this feature is mostly requested by new users, the above approach is nice since it gives those users a path to getting used to the full power of goimports.

@findleyr
Copy link
Contributor Author

Yes, we regularly hear feedback from users who are frustrated that after typing an import path and saving, the import is deleted. I personally am used to never typing an import path, but understand that this may not be the workflow for users coming from a different language, or who are working in codebases where goimports doesn't work as well -- our codebases tend to have very few dependencies, and so goimports generally does a good job picking the right one.

@findleyr findleyr changed the title x/tools/gopls: consider adding a setting to control removal of unused imports x/tools/gopls: add a setting to control removal of unused imports, and a quickfix Aug 15, 2023
@aathan
Copy link

aathan commented Aug 15, 2023

Just to provide a little more color re golang/vscode-go#2930: This may be due to being unfamiliar with some of the tooling features, but in my experience so far, it is not possible to "never" type an import path (can you elaborate on how this is achieved?) ... e.g., if an import is for a third party module, e.g., one provided from a git repo etc, I'm not sure I see how that import could possibly be inserted for me -- at least not until I've done a "go get" or otherwise manually having added it to the project somehow. Then, the issue is that "go get" (again, I may be showing my noob-iness here), won't do the right thing if there is no pre-existing import reference to the project.

That's where there's a frustrating loop that occurs: You try to add the import, the import gets deleted by reformat-on-save, you can't do the proper "go get" etc.

I guess a workflow would be to add the import and use it somewhere in the file before saving, but you may be relying on symbol completion etc to really use it properly vs entering some dummy use.

Maybe another approach would be to add features to the reformatter such that if a module is imported and not used, but it also doesn't appear in go.mod etc properly, then it is not removed, and instead a comment is added //unused: not in go.mod ?

EDIT: btw, in my setup, the format tool is set to "default" so I guess it's gopls that's doing the import reformatting on save.

EDIT2: After re-reading the rest of the comments here, I wanted to add that maybe something strange or again, related to my noob-iness, was occurring. See the issue linked at the top of this comment for a hopefully clear-enough description based on my vague understanding of what was going on.

EDIT3: Another idea which may obviate the need for a toggle to disable goimports import cleanup: Provide an action which adds an import explicitly, marking it as I suggested above with a comment. The comment can then automatically be removed once the import is in fact used in the code. If the comment format is simple enough, users can also add the import manually without invoking the command.

@findleyr findleyr added this to the gopls/v0.15.0 milestone Oct 9, 2023
@jichanbachan
Copy link

My input may not be particularly useful as I am very new to Go, but after being slightly frustrated by having my imports deleted before I could use them, I attempted to see if I could turn it off. I understand now that unused imports are treated as compile errors, but it was a bit unfortunate that my only options as a newbie were to get used to it or turn it off entirely. For me, I run into this when I am typing out an import statement and immediately hitting save before using anything from the package.

I would like to suggest an option to simply have the unused import be commented out, so that it avoids the compile error without removing all traces of what was there.

@aathan
Copy link

aathan commented Oct 29, 2023

note that there is an option to save without formatting

@data-pata
Copy link

data-pata commented Nov 26, 2023

note that there is an option to save without formatting

@aathan, Can you elaborate?

"editor.formatOnSave": false doesn't change the behavior of gopls with regard to organizing imports. See above.
(I'm assuming that you refer to VS Code settings).

@findleyr findleyr self-assigned this Jan 12, 2024
@findleyr findleyr modified the milestones: gopls/v0.15.0, gopls/v0.16.0 Jan 22, 2024
@findleyr findleyr modified the milestones: gopls/v0.16.0, gopls/v0.17.0 May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls/imports gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

10 participants