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

Add new combinator for getting ALL the dependencies of a decl. #4322

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Sep 12, 2023

Overview

Share isn't properly rendering record fields at the moment: https://unisoncomputing.slack.com/archives/CLGTK464E/p1694523701223369

The reason is that Share only pulls in names for things it knows are referenced from what's being pretty-printed,
BUT there's no actual hard-link between a record and its fields, it will only be rendered as a record if all the proper field definitions exist in the pretty-printer.

Some ideas for how I can solve this:

  1. Re-generate the accessor terms for the decl I'm printing using made-up names, hash them, then use those hashes to search for the real names for those accessors so I can add them to the PPE so they'll be pretty-printed
  2. Generate all the types that I know the accessors would have, then see if I can use the find_type_index to search for the terms which match those types. If I find multiple matches I guess I just include them all?
  3. Give up on being clever and just recursively include every name within the subnamespace which matches the current decl's name.
  4. Redesign how record fields are tracked somehow, maybe add an additional index on Share for it? This would also require changes to unison because currently the accessors are just added to the file as regular terms and after that there's no easy way to figure out where they came from.

I went with 1 for this approach and it's working 👍🏼

Implementation notes

Add a new method for getting the dependencies of a Decl which includes the field accessors.
I had to add a new module to store this in parser-typechecker because we use the typechecker in generating the field accessors.

Test coverage

Tested on local enlil

Loose ends

See enlil PR.

@ChrisPenner ChrisPenner marked this pull request as ready for review September 12, 2023 22:50
@ChrisPenner ChrisPenner self-assigned this Sep 12, 2023
@ChrisPenner ChrisPenner requested review from aryairani and removed request for aryairani September 12, 2023 22:52
@ChrisPenner ChrisPenner marked this pull request as draft September 12, 2023 22:55

-- | Generate Referents for all possible field accessors of a Decl.
-- Returns 'Nothing' if typechecking of any accessor fails (which shouldn't happen).
hashFieldAccessors ::
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled this out of the DeclPrinter and generalized it so we can share.

@ChrisPenner ChrisPenner marked this pull request as ready for review September 13, 2023 18:08
@ChrisPenner ChrisPenner marked this pull request as draft September 13, 2023 18:22
@ChrisPenner ChrisPenner removed the request for review from aryairani September 13, 2023 18:22
@ChrisPenner ChrisPenner marked this pull request as ready for review September 14, 2023 17:21
@aryairani aryairani merged commit 21c587f into trunk Sep 18, 2023
6 checks passed
@aryairani aryairani deleted the cp/accessor-dependencies branch September 18, 2023 18:23
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.

2 participants