It'd be a shame to remove GeneralLabelOptic just after kcsongor/generic-lens#120 got in... on the other hand I do see that it would be nice to reduce the complexity by dropping the extension point. Would we simply drop any possibility of generic prisms (i.e. they would require TH/manual declarations)?

I like the idea of making partial fields safer and getting AffineTraversals for them. It's a shame HasField doesn't have a nice way to enable this at the moment. But I'm a bit worried by the confusion that could result if partial fields use HasField to get a lens in the absence of a Generic instance, but a generic affine traversal if there is such an instance. And it does seem like checking partiality at each use site might get expensive for large datatypes?

A thought that someone else brought up recently: could we use DerivingVia to require explicit (but simple) instance declarations for fields but give the user more control?

I'll continue thinking about this and look at the code, but I'm away on holiday for the next couple of weeks, so may not have much availability for a while.

It'd be a shame to remove GeneralLabelOptic just after kcsongor/generic-lens#120 got in...

The timing is indeed a bit unfortunate, but it was laying there for a couple of months (and my other PRs are still not merged). I'd like optics to provide simple and fast (both at compile and run time) solution, generic-lens currently struggles a little in this department.

Would we simply drop any possibility of generic prisms (i.e. they would require TH/manual declarations)?

I added prisms for constructors having up to 5 fields (should take care of 99% of cases).

But I'm a bit worried by the confusion that could result if partial fields use HasField to get a lens in the absence of a Generic instance, but a generic affine traversal if there is such an instance. And it does seem like checking partiality at each use site might get expensive for large datatypes?

Right, it's a bit problematic. I think the best way would be to offer generic affine traversals as a function, but not integrated as labels to avoid the confusion after we integrate with HasField.

Also, it's better to not encourage usage of partial field selectors as they frankly suck.

Ok, I created Optics.Generic with GField, GAffineField and GConstructor (also moved GPlate there) and modified LabelOptic to use GField and GConstructor if appropriate.

I added support for type changing updates. Only one variant as they compile faster than type-preserving variants from generic-lens (not to mention type-changing ones) and have excellent type inference.

λ> data X ph a b = X1 String a Int | X2 b deriving (Show, Generic)
λ> x1 :: Prism (X ph a b) _ _ _; x1 = #_X1

<interactive>:46:24: error:
    • Found type wildcard ‘_’ standing for ‘X b1 b2 b’
      Where: ‘b1’, ‘b2’, ‘b’ are rigid type variables bound by
               the inferred type of
                 x1 :: Prism
                         (X ph a b) (X b1 b2 b) ([Char], a, Int) ([Char], b2, Int)
               at <interactive>:46:1-39
      To use the inferred type, enable PartialTypeSignatures
    • In the second argument of ‘Prism’, namely ‘_’
      In the type ‘Prism (X ph a b) _ _ _’
      In the type signature: x1 :: Prism (X ph a b) _ _ _

<interactive>:46:26: error:
    • Found type wildcard ‘_’ standing for ‘([Char], a, Int)’
      Where: ‘a’ is a rigid type variable bound by
               the inferred type of
                 x1 :: Prism
                         (X ph a b) (X b1 b2 b) ([Char], a, Int) ([Char], b2, Int)
               at <interactive>:46:1-28
      To use the inferred type, enable PartialTypeSignatures
    • In the third argument of ‘Prism’, namely ‘_’
      In the type ‘Prism (X ph a b) _ _ _’
      In the type signature: x1 :: Prism (X ph a b) _ _ _

<interactive>:46:28: error:
    • Found type wildcard ‘_’ standing for ‘([Char], b2, Int)’
      Where: ‘b2’ is a rigid type variable bound by
               the inferred type of
                 x1 :: Prism
                         (X ph a b) (X b1 b2 b) ([Char], a, Int) ([Char], b2, Int)
               at <interactive>:46:31-39
      To use the inferred type, enable PartialTypeSignatures
    • In the fourth argument of ‘Prism’, namely ‘_’
      In the type ‘Prism (X ph a b) _ _ _’
      In the type signature: x1 :: Prism (X ph a b) _ _ _

λ> x1 :: Prism _ (X ph a b) _ _; x1 = #_X1

<interactive>:47:13: error:
    • Found type wildcard ‘_’ standing for ‘X b1 b2 b’
      Where: ‘b1’, ‘b2’, ‘b’ are rigid type variables bound by
               the inferred type of
                 x1 :: Prism
                         (X b1 b2 b) (X ph a b) ([Char], b2, Int) ([Char], a, Int)
               at <interactive>:47:1-39
      To use the inferred type, enable PartialTypeSignatures
    • In the first argument of ‘Prism’, namely ‘_’
      In the type ‘Prism _ (X ph a b) _ _’
      In the type signature: x1 :: Prism _ (X ph a b) _ _

<interactive>:47:26: error:
    • Found type wildcard ‘_’ standing for ‘([Char], b2, Int)’
      Where: ‘b2’ is a rigid type variable bound by
               the inferred type of
                 x1 :: Prism
                         (X b1 b2 b) (X ph a b) ([Char], b2, Int) ([Char], a, Int)
               at <interactive>:47:31-39
      To use the inferred type, enable PartialTypeSignatures
    • In the third argument of ‘Prism’, namely ‘_’
      In the type ‘Prism _ (X ph a b) _ _’
      In the type signature: x1 :: Prism _ (X ph a b) _ _

<interactive>:47:28: error:
    • Found type wildcard ‘_’ standing for ‘([Char], a, Int)’
      Where: ‘a’ is a rigid type variable bound by
               the inferred type of
                 x1 :: Prism
                         (X b1 b2 b) (X ph a b) ([Char], b2, Int) ([Char], a, Int)
               at <interactive>:47:1-28
      To use the inferred type, enable PartialTypeSignatures
    • In the fourth argument of ‘Prism’, namely ‘_’
      In the type ‘Prism _ (X ph a b) _ _’
      In the type signature: x1 :: Prism _ (X ph a b) _ _

It can even change phantom type variables.

Compile time benchmarks from https://github.com/arybczak/generic-lens/tree/perf-work (cabal build generic-optics:perf) with GHC 8.10.2.

field',field, position' and position are from generic-optics library, L.field is from generic-optics-lite library (@phadej have a look, it's very inefficient).

1 constructor:

2 constructors:

3 constructors:

It uses generics only for setting, so then if the data type is large enough (>10 fields for a pure product) and they don't optimize away it's going to be a couple of times slower than hand-written/TH generated one.

I would prefer that to be an error rather than a (hard to spot in large) slowdown, if I forget to TH generate an instance.

I would prefer that to be an error rather than a (hard to spot in large) slowdown, if I forget to TH generate an instance.

Right, I see your point. I'm just unsure how to solve it in a satisfactory way.

I thought this PR will allow libraries to use labels with optics-core :(

Perhaps we could add a GenericOptics class that has a trivial associated type family (so that we can detect whether a type derived it with the stuck type family trick) for the generic instance to work?

@phadej does 65194d5 (Require GenericLabelOptics instance for generic labels) satisfy you?

@arybczak yes, that is great for me. But indeed, I'm quite sure there are people who would prefer having generic labels for everything on by default. I don't see a way to satisfy everyone (thus why I'm fine with cabal flag for my-pedantic-self).

Yeah. That covers the case of libraries wanting to use generic labels with their data types and applications wanting to use generic labels in general (as if a specific library is missing the instances, orphans can be provided), but it doesn't work for libraries wanting to use generic labels with data types from other libraries not knowing about optics :( Well, they still can, but need to use gfield instead of a label.

Ideally I wanted this to create a sort of compatibility layer for sane field handling with labels from now on until we get SetField - as then the overlappable instance in optics would just use SetField for setting instead of generics and no changes would be necessary to code that uses optics.

But I do see the problem of using generics silently if one forgets to define LabelOptic instances (either by hand or via TH)...

At least this is better than the cabal flag, since with a flag no library can use generic labels.

I'll let it marinate for another week or so, maybe someone will have an enlightenment how to solve it gracefully.

At least this is better than the cabal flag, since with a flag no library can use generic labels.

What about having a manual flag biased towards allowing generic labels:

• By default, labels use generics silently. Libraries are implicitly allowed to assume this will be the case.
• If the flag is set, the generic instance is replaced with one that uses TypeError.

That way users who really care about performance can set the flag at build time and guarantee the absence of generic instances. Of course they might hit the problem that some library they depend on uses a generic instance from another library they depend on, and the build will fail. But if they did explicitly ask for that...

We can alter the overlappable instance with the cabal flag, but I personally am not a fan of having a flag change API in such a subtle way.

I generally agree that flags shouldn't change APIs, but in this case we are really just saying we want to build in "hard mode" as an additional check on the build. And if that is explicitly requested by the person building the package, it seems okay to me.

Really we want some way to (optionally) request a warning when a certain instance is used, which is occasionally requested as a GHC feature.

@adamgundry I was thinking about a similar thing - manual flag, enabled by default and explicit note that turning it off is generally unsupported.

That would render the flag unusable once/if libraries rely on it enough. On the other hand, the GenericLabelOptics approach would most likely mean that labels will never gain enough traction 🙂 at least before SetField.

Choices...

I think I have a middle ground solution.

I added explicit-generic-labels flag to optics-core (disabled by default). When disabled, only the Generic instance is needed for the overlappable LabelOptic instance to fire. When enabled, GenericLabelOptics instance is needed in addition.

This gives more flexibility to application authors as they can enable this flag and patch/send PRs to affected libraries to simply derive GenericLabelOptics (or include specific LabelOptic instances) for offending data types.

@phadej @adamgundry what do you think?

This looks satisfactory to me, I'll merge (this time for sure 😛 ) during the weekend.

But I think it is fine to say they shouldn't normally bother

Libraries shouldn't bother unless they themselves use generic labels with their data types, then it makes sense for them to derive these instances so that the library works with explicit-generic-labels (I added this to the doc).

@adamgundry I applied your suggestions, I think we're good to merge this 🎉