support add-argument action #3149

santiweight · 2022-09-12T11:22:02Z

This action supports an action "Add argument xxx to function" whenever there is an undefined variable error:

foo = unknown
-- becomes
foo unknown = unknown

The only minor wrinkle is that

foo = True
   where
      bar = unknown
-- becomes
foo unknown = True
   where
      bar = unknown

But this is good enough for now as a matter of pragmatism.

As part of this MR, I fixed some behavior. Previously an unknown-type undefined variable did not offer a "define x" option, but now does:

foo = unknown
-- previously no code actions
-- now becomes
foo = unknown

unknown :: _
unknown = _

kokobd · 2022-09-12T15:16:26Z

Thanks for your contribution!

For reference, below are the behavior of some other language servers/IDEs on this:

rust-analyzer in VSCode: no quickfix
TypeScript in VSCode: no quickfix
Java in Intellij IDEA Community: has this quickfix, but provides a dialog to modify function signature

screenshots

I'm not quite sure about the design, maybe someone else will provide better insight.

santiweight · 2022-09-12T15:29:10Z

Thanks for the screenshots! Very helpful context :) You note that Rust/TS have no quick fix.

I am quite familiar with the IntelliJ interface from my previous job. I would love to support this workflow, but after I looked around the HLS and LSP issues I got the impression that such a pretty workflow would require too much work to justify. Is that you guys' experience? I really think our UX could be best in class given Haskell's type system, but I'm concerned about maintenance hell.

On a side note, it would be really great to have some sort of function:

alterAllUsages :: {- function name -} RdrName -> (LHsExpr -> TransformT m LHsExpr) -> IDEState -> TransformT m IDEState

The idea being that when I add a function parameter to a definition, I can insert appropriate holes in all usages project wide. Does anyone have any thoughts on the feasibility of this? @OliverMadine seems like a candidate for such knowledge!

module One where

import Two

bar = foo True
----
module Two where

foo True = unknown False

----- BECOMES -----
module One where

import Two

bar = foo True _unknown
----
module Two where

foo True unknown = unknown False

OliverMadine · 2022-09-12T16:04:45Z

The idea being that when I add a function parameter to a definition, I can insert appropriate holes in all usages project wide. Does anyone have any thoughts on the feasibility of this? @OliverMadine seems like a candidate for such knowledge!

Yes, it seems very feasible to me!
Most of the logic could be extracted from the rename plugin. It would, however, be subject to the same multi-component limitation of the rename plugin (#2193)

kokobd · 2022-09-13T06:05:18Z

plugins/hls-refactor-plugin/src/Development/IDE/GHC/ExactPrint.hs

@@ -623,6 +653,14 @@ eqSrcSpanA l r = leftmost_smallest l r == EQ
 #endif

 #if MIN_VERSION_ghc(9,2,0)
+
+spanContainsRange :: SrcSpan -> Range -> Bool


Maybe you can use realSrcSpanToRange and isSubrangeOf ?

I forgot to respond. This worked perfectly thanks :)

plugins/hls-refactor-plugin/src/Development/IDE/GHC/ExactPrint.hs

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

pepeiborra · 2022-09-18T14:38:49Z

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

+            pure $ Just [decl']
+          _ -> pure Nothing
+    case runTransformT $ modifySmallestDeclWithM (`spanContainsRange` range) insertArg (makeDeltaAst parsedSource) of
+      Left err -> error $ "Error when inserting argument: " <> err


Avoid throwing an error here, as it will at best kill all other code actions and at worst kill all other plugins. Ideally you should return a ResponseError but that probably requires too much refactoring.

I'm unclear on what to do here. For the time being I used a trace call and returned no diffs. Is ResponseError part of the MonadLSP interface?

Yes it is :)

I summoned up the courage to reveal ResponseErrors in the CodeAction API. The only hold up is I don't know how to throw/report ResponseErrors with MonadLSP after some hoogling.

Anyone have any pointers? 😅 Note that I am returning a list of errors and successes (since there are many potential code actions), so it should just be logResponseError :: MonadLSP m => ResponseError -> m (), not throw

I think it's fine to just use logError or logWarn and return nothing

After some investigation, it appears that the only Recorder available to CodeActions is Recorder (WithPriority E.Log), which is the logging from Development.IDE.Core.Shake.

Which logger should I be using for code actions? I can't find a Logger or nonShake recorder in the parent functions...

This is because this plugin was only recently extracted from ghcide core. The plugin needs to define its own Logger I guess.

plugins/hls-refactor-plugin/src/Development/IDE/GHC/ExactPrint.hs

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

drsooch · 2022-09-27T21:23:50Z

There is a plug-in that attempts to update signatures when it can. change-type-signature It's pretty rudimentary and only works in a handful of cases where GHC provides enough information. When the upstream lsp package updates to the lsp spec 3.17. We should be able to get direct access to GHCs Error Message API and potentially be worthwhile to this case.

santiweight · 2022-10-02T18:26:03Z

@drsooch I've thought about the change-type-sig plugin in this context. I think they're fairly different situations. In change-type-sig, I can only see occurrences where the new type signature is given.

However, in this case, we do not have the full type signature: we only have the type of the argument we're adding. That means we do have to manually alter the HsSigType with the exact print api, which is more complicated.

I implemented a working first attempt (it's surely very buggy for non-trivial layouts): https://github.com/santiweight/haskell-language-server/tree/add-arg-type-sig

pepeiborra

Looks good to me modulo the build failures

michaelpj · 2022-10-10T13:49:54Z

Can you add yourself to CODEOWNERS? You might also want to mention this in features.md.

santiweight · 2022-10-16T02:37:53Z

It seems that the tests didn't run as part of CI? Besides that I've added myself to codeowners but I don't have write access, which is causing an error in the code owners file!

I'm happy to merge whenever now :) Thanks for the help getting this one through!

michaelpj · 2022-10-17T11:45:31Z

It seems that the tests didn't run as part of CI?

They should have, we're testing hls-refactor-plugin which should include the new stuff, right?

Besides that I've added myself to codeowners but I don't have write access, which is causing an error in the code owners file!

I've invited you to the repo :)

michaelpj

I took a fairly superficial look, but I think we can get this over the line :)

Thank you for digging into the code actions in hls-refactor-plugin. As Pepe mentioned, they only recently got extracted from ghcide, so they quite probably be tidied up more to be more in keeping with the style of more recent code action handlers if you felt moved to!

michaelpj · 2022-10-24T21:51:16Z

ghcide/src/Development/IDE/GHC/Error.hs

@@ -119,6 +121,9 @@ p `isInsideSrcSpan` r = case srcSpanToRange r of
  Just (Range sp ep) -> sp <= p && p <= ep
  _                  -> False

+spanContainsRange :: SrcSpan -> Range -> Bool
+spanContainsRange srcSpan range = maybe False (range `isSubrangeOf`) $ srcSpanToRange srcSpan


I think this is clearer as a simple case

Suggested change

spanContainsRange srcSpan range = maybe False (range `isSubrangeOf`) $ srcSpanToRange srcSpan

spanContainsRange srcSpan range = case srcSpanToRange srcSpan of

Just r -> range `isSubrangeOf` r

-- Nice place to explain why this is the right policy

Nothing -> False

And having written it that way, I'm not sure whether we have got the right policy there. Do we want to return False in the "can't turn a src span into a range" case? Should we be returning a Maybe Bool and preserving that information?

At the very least the Haddock should tell you that the returned bool includes this information!

I returned a Nothing and added a comment; I think it's clearer now...

michaelpj · 2022-10-24T21:54:26Z

plugins/hls-refactor-plugin/test/Main.hs

  ]

+#if MIN_VERSION_ghc(9,2,1)
+addFunctionArgumentTests :: TestTree


This is a great time to break the cycle of violence and stop adding things to the single gigantic test file :) put it in another module!

I have already done this in a followup change. Unfortunately fixing this now would cost me some time with conflicts, but I promise to follow up :)

michaelpj · 2022-10-24T21:55:19Z

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

@@ -881,34 +891,95 @@ suggestReplaceIdentifier contents Diagnostic{_range=_range,..}
        = [ ("Replace with ‘" <> name <> "’", [mkRenameEdit contents _range name]) | name <- renameSuggestions ]
    | otherwise = []

+matchVariableNotInScope :: T.Text -> Maybe (T.Text, Maybe T.Text)


Perhaps an annoying suggestion, but these matching functions are all nice pure functions that could benefit from some direct tests checking that they do definitely match all the cases you care about.

Also, this module is also quite large, perhaps the add-action stuff could go in a separate module also?

I actually already did this in a followup MR. Would you be okay with following up with this change (to avoid unnecessary conflicts)?

michaelpj · 2022-10-24T21:59:32Z

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

+suggestNewDefinition ideOptions parsedModule contents Diagnostic {_message, _range}
+  | Just (name, typ) <- matchVariableNotInScope message =
+      newDefinitionAction ideOptions parsedModule _range name typ
+  | Just (name, typ) <- matchFoundHole message,


I think you have a Plan. Something like:

Look for errors of a particular kind

Based on the kind of error, make certain kinds of suggestion

But the Plan is not written down anywhere, and as a reader it's hard to figure out what it is. Maybe worth writing it down somewhere and referring to it?

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs

santiweight · 2022-10-24T23:16:00Z

Thanks for the review @michaelpj

A heads up on the status of this PR... I'm pretty happy with where it is right now, but I'm a little worried about my approach to using the Exactprint API, and so I'm going to attempt reimplementing the code in a better style (by following the examples in ghc-exactprint).

This MR isn't forgotten, just hopefully being replaced by something better...

santiweight · 2022-11-05T18:47:10Z

I was hoping to upstream some of the changes here in ghc-exactprint before merging, but I think that process will take some time.

I've answered the comments from reviewers, and I now think this is ready to merge!

michaelpj · 2022-11-05T22:21:20Z

I'm happy for you to continue improving things, especially since it sounds like you're definitely continuing to work in this area :)

support add-argument action

51e0d1c

kokobd reviewed Sep 13, 2022

View reviewed changes

Merge branch 'master' into add-argument

5206ce2

pepeiborra requested changes Sep 18, 2022

View reviewed changes

respond to review comments

cae508a

santiweight requested a review from eddiemundo as a code owner October 2, 2022 08:57

review: add ability to report errors in CodeAction api

be159c6

pepeiborra approved these changes Oct 8, 2022

View reviewed changes

Santiago Weight added 4 commits October 7, 2022 22:38

review: use already-defined function

3c6fba3

attempts at cpp

4be40b7

fix format error

2627118

fix broken test

9388c54

doc: add self to codeowners; add doc to features.md

3a0a8f9

santiweight requested a review from michaelpj as a code owner October 16, 2022 02:30

Santiago Weight added 4 commits October 15, 2022 19:34

Merge remote-tracking branch 'haskell/master' into add-argument

6bc2454

formatting

57b4edb

formatting

123a4f0

fix an import

9a232d2

Merge branch 'master' into add-argument

01cc14e

michaelpj approved these changes Oct 24, 2022

View reviewed changes

review

f2659a7

Santiago Weight added 2 commits November 5, 2022 11:17

formatting

136c232

add testcase with comments

c39b6f3

Merge branch 'master' into add-argument

34e7277

michaelpj and others added 2 commits November 5, 2022 22:21

Merge branch 'master' into add-argument

fceae39

fix build error

0fce830

santiweight merged commit 9d70df0 into haskell:master Nov 6, 2022

santiweight deleted the add-argument branch November 20, 2022 23:46

santiweight mentioned this pull request Mar 14, 2023

Simpler Tier 1 tactics for case splits #3525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support add-argument action #3149

support add-argument action #3149

santiweight commented Sep 12, 2022 •

edited

Loading

kokobd commented Sep 12, 2022

santiweight commented Sep 12, 2022 •

edited

Loading

OliverMadine commented Sep 12, 2022

kokobd Sep 13, 2022

santiweight Oct 8, 2022

pepeiborra Sep 18, 2022

santiweight Oct 2, 2022

fendor Oct 2, 2022

santiweight Oct 8, 2022

pepeiborra Oct 8, 2022

santiweight Oct 8, 2022

pepeiborra Oct 8, 2022

drsooch commented Sep 27, 2022

santiweight commented Oct 2, 2022 •

edited

Loading

pepeiborra left a comment

michaelpj commented Oct 10, 2022

santiweight commented Oct 16, 2022

michaelpj commented Oct 17, 2022

michaelpj left a comment

michaelpj Oct 24, 2022

michaelpj Oct 24, 2022

santiweight Nov 5, 2022

michaelpj Oct 24, 2022

santiweight Nov 5, 2022

michaelpj Oct 24, 2022

michaelpj Oct 24, 2022

santiweight Nov 5, 2022

michaelpj Oct 24, 2022

santiweight Nov 5, 2022

santiweight commented Oct 24, 2022

santiweight commented Nov 5, 2022

michaelpj commented Nov 5, 2022

-spanContainsRange srcSpan range = maybe False (range `isSubrangeOf`) $ srcSpanToRange srcSpan
+spanContainsRange srcSpan range = case srcSpanToRange srcSpan of
+  Just r -> range `isSubrangeOf` r
+  -- Nice place to explain why this is the right policy
+  Nothing -> False

support add-argument action #3149

support add-argument action #3149

Conversation

santiweight commented Sep 12, 2022 • edited Loading

kokobd commented Sep 12, 2022

santiweight commented Sep 12, 2022 • edited Loading

OliverMadine commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drsooch commented Sep 27, 2022

santiweight commented Oct 2, 2022 • edited Loading

pepeiborra left a comment

Choose a reason for hiding this comment

michaelpj commented Oct 10, 2022

santiweight commented Oct 16, 2022

michaelpj commented Oct 17, 2022

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiweight commented Oct 24, 2022

santiweight commented Nov 5, 2022

michaelpj commented Nov 5, 2022

santiweight commented Sep 12, 2022 •

edited

Loading

santiweight commented Sep 12, 2022 •

edited

Loading

santiweight commented Oct 2, 2022 •

edited

Loading