Skip to content

[RPC] Finish cleaning cryptoltypes.py#1317

Merged
m-yac merged 12 commits intomasterfrom
rpc/clean-cryptoltypes-2
Dec 23, 2021
Merged

[RPC] Finish cleaning cryptoltypes.py#1317
m-yac merged 12 commits intomasterfrom
rpc/clean-cryptoltypes-2

Conversation

@m-yac
Copy link
Contributor

@m-yac m-yac commented Dec 21, 2021

This PR finishes cleaning up cryptoltypes.py, specifically:

  • Finishes adding classes for all of the currently implemented Cryptol props (see Cryptol.TypeCheck.TCon and CryptolServer.Data.Type)
  • Adds class for lengthFromTo type operator
  • Adds type annotations for all instance variables
  • Finishes implementing __str__ methods for all Cryptol types
  • Uses @dataclass everywhere possible to avoid writing __repr__ and __eq__ methods
  • Adds more docstrings
  • Adds comments delineating sections

@m-yac m-yac requested a review from pnwamk December 21, 2021 23:47
@m-yac m-yac force-pushed the rpc/clean-cryptoltypes-2 branch from 819cde8 to 9466ccd Compare December 22, 2021 00:44
@m-yac m-yac force-pushed the rpc/clean-cryptoltypes-2 branch from 9466ccd to bc65999 Compare December 22, 2021 00:48
Copy link
Contributor

@pnwamk pnwamk left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments.

@yav
Copy link
Member

yav commented Dec 22, 2021

I haven't followed closely what's been happening with the Python API, but looking at the files here I am a getting worried that we are duplicating a whole bunch of Cryptol in Python, which might lead to maintenance headaches. I am thinking of some of the things in cryptoltypes.py specifically. If we do need such code, I wonder if we could, perhaps, auto-generate it from Haskell, which is likely to be less error prone and then if we make changes in Cryptol, they'll just get reflected in the python client. Thoughts?

@m-yac
Copy link
Contributor Author

m-yac commented Dec 22, 2021

I haven't followed closely what's been happening with the Python API, but looking at the files here I am a getting worried that we are duplicating a whole bunch of Cryptol in Python, which might lead to maintenance headaches. I am thinking of some of the things in cryptoltypes.py specifically. If we do need such code, I wonder if we could, perhaps, auto-generate it from Haskell, which is likely to be less error prone and then if we make changes in Cryptol, they'll just get reflected in the python client. Thoughts?

This is an excellent idea! I have been uneasy about the fact that we're duplicating code here for a while, but I didn't get to the point where I realized we could just generate it. I'm going to experiment with this.

@pnwamk
Copy link
Contributor

pnwamk commented Dec 22, 2021

I haven't followed closely what's been happening with the Python API, but looking at the files here I am a getting worried that we are duplicating a whole bunch of Cryptol in Python, which might lead to maintenance headaches. I am thinking of some of the things in cryptoltypes.py specifically. If we do need such code, I wonder if we could, perhaps, auto-generate it from Haskell, which is likely to be less error prone and then if we make changes in Cryptol, they'll just get reflected in the python client. Thoughts?

I empathize with the overall concerns you describe.

This PR is specifically aimed at tidying up cryptoltypes.py. Perhaps while this is fresh in our minds, we could add some comments in this PR describing not only the "what" but the "why" for the code in here (specifically, why does the python client need to do these things, instead of delegating the work to the server? Some parts might be obvious, but I suspect there are a few design decisions/justifications which are more subtle we might forget from time to time).

As for auto generating the cryptoltypes.py file... it's not clear to me how much of a win/pain that would be. What kinds of changes in Cryptol would actually "break" the code in that file? How often are those likely to happen? Aren't we (or shouldn't we) already be testing for those exact kinds of things in our CI? I'm happy to dive into this further in more detail elsewhere; at the moment in my head it feels like a premature optimization/abstraction given the size and content, I dunno ¯\_(ツ)_/¯

@m-yac m-yac force-pushed the rpc/clean-cryptoltypes-2 branch from bc65999 to 4e4c05e Compare December 22, 2021 21:12
@m-yac
Copy link
Contributor Author

m-yac commented Dec 22, 2021

Inspired by a conversation between me and @pnwamk, I've taken the opposite approach and have deleted as many of these classes as I could. Specifically, I added the TypeOp subclass of CryptolType, which incorporates all of the type level operators (previously Plus, Max, etc.), so if one were to be removed/changed/added, we wouldn't have to update the Python interface. I also did the same for CryptolProps.

I also cleaned up CryptolTypeSchema and used it in the check_type command – so now check_type will work correctly on functions.

Finally, I updated test_types.py to reflect these changes.

@m-yac m-yac force-pushed the rpc/clean-cryptoltypes-2 branch from 4e4c05e to 6685a6b Compare December 22, 2021 21:53
@m-yac m-yac merged commit 34404d7 into master Dec 23, 2021
@m-yac m-yac deleted the rpc/clean-cryptoltypes-2 branch December 23, 2021 20:12
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.

3 participants