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

Python: Bump argo-client, mypy, cryptol versions #1809

Merged
merged 2 commits into from Feb 1, 2023
Merged

Conversation

RyanGlScott
Copy link
Contributor

This patch:

This brings in changes from Cryptol's constraint guards and FFI patches, which
require some light code changes on SAW's end. These changes are mostly taken
from #1751.

Co-authored-by: Iavor Diatchki <iavor.diatchki@gmail.com>
@RyanGlScott
Copy link
Contributor Author

I'm including @yav as a reviewer, since I had to selectively cherry-pick some changes from #1751 in order to bump the cryptol submodule. I think I did this the right way, but please double-check me.

@RyanGlScott
Copy link
Contributor Author

When the CI invokes mypy, it fails with a rather baffling error that I cannot reproduce locally:

saw_client/connection.py:55: error: Assignment expressions are only supported in Python 3.8 and greater  [syntax]

The thing that baffles me is that right before this step, I specifically install Python 3.11, so I have no idea why mypy thinks it is using Python 3.8...

@kquick
Copy link
Member

kquick commented Jan 31, 2023

This patch:

* Bumps the `mypy` lower bounds to avoid incurring a dependency on `typed-ast`,
  which is being EOL'd soon. See python/typed_ast#179.
* Bumps the `argo` submodule to bring in the changes from
  GaloisInc/argo#195, which makes corresponding changes on the `argo` side.
* Bumps the `cryptol` submodule to bring in the changes from
  GaloisInc/cryptol#1493, which makes the corresponding changes on the `cryptol`
  side.
@RyanGlScott
Copy link
Contributor Author

You are exactly right, @kquick. I've pushed a new version of the patch which changes mypy.ini accordingly.

Somewhat distressingly, the mypy.ini files for both argo-client and cryptol make this same mistake, but they were not caught by CI. I suppose we should update those as well...

Copy link
Member

@yav yav left a comment

Choose a reason for hiding this comment

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

I think not handling DForeign and DProp in SAW for the moment is OK.

I just wonder if error is the right way to report these as I am not sure what's SAW's error reporting mechanism. We should make sure that if people use these features in a Cryptol spec and then try to use in SAW they get a normal error, rather than crashing.

@RyanGlScott
Copy link
Contributor Author

Hah, I simply copied the error calls over from #1751 :)

Looking at the code, the existing convention appears to be that error is used for user-reachable code, e.g.,

-- functions
TV.TVFun _aty _bty ->
pure $ V.VFun mempty (error "exportValue: TODO functions")

And panic is used for code that is not intended to be reachable, e.g.,

| otherwise = panic "importFirstOrderValue" ["Unexpected field name while importing finite value:", show nm]

I could be wrong in this assessment, so please speak up if I am wrong here.

@chameco
Copy link
Contributor

chameco commented Feb 1, 2023

That error-handling convention is what I'd expect (at least for existing code that isn't in MonadThrow).

@RyanGlScott RyanGlScott added the ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully. label Feb 1, 2023
@mergify mergify bot merged commit ea968c9 into master Feb 1, 2023
@mergify mergify bot deleted the cryptol-T1491 branch February 1, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants