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

Don't suggest imports from deprecated modules #2949

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amesgen
Copy link
Contributor

@amesgen amesgen commented Jun 12, 2022

Closes #2415

I tested this manually for now; but I will also try to come up with a regression test, will remove draft status when that is done.

Concerning the concrete changes in behavior, have a look at this project, specifically the file src/B.hs:

ex1 :: String
ex1 = hello <> " " <> foo

ex2 :: [a] -> Int -> a
ex2 l i = l !? i

ex3 = offsetTimeIncrease

It contains four things which are not in scope, so HLS shows code actions for adding imports for them.

  • hello: A non-deprecated identifier from the module A, but also reexported from the deprecated module AD.
    • Before this PR: HLS offers to import hello from both A and AD.
    • After this PR: HLS offers to import it only from A.
  • foo: A deprecated identifier from a deprecated module.
    • Before this PR: HLS offers to import it from AD.
    • After this PR: HLS still offers to import it from AD. My only guess it that this is due to HIE files not containing deprecation information? Also compare to offsetTimeIncrease.
  • !?: The motivation example from Import extension plugin: don't suggest imports from deprecated modules #2415, now works as expected.
  • offsetTimeIncrease: A deprecated identifier from the non-deprecated module System.Time.Extra.
    • Before this PR: HLS offers to import it from Extra and System.Time.Extra.
    • After this PR: HLS does not offer to import it at all.

Any opinions on whether this behavior is fine?

@amesgen amesgen force-pushed the completion-ignore-deprecated branch from b211bc5 to 16270c2 Compare June 12, 2022 16:24
@michaelpj
Copy link
Collaborator

This is a bit tricky, I think your behaviour seems reasonable, but we should capture the design in a comment somewhere.

Things are nicer for completion items, where you can explicitly tag them as deprecated...

@July541
Copy link
Collaborator

July541 commented Jun 14, 2022

I'd prefer to mark deprecated items as deprecated instead of hiding them. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem

@michaelpj
Copy link
Collaborator

I'd prefer to mark deprecated items as deprecated instead of hiding them

This PR is only about code actions (despite the branch name), so that doesn't apply. For completions, yeah, we should do that if we don't already.

@amesgen
Copy link
Contributor Author

amesgen commented Jun 14, 2022

I'd prefer to mark deprecated items as deprecated instead of hiding them.

Thanks for pointing that out, I agree. Right now, this PR works by completely forgetting that deprecated identifiers exist, so I am now changing that such that the completion code can take that into account (right now, e.g. deprecated are not marked as such in the completions).

Also, nice touch that the deprecated property is deprecated 😄 (in favor of tags).

@pepeiborra
Copy link
Collaborator

Is this ready for review?

@pepeiborra pepeiborra added the status: unfinished Status for PRs that have been semi-abandoned label Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: unfinished Status for PRs that have been semi-abandoned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import extension plugin: don't suggest imports from deprecated modules
4 participants