Gen hackery

[endgame]

I had hoped to teach gen how to handle recursive shape definitions in general, but ran out of time and mainline project work re-starts from next week. This at least lets us regenerate everything, as previously rds-data failed to generate due to a three-deep recursive cycle.

This PR teaches gen to handle recursive shape definitions.

[endgame]

@brendanhay I tried generating a Ptr every time we see a type that we've seen before, basically by generalising the condition on the below line to n `elem` seen:

amazonka/gen/src/Gen/AST/Cofree.hs

Line 65 in f73a957

| conseq seen = pure $! n :< Ptr (s ^. info) (pointerTo n s)

When I tried to generate bindings for dynamodb (it's a service I know reasonably well), it failed to generate lens prefixes on labels, like what happened when you tried to generate kendra bindings (reported at #730). This makes me think that there's some rewriting pass or something that doesn't add the prefixes correctly if it sees a Ptr instead of a proper recursive shape; do you have any insights?

Otherwise, I hope we can at least turn the handle on some of the new services, and get a big regenerate in sometime in the next few weeks.

[RobertFischer]

We are going to get rid of the lens stuff, right? So maybe just sit on this until then?

…

On Fri, Mar 11, 2022, 01:15 endgame ***@***.***> wrote: @brendanhay <https://github.com/brendanhay> I tried generating a Ptr every time we see a type that we've seen before, basically by generalising the condition on the below line to n `elem` seen: https://github.com/brendanhay/amazonka/blob/f73a957d05f64863e867cf39d0db260718f0fadd/gen/src/Gen/AST/Cofree.hs#L65 When I tried to generate bindings for dynamodb (it's a service I know reasonably well), it failed to generate lens prefixes on labels, like what happened when you tried to generate kendra bindings (reported at #730 <#730>). This makes me think that there's some rewriting pass or something that doesn't add the prefixes correctly if it sees a Ptr instead of a proper recursive shape; do you have any insights? Otherwise, I hope we can at least turn the handle on some of the new services, and get a big regenerate in sometime in the next few weeks. — Reply to this email directly, view it on GitHub <#761 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE5VZFRMFHYQMTJM7NK33U7LQHDANCNFSM5QOWCBEA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[endgame]

If you mean the PR as a whole, no: we need to be able to regen services.

If you mean the Ptr stuff, then I still think that's unwise: I think there's probably something else afoot that we ignore at our peril. AIUI, the eventual plan for lens is to avoid the lens dependency (so callers can use either generic-lens or generic-optics, but still provide VL lenses for data types under unambiguous names.

[brendanhay]

Need to run this and poke around to rebuild my memory index as nothing concrete comes to mind.

I suspect your intuition is correct, ie. elaborate from the flat JSON parse result (shape) -> recursive cofree, then probably only rename the top-level without descending into the recursive shape to rename all references, or something similarly stupid.

[endgame]

That would be excellent. I wasn't able to grok that part of the generator, so if you get a chance to test my theory, I'd be very grateful.

[endgame]

@brendanhay Starting next week, I should have a couple of work weeks where I can give amazonka some more attention. If you can get a chance to look at the generator before then, I'd really appreciate it. If not, then I intend to merge this to unblock things (we can't gen right now for some services we previously could; I suspect the change to hash functions in hashable ^>=1.4) and see what i can get done.

[endgame]

@brendanhay Some friends and I found what we believe to be a subtle bug in the way the seen list was being tracked inside elaborate: when recursing into refs, the refs would add themselves to the seen list. If the root object adds itself to seen, then it looks like we can unconditionally generate Ptr when we hit a shape that we've seen before. Because seen is cleared each time we explore a value in the input HashMap, we guarantee that we never memoise a Ptr and that the top level structures are always StructF, so generation still proceeds.

It would be great if you could review b3985cd in particular, but I'm happy to merge and get back to fixing services if you're busy.

[endgame]

When I tried to generate bindings for dynamodb (it's a service I know reasonably well), it failed to generate lens prefixes on labels, like what happened when you tried to generate kendra bindings (reported at #730)...

This appears to be fixed, too.

[endgame]

For some libraries with recursive definitions (e.g., wafv2; #736), this version of gen successfully generates code, but the generated library will not build thanks to a module import cycle. Should we generate .hs-boot files for any shapes where we had to create a Ptr?

[endgame]

Notes for myself, for when I next get a chance to hack on this:

I think also it might not be too bad:

• when writing out a type file, if the shape you need to import is a Ptr, do an import {-# SOURCE #-}
• when building the tree of files to write out, generate .hs-boot for files which are referenced at least once by Ptr

[endgame]

@brendanhay The branch in its current form can re-generate all configured services in the current botocore pin, and they all compiled successfully on my machine. Do you think we should merge this?

Future work: when we advance the botocore pin and configure new services, we may have problems with import cycles - wafv2 is the only one I'm aware of. If it's the only one, I think a manual fixup is probably the way to go: suppress generation of its Statement shape, and replace it with a proper sum type a la #724 .

I hit other problems on an updated botocore pin, but I think we do them one at a time:

1. merge this
2. ship config fixups that have current issues/PRs
3. regen all services
4. update botocore, regen all services, fixing up as necessary
5. start configuring new services.

Thoughts? If you're busy, I'll probably merge in the back half of next week.