A plugin for GADT syntax converter #2899

July541 · 2022-05-09T09:43:29Z

This plugin is for #2261

~~Currently only work for ghc-9.2.~~ Fully support now!

Known limits:

~~Origin comments missing(I hope gives comments up since they are not easy to put in a proper place).~~
~~(It's not handy, I'll try to keep some comments)~~
It's hard to trace comments, so I decide to put it on hold.

July541 · 2022-05-12T08:48:48Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

+getNextPragma state nfp = handleMaybeM "Error: Could not get NextPragmaInfo" $ do
+      ghcSession <- liftIO $ runAction "GADT.GhcSession" state $ useWithStale GhcSession nfp
+      (_, fileContents) <- liftIO $ runAction "GADT.GetFileContents" state $ getFileContents nfp
+      case ghcSession of


It's from hls-alternate-number-format-plugin, we can't move this to hls-plugin-api unless hls-plugin-api has dependency of ghcide.

I think it would be good to move the pragma-insertion logic to hls-plugin-api from ghcide, since it's a very common thing to want to do. Ideally we could wrap it all up in something like insertPragmaIfNotPresent or something.

Did you figure out whether there was anything sensible we can do to share code here? Or do you want to leave it as a follow-up?

plugins/hls-gadt-plugin/src/Ide/Plugin/GHC.hs

hls-test-utils/src/Test/Hls/Util.hs

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

michaelpj

I'm a little worried that the re-formatting logic looks quite complicated and version-dependent, is this going to be a pain to maintain?

hls-plugin-api/src/Ide/PluginUtils.hs

hls-test-utils/src/Test/Hls/Util.hs

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

michaelpj · 2022-05-13T09:02:16Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

+                _isPreferred = Nothing
+                _disabled = Nothing
+                _edit = Nothing
+                _command = Just


Why are you doing the indirection via a command? You could just compute the edit in the code action handler and return it here!

I don't think your command handler is significantly more expensive - I think it also mostly relies on the parsed module, which you also need here.

I think users won't use this plugin in most cases, it's unfair to give GADT results every time for these cases.

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

michaelpj · 2022-05-13T09:06:19Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

+getNextPragma state nfp = handleMaybeM "Error: Could not get NextPragmaInfo" $ do
+      ghcSession <- liftIO $ runAction "GADT.GhcSession" state $ useWithStale GhcSession nfp
+      (_, fileContents) <- liftIO $ runAction "GADT.GetFileContents" state $ getFileContents nfp
+      case ghcSession of


I think it would be good to move the pragma-insertion logic to hls-plugin-api from ghcide, since it's a very common thing to want to do. Ideally we could wrap it all up in something like insertPragmaIfNotPresent or something.

michaelpj · 2022-05-13T09:07:21Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GHC.hs

+   format, and may have different ident size between constructors.
+
+Hence, we first use `printOutputable` to get an initial GADT syntax,
+then use `ghc-exactprint` to parse the initial result, and finally


Why aren't we just using exactprint for everything? 🤔

michaelpj · 2022-05-13T09:07:58Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GHC.hs

+{-# LANGUAGE TupleSections    #-}
+{-# LANGUAGE TypeApplications #-}
+{-# OPTIONS_GHC -Wno-missing-signatures #-}
+module Ide.Plugin.GHC where


I haven't really reviewed this, it seems complicated and we should probably get one of our exactprint experts to take a look.

I'm not an exactprint expert either, but I have no concerns about the code in this module since it is well commented and isolated in this plugin

July541 · 2022-05-14T12:45:26Z

Why aren't we just using exactprint for everything? 🤔

~~I'm trying to rewrite this, it may not be very struggle.~~

See the next comment.

July541 · 2022-05-14T14:35:43Z

I decide to keep the current design without a better substitute.

Why?

comments with their anchor will be broken after we trans to GADT.

data A where -- comment
-- comment
{- comment -} B | C {-comment-} |
D -- comment
deriving ()

Something like the above is hard to track its semantic place.

Take the following code as example

data (Eq a) => A a = B a

after we convert to GADT syntax, it would be

data           A a   BEqaaAa

while using exactPrint directly. So we need printOutputable to convert it to

data A a
  where
    B :: (Eq a) => a -> A a

and use this to continue later pretty adjustment.

July541 · 2022-05-14T14:42:01Z

I'm a little worried that the re-formatting logic looks quite complicated and version-dependent, is this going to be a pain to maintain?

The re-formatting relies on the behavior of showSDoc, we'd adjust the pretty function if showSDoc has any changes. Seeing that its behavior is stable crossing ghc-8.6 to ghc-9.2, I don't think we have much pain in future.

July541 · 2022-05-14T15:03:53Z

I think it would be good to move the pragma-insertion logic to hls-plugin-api from ghcide, since it's a very common thing to want to do. Ideally we could wrap it all up in something like insertPragmaIfNotPresent or something.

Can't agree more, but currently, pragma insertion functions have dependencies on Development.IDE.GHC.Compat and Development.IDE. we can't do this unless we moved compat part to a new package and extract some util functions from ghcide to hls-plugin-api.

pepeiborra · 2022-05-15T09:24:49Z

I think it would be good to move the pragma-insertion logic to hls-plugin-api from ghcide, since it's a very common thing to want to do. Ideally we could wrap it all up in something like insertPragmaIfNotPresent or something.

Can't agree more

I am not following this conversation, just wanted to make a few points:

his-plugin-api is an interface package with little or no implementation details, whereas ghcide is a library for building ghc-api based LSP servers and is full of implementation details.
The main motivation to keep things out of ghcide is to make it easier to upgrade to new GHC versions, and the direction of travel is usually ghcide ---> <plugin>.
It's also fine to have dependencies across plugins,

Based on those points, I would rather extract the pragma insertion logic to hls-pragma-plugin rather than to hls-plugin-api.

July541 · 2022-05-15T11:57:21Z

I think it would be good to move the pragma-insertion logic to hls-plugin-api from ghcide, since it's a very common thing to want to do. Ideally we could wrap it all up in something like insertPragmaIfNotPresent or something.

Can't agree more

I am not following this conversation, just wanted to make a few points:

his-plugin-api is an interface package with little or no implementation details, whereas ghcide is a library for building ghc-api based LSP servers and is full of implementation details.

The main motivation to keep things out of ghcide is to make it easier to upgrade to new GHC versions, and the direction of travel is usually ghcide ---> <plugin>.

It's also fine to have dependencies across plugins,

Based on those points, I would rather extract the pragma insertion logic to hls-pragma-plugin rather than to hls-plugin-api.

Agree with your words. But maybe we can't extract pragma insertion logic from ghcide to hls-pragma-plugin, because hls-pragma-plugin has dependency of ghcide.

I hope it's better to have a package like hls-utils to accommodate util functions. So we can have hls-utils ---> ghcide ---> <plugin>.

For 2, I think It's time to do #2454 and #2568.

pepeiborra · 2022-05-15T17:44:41Z

Agree with your words. But maybe we can't extract pragma insertion logic from ghcide to hls-pragma-plugin, because hls-pragma-plugin has dependency of ghcide.

I don't see why this would be a problem

I hope it's better to have a package like hls-utils to accommodate util functions. So we can have hls-utils ---> ghcide ---> .

This is a fine option too.

michaelpj · 2022-05-16T09:40:09Z

For some reason I thought we already had hls-plugin-utils. In fact, hls-plugin-api contains lots of stuff that belongs in a "plugin utils" package!

comments with their anchor will be broken after we trans to GADT.

This is non-haddock comments, right? Haddock comments should be attached to their piece of AST so shouldn't be too hard to put in the right place? I'm inclined to not worry about non-Haddock comments and just have that be a limitation of the plugin. Or have them all printed above the new declaration.

Take the following code as example

I don't understand this example. Why would we get nonsense from exactprint like you wrote? Surely if we construct a correct AST then exactprint should emit it properly??

July541 · 2022-05-16T15:24:57Z

Haddock comments should be attached to their piece of AST so shouldn't be too hard to put in the right place

All comments with RealSrcSpan may lose their correct place after converting to GADT. We must attach anchor operation to describe its new place, but I don't have a proper solution.

I'm inclined to not worry about non-Haddock comments and just have that be a limitation of the plugin.

Agree.

Or have them all printed above the new declaration.

Comments without their target are nonsense, I prefer abandon them.

Why would we get nonsense from exactprint like you wrote?

It is because we printed an incomplete GADT decl, it only has a semantic presentation but all place info is for H98 decl, that's why I use printOutputable to get a correct semantic GADT decl and then we do some patch on the the new GADT decl to make it pretty.

michaelpj · 2022-05-18T14:10:45Z

I really don't understand what's going on, sorry :/ But it seems like we should be able to mess with the AST and print it out again nicely, I thought that was half the point of exactprint...

Maybe @alanz can help us?

July541 · 2022-05-18T15:12:28Z

Something that can convert ast's all absolute locations to relative locations is much helpful.

I have been thinking about edit data constructors one by one instead of edit the whole data decl, maybe it can keep comments for us, I'll try this later.

July541 · 2022-05-25T11:01:48Z

Don't have a feasible solution, I want to merge this and left improvement in the future.

cc @michaelpj

michaelpj

Couple of nits, but otherwise I'm happy so long as you're on the hook for maintaining it, @July541 😆

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

michaelpj · 2022-05-25T11:06:49Z

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs

+getNextPragma state nfp = handleMaybeM "Error: Could not get NextPragmaInfo" $ do
+      ghcSession <- liftIO $ runAction "GADT.GhcSession" state $ useWithStale GhcSession nfp
+      (_, fileContents) <- liftIO $ runAction "GADT.GetFileContents" state $ getFileContents nfp
+      case ghcSession of


Did you figure out whether there was anything sensible we can do to share code here? Or do you want to leave it as a follow-up?

July541 · 2022-05-25T11:16:14Z

Couple of nits, but otherwise I'm happy so long as you're on the hook for maintaining it, @July541 😆

Sure, I'm glad to improve it if we have any better solution.

I still think making the change here to pass it when we construct the descriptor makes more sense.

I forgot it, thank you point it out!

Did you figure out whether there was anything sensible we can do to share code here? Or do you want to leave it as a follow-up?

Currently not, let's leave it here, I have the same code in hls-class-plugin. I'll follow #2899 (comment), it's a big work but worth doing.

@michaelpj

* initial hls-gadt-plugin * Correct condition * Render context correctly * Fix typo * Support from ghc-8.6 to ghc-9.0 * ghc8.6 compat * Try to fix test for ghc-8.6 * Pretty name * Add GADTs pragma automatically * Enrich Readme * Clean up * Update hls docs * Update CODEOWNERS * Fix typo * Rename withTempDir with withCanonicalTempDir * Add @michaelpj's suggestions * Pass PluginId through descriptor * Explicit forall

initial hls-gadt-plugin

5881fda

July541 requested review from jneira, Anton-Latukha and Ailrun as code owners May 9, 2022 09:43

July541 added 13 commits May 9, 2022 17:49

Merge master

bfab7a9

Correct condition

fdf37fb

Render context correctly

f3e8148

Fix typo

52298ec

Support from ghc-8.6 to ghc-9.0

54c6d76

ghc8.6 compat

dc989e7

Try to fix test for ghc-8.6

95d941a

Pretty name

875efe3

Add GADTs pragma automatically

5f8f1bc

Enrich Readme

5bec10b

Clean up

4b842f2

Update hls docs

e63c577

Update CODEOWNERS

22c5929

July541 requested review from michaelpj and berberman as code owners May 12, 2022 08:44

Fix typo

0a092f5

July541 commented May 12, 2022

View reviewed changes

plugins/hls-gadt-plugin/src/Ide/Plugin/GHC.hs Outdated Show resolved Hide resolved

July541 changed the title ~~WIP: A plugin for GADT syntax converter~~ A plugin for GADT syntax converter May 12, 2022

Ailrun reviewed May 13, 2022

View reviewed changes

hls-test-utils/src/Test/Hls/Util.hs Show resolved Hide resolved

plugins/hls-gadt-plugin/src/Ide/Plugin/GADT.hs Outdated Show resolved Hide resolved

michaelpj reviewed May 13, 2022

View reviewed changes

Merge branch 'master' into gadt

eced253

July541 mentioned this pull request May 14, 2022

Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier #2906

Closed

July541 added 2 commits May 14, 2022 18:39

Rename withTempDir with withCanonicalTempDir

63e0f77

Add @michaelpj's suggestions

6a65e36

merge master

bdb7838

michaelpj reviewed May 25, 2022

View reviewed changes

michaelpj approved these changes May 25, 2022

View reviewed changes

July541 added 2 commits May 25, 2022 23:50

Pass PluginId through descriptor

5f41198

Explicit forall

004b056

July541 merged commit faa88f6 into haskell:master May 26, 2022

July541 mentioned this pull request Apr 16, 2023

GADT syntax converter throws out all datatype comments #3516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A plugin for GADT syntax converter #2899

A plugin for GADT syntax converter #2899

July541 commented May 9, 2022 •

edited

Loading

July541 May 12, 2022 •

edited

Loading

michaelpj May 13, 2022

michaelpj May 25, 2022

michaelpj left a comment

michaelpj May 13, 2022

michaelpj May 13, 2022

July541 May 14, 2022

michaelpj May 13, 2022

michaelpj May 13, 2022

michaelpj May 13, 2022

pepeiborra May 15, 2022

July541 commented May 14, 2022 •

edited

Loading

July541 commented May 14, 2022

July541 commented May 14, 2022

July541 commented May 14, 2022

pepeiborra commented May 15, 2022

July541 commented May 15, 2022 •

edited

Loading

pepeiborra commented May 15, 2022

michaelpj commented May 16, 2022

July541 commented May 16, 2022

michaelpj commented May 18, 2022 •

edited

Loading

July541 commented May 18, 2022

July541 commented May 25, 2022

michaelpj left a comment

michaelpj May 25, 2022

July541 commented May 25, 2022

A plugin for GADT syntax converter #2899

A plugin for GADT syntax converter #2899

Conversation

July541 commented May 9, 2022 • edited Loading

July541 May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

July541 commented May 14, 2022 • edited Loading

July541 commented May 14, 2022

July541 commented May 14, 2022

July541 commented May 14, 2022

pepeiborra commented May 15, 2022

July541 commented May 15, 2022 • edited Loading

pepeiborra commented May 15, 2022

michaelpj commented May 16, 2022

July541 commented May 16, 2022

michaelpj commented May 18, 2022 • edited Loading

July541 commented May 18, 2022

July541 commented May 25, 2022

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

July541 commented May 25, 2022

July541 commented May 9, 2022 •

edited

Loading

July541 May 12, 2022 •

edited

Loading

July541 commented May 14, 2022 •

edited

Loading

July541 commented May 15, 2022 •

edited

Loading

michaelpj commented May 18, 2022 •

edited

Loading