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

Consider switching to microlens or overloaded dot syntax? #465

Open
luc-tielen opened this issue Dec 8, 2022 · 9 comments
Open

Consider switching to microlens or overloaded dot syntax? #465

luc-tielen opened this issue Dec 8, 2022 · 9 comments

Comments

@luc-tielen
Copy link

Hi,

I saw that lsp has a dependency on lens, which is a fairly large dependency.
Would it be possible to swap it out for an alternative such as overloaded dot syntax or microlens?
I'm asking this because my compiler's build time almost doubled just including this library.

Are there any things blocking this? Or is there another reason behind it?

@michaelpj
Copy link
Collaborator

I think the main issue is that we're very reliant on two things: a) the lens classy lenses and b) the lens auto-generation machinery.

a) is important because there is a lot of name reuse in the LSP spec. We really need overloaded field accessors, and with lens that means using the classy lenses AFAIK. b) is important because there are so many types, there is no way we could write them by hand.

Maybe once I finally get #458 over the line we could consider writing our own lens generation machinery... but that seems like a lot of work :/ Alternatively I guess something like generic-lens might work. Any other ideas?

@luc-tielen
Copy link
Author

I have to admit I'm not that experienced with lenses and such. 😅
I found this recent thread on reddit: https://www.reddit.com/r/haskell/comments/z0czfc/the_modern_lens_setup_genericlens/
Maybe optics-core provides enough functionality for what you need? (See one of the replies.)

@phadej
Copy link

phadej commented Dec 10, 2022

In any particularly big development you'll have lens. Having microlens will only add microlens.

Consider the install plan for haskell-language-server-1.7.0.0 with GHC-9.0.2.

  • There is no microlens now!
  • hls-plugin-api depends on lens-aeson and thus lens
  • lsp-types depends on lens

I'm actually somewhat surprised that there aren't more dependencies on lens.

In the big picture lens is a small library. E.g. profunctors and semigroupoids are needed by stm-containers. free (which also needs profunctors and semigroupoids) is used by ghc-exactprint. So "big" kmett's lens dependencies are already there. And then vector, unordered-containers, ... are depended upon other stuff, e.g. aeson.

TL;DR if you change lens to microlens in one place, you just add microlens to the mix.


EDIT: generic-lens is slow. Compile-time is slow at use sites, as well as there is no performance guarantees. TH generated lenses are more predicatable in both regards.

@michaelpj
Copy link
Collaborator

I'm considering switching to generic-lens. Reasons:

  • Avoids any lens dependency whatsoever in lsp-types
  • Allows us to use un-prefixed field names in records, which
    • Plays well with RecordDotSyntax for those who want simple usage
    • Makes it easier to get documentation links in the generated code right
    • Is just more natural
  • Lets us drop the creation and compilation of hundreds of lens declarations from lsp-types

I don't know how bad this would be downtream, though.

@michaelpj
Copy link
Collaborator

Trying it out it seems that kcsongor/generic-lens#96 would be annoying for us.

@phadej
Copy link

phadej commented May 19, 2023

Lets us drop the creation and compilation of hundreds of lens declarations from lsp-types

But then you'll do a lot of generics evaluation at compile time at each use site (and GHC is slow to do that). It's far from clear it is a net win (especially if lsp-types is relatively more stable part of codebase).

@michaelpj
Copy link
Collaborator

Yes, I agree the compilation time is maybe a wash. Honestly the thing I find most pleasing is being able to get away from prefixing all the fields with _...

@phadej
Copy link

phadej commented May 19, 2023

With NoFieldSelectors you can have lens and no _ prefix (using custom LensRules and makeLensesWith):

{-# LANGUAGE NoFieldSelectors, TemplateHaskell #-}

import Control.Lens
import Language.Haskell.TH (mkName, nameBase)

data Foo = MkFoo { int :: Int, char :: Char} deriving Show

$(let namer _ty _ns n = [TopName $ mkName $ nameBase n] -- identity namer
      rules =  lensRules & lensField .~ namer
  in makeLensesWith rules ''Foo)

main :: IO ()
main = do
    print $ (MkFoo 21 'x')
      & char .~ 'y'
      & int  %~ (* 2)

Tested with GHC-9.2.7.

@arybczak
Copy link

arybczak commented May 22, 2023

FYI, generic lenses and prisms from optics-core are well-optimized for compile time evaluation (much better than generic-lens, some measurements are available here) and have better ergonomics.

As for the runtime performance, generic lenses at least for pure products always (well, up to 100 fields at minimum) optimize well (this is ensured by tests from https://gitlab.haskell.org/ghc/ghc/-/merge_requests/2965), for sums it's more restrictive (more details are in the MR if you're interested).

A long time ago I've switched a large codebase with about 3k use sites of lenses from TH generated optics to generic optics and compile times with optimizations were pretty much the same (about 15% slower without optimizations).

michaelpj added a commit that referenced this issue Mar 28, 2024
This adopts the approach discussed here:
#465 (comment)

That is:
- We export normal, non-prefixed record selectors (still using
  `DuplicateRecordFields`, of course).
- Users who want lenses can use `generic-lens`; `lsp` and `lsp-test` do
  this.
- It's sensible for `lsp-types` to define some useful lenses that aren't
  derived from fields; these go in a `lsp-types-lens` component.

I think the result is... fine?
kcsongor/generic-lens#96 is a pain in some
cases, but by and large using the generic lenses is quite nice.

I also tried to just use `OverloadedRecordDot` instead of lenses where I
could, since we now support 9.2 as our earliest version. I couldn't
quite get rid of `lens` in `lsp`, it's too useful. I did get rid of it
entirely in `lsp-types`, which was quite painful in at least one place.

This would obviously be a huge breaking change, but I think it's the
right direction.
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

No branches or pull requests

4 participants