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

Best-effort support of Qualified Imports in GHC 9.4 #3712

Merged
merged 21 commits into from
Jul 23, 2023

Conversation

konn
Copy link
Collaborator

@konn konn commented Jul 12, 2023

As discussed in #3473, we cannot provide import qualified feature in GHC 9.4 due to the regression in GHC, at least based on the textual information given by error messages.

On the other hand, we can still determine qualified module name from the information of error-range and textual source code.
As I am heavily using Import Qualified feature everyday life, so I try to follow this path to tentatively support it in GHC 9.4.
Even though there can be some corner cases that the method implemented in this PR cannot handle correctly, but even a partial support of this could be still helpful.

TODOs

  • Check if this works with multibyte symbols
  • (Optional) revive adding new import item to existing list along this line This cannot be handled as GHC 9.4 won't give us suggested imports.

@konn
Copy link
Collaborator Author

konn commented Jul 15, 2023

It seems that regression #22130 in GHC removes import suggestion for qualified symbols. This cannot be fixed even with our approach, so I decided to concentrate on reviving add qualified import feature in this PR.

@konn konn marked this pull request as ready for review July 15, 2023 06:57
@konn
Copy link
Collaborator Author

konn commented Jul 15, 2023

I think this PR is ready for review now.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you very much, this is a great addition!

Small documentation things, otherwise, gtg.

plugins/hls-refactor-plugin/test/Main.hs Outdated Show resolved Hide resolved
hls-plugin-api/src/Ide/PluginUtils.hs Outdated Show resolved Hide resolved
@konn
Copy link
Collaborator Author

konn commented Jul 22, 2023

@fendor @michaelpj Thank you for your reviews and sorry for my late response! I had been a bit busy this week. I'm starting responding to the reviews from now.

@konn
Copy link
Collaborator Author

konn commented Jul 22, 2023

I think I addressed all the reviews so far. Thank you for your feedbacks! I mark some threads that as resolved with some changes, but please feel free to unresolve threads if you are unsatisfied with changes.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

Two small comments

Comment on lines 1818 to 1821
mapNotInScope :: (T.Text -> T.Text) -> NotInScope -> NotInScope
mapNotInScope f (NotInScopeDataConstructor d) = NotInScopeDataConstructor (f d)
mapNotInScope f (NotInScopeTypeConstructorOrClass d) = NotInScopeTypeConstructorOrClass (f d)
mapNotInScope f (NotInScopeThing d) = NotInScopeThing (f d)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this function used for anything else? If not, we can likely turn it into a local function (e.g. in the where block) and remove the parameter f with the thing it is supposed to do (adding a ".")

Copy link
Collaborator Author

@konn konn Jul 22, 2023

Choose a reason for hiding this comment

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

Good point! I made it local, changed to more speic name qualify, and made it taking qualifier q instead of general mapping function f.
Addressed in e0eb90a.

@konn
Copy link
Collaborator Author

konn commented Jul 22, 2023

Test failure with GHC 9.2 on Win seems like a genuine regression:

Test suite func-test: RUNNING...
haskell-language-server
  code actions
    redundant import code actions
      remove solitary redundant imports: FAIL (6.48s)
        test\functional\FunctionalCodeAction.hs:259:
        expected: Nothing
         but got: Just (Command {_title = "Execute Code Action", _command = "4884:hlint:codeActionResolve", _arguments = Just [Object (fromList [("data",Object (fromList [("_uri",String "file:///D:/a/haskell-language-server/haskell-language-server/test/testdata/redundantImportTest/src/CodeActionRedundant.hs"),("_value",Object (fromList [("ignoreHintTitle",String "Use module export list"),("tag",String "IgnoreHint"),("verTxtDocId",Object (fromList [("uri",String "file:///D:/a/haskell-language-server/haskell-language-server/test/testdata/redundantImportTest/src/CodeActionRedundant.hs"),("version",Number 0.0)]))]))])),("diagnostics",Array [Object (fromList [("code",String "Use module export list"),("message",String "Use module export list\nFound:\n  module CodeActionRedundant where\nWhy not:\n  module CodeActionRedundant (\n        module CodeActionRedundant\n    ) where\nan explicit list is usually better\n"),("range",Object (fromList [("end",Object (fromList [("character",Number 0.0),("line",Number 0.0)])),("start",Object (fromList [("character",Number 0.0),("line",Number 0.0)]))])),("severity",Number 3.0),("source",String "hlint")])]),("isPreferred",Bool False),("kind",String "quickfix"),("title",String "Ignore hint \"Use module export list\" in this module")])]})

forM_ allActions $ \a -> a ^. L.command @?= Nothing

https:/haskell/haskell-language-server/actions/runs/5629658777/job/15256519280?pr=3712#step:10:211

It seems rather strange as all other versions of GHC on Win passes tests.
Does anybody guess the root cause, or is it just a flaky test?

@fendor
Copy link
Collaborator

fendor commented Jul 22, 2023

I presume flaky error, I rerun the failing CI run.

@konn
Copy link
Collaborator Author

konn commented Jul 23, 2023

@fendor Now it seems that all Testings are green even with GHC 9.4 on Win. Thanks!

It smees Nix build on ubuntu also failed, but it seems due to the storage shortage. So I suppose everything is OK, right?

@fendor
Copy link
Collaborator

fendor commented Jul 23, 2023

@konn CI and I are happy, but you'll need to rebase the PR 😅

@michaelpj michaelpj merged commit 5aa14b3 into master Jul 23, 2023
41 of 43 checks passed
@fendor fendor mentioned this pull request Aug 8, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants