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

Use ghc-lib-parser with stylish-haskell #293

Closed
wants to merge 81 commits into from

Conversation

felixmulder
Copy link
Contributor

@felixmulder felixmulder commented Jul 23, 2020

This PR contains the following:

  • Use ghc-lib-parser for Data
  • Add module header step
  • Add hardcoded imports step
  • Allows non-converted steps to be run alongside new ones
  • Refactor imports step to use old Options configuration
  • Language pragmas step
  • Align
  • Squash step (completed by GSoC?)

This is at the point where I think we should start discussing the changes.

Known issues

  • The printer monad does not care about line length
  • The imports are not deduplicated
  • Does not support GADTs yet (should be quite trivial) EDIT: Basic GADT support is now included
  • Comments within deriving clauses are hard to put back next to the correct clause. I've opted for placing them before all deriving clauses as an intermediate workaround. We should file an issue for this if we merge this.
  • Haven't implemented getting rid of redundant language pragmas

Scope of PR

I'll be happy to fix most of these issues, but I'm not too keen on doing:

  • Align step
  • Squash step
  • Refactoring imports to use Options datatype
  • Implement removal of redundant language pragmas

Improvements to be had

Since this PR lays the foundation of having a context aware printer monad, we should be able to use it in order to have automatic line breaking etc at appropriate times. I think it's a huge improvement!

@felixmulder
Copy link
Contributor Author

At this point I am starting to feel like it is good enough for review and potentially to merge as a good step in the right direction ✌️

@felixmulder
Copy link
Contributor Author

Thanks @jaspervdj! I fixed all the CI issues now and so this should be in pretty good shape. Looking forward to your review!

@felixmulder felixmulder changed the title [RFC] Use ghc-lib-parser with stylish-haskell Use ghc-lib-parser with stylish-haskell Jul 25, 2020
Copy link
Member

@lukasz-golebiewski lukasz-golebiewski left a comment

Choose a reason for hiding this comment

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

Looks great <3 Thanks Felix!
Just a few nitpicks from my side.
I'd merge right away but I think the honor should be Jasper's

data/stylish-haskell.yaml Outdated Show resolved Hide resolved
src/Main.hs Outdated Show resolved Hide resolved
lib/Language/Haskell/Stylish/Config.hs Outdated Show resolved Hide resolved
showOutputable :: GHC.Outputable a => a -> String
showOutputable = GHC.showPpr baseDynFlags

compareOutputable :: GHC.Outputable a => a -> a -> Ordering
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any other orderings in use? If not maybe it would make sense to extract this to an orphan Ord instance for GHC.Outputable and not use this one explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly used in the imports step. I think it might be a good idea here to invent our type for imports that carries the information we need. This'll probably be nice when we add support for merging imports - so then we add the ordering instance for that type.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I did two things, I changed the newtype representation of:

newtype Imports = Imports [LImportDecl GhcPs]

-- to:

newtype Import = Import (ImportDecl GhcPs)

This makes things a bit easier to reason about, it was possible to define equality for the import individually and I think it'll give us a more intuitive API down the line to work with imports. E.g. I added a helper function canMergeImport which is used in the equality instance. This helper could be used in the Imports step to merge imports - and get better ordering... I think I'm gonna play with that a bit today

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasz-golebiewski @felixmulder

Do we have any other orderings in use? If not maybe it would make sense to extract this to an orphan Ord instance for GHC.Outputable and not use this one explicitly?

This thread caught my eye as I perused this ticket. It looks to me that these lines are covering the same ground as ghc-lib-parser-ex's HsExtendInstances wrapper. I've mused before that ghc-lib-parser-ex might be useful for stylish-haskell to use/extend on its conversion from haskell-src-exts to ghc-lib-parser - would be nice to share these sorts of common utilities with e.g. HLint!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm c.f. showOutputable :: GHC.Outputable a => a -> String with unsafePrettyPrint (from Language.Haskell.GhclibParserEx.GHC.Utils.Outputable) too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that'd be neat 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that'd be neat 👍

Let's let this land and revisit. If you're open to it moving some of this down to ghc-lib-parser-ex, I think we can eliminate this file at least in entirety.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see also that Module.hs has quite a lot of overlap with code under GHC/Util in HLint (e.g. parsing out language pragmas); most code in there hasn't been deemed general enough before now to get pulled down into ghc-lib-parser-ex but I'm starting to see opportunities to coalesce on it now.

Copy link
Member

@jaspervdj jaspervdj left a comment

Choose a reason for hiding this comment

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

This is excellent! I'm holding of on merging for a bit so I can discuss how to merge this with the GSoC work tomorrow.

@jaspervdj
Copy link
Member

@felixmulder I spoke to @VBeatrice and the current plan is for her to finish the Align step. After that we can merge the Squash and Align steps from GSoC into your work. Finally, we'll probably need to do some remaining bits (the Options you mentioned), and removing a bunch of dead code. I can do together with the student and hopefully that'll put us in a good position to make a release.

@felixmulder
Copy link
Contributor Author

felixmulder commented Jul 29, 2020

@jaspervdj - sounds good to me 👍

There are a couple of systemic things that I think we should keep in mind moving forward:

Comments are a pain in the rear. I think we basically have two choices here:

  • Make each step responsible for preserving comments (current approach)
  • Put comment handling into the printer monad

The former puts the onus on the implementer to get comments right for their step. It does give more precision and maybe better formatting(?). However, it's pretty darn error prone since there are so many edge cases.

It could be abstracted into the printer monad. Which makes it easier for the implementer, and arguably - if we want to move line-wrapping into the printer monad, it might make that job easier as well.

Not sure either way is preferable right now, but I don't think we're painting ourselves into a corner either way. The testing covers things fairly well at this point and our monorepo has >50kLOC that we're running this branch of stylish on for each commit since a few weeks back.

@jaspervdj
Copy link
Member

GSoC is over for this year. I'll be working on merging in these changes and doing some final checks this week / weekend.

@jaspervdj jaspervdj mentioned this pull request Sep 14, 2020
13 tasks
@jaspervdj
Copy link
Member

Merged in 250e709, thanks again to everyone involved!

@jaspervdj jaspervdj closed this Oct 3, 2020
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.

None yet

4 participants